Deprecating docstring control: Zope Start may get blocked by Products #1202
Comments
|
drfho wrote at 2024-4-2 13:39 -0700:
Hello @d-maurer,
I am sorry to report the following error launching zope:
after merging "**Preliminary step to deprecate docstring based publication control**" #1197 into main-branch 3b32e2e Zope fails starting with a type error calling [Products.CMFCore](https://github.com/zopefoundation/Products.CMFCore)
I am not sure where the problem may be located:
a bug due to the code-change of Zope or Products.CMFCore must get compatible with the new code?
Any hints/comments?
Thank you for your report!
I am on holidays; when I am back, I will look into this in detail.
...
File "/home/zope/vpy38_zms/lib/python3.8/site-packages/Products/CMFCore/__init__.py", line 129, in initialize
utils.ToolInit('CMF Core Tool', tools=tools,
File "/home/zope/vpy38_zms/lib/python3.8/site-packages/Products/CMFCore/utils.py", line 602, in initialize
context.registerClass(
File "/home/zope/src/zopefoundation/Zope/src/App/ProductContext.py", line 206, in registerClass
m[name] = zpublish_wrap(method)
File "/home/zope/src/zopefoundation/Zope/src/ZPublisher/__init__.py", line 152, in zpublish_wrap
sig = signature(callable)
...
TypeError: <Products.CMFCore.utils.ToolInit object at 0x7f03f136c2e0> is not a callable object
...
The code in `ProductContext` tries to automatically mark
constructors (mentioned in a `registerClass`) as publishable.
Because they might be used for other purposes as well,
the mark is not set on the constructor itself but
a wrapper with identical signature is created and marked.
The `signature` call used to determine the signature fails.
The name (`method`) indicates that the `method` should be callable
and `inspect.signature` can handle a wide variety of callables
**but** obviously not the one used by `CMFCore`.
The code in `ZPublisher.zpublish_wrap` will need to be extended
to handle this kind of "callable".
More when I am back.
|
|
I see the same in a Plone 6.0.10 site when I use a checkout of Zope master. (Actually a different branch of a PR that I am testing.) When I do a try/except to swallow the error and add some printing, I see the error happens several times: So:
|
|
I have looked into this problem: likely it comes from an abuse of the The problem (but not the abuse) will disappear once I assume that the I recommend a new branch for |
|
The An alternative approach could be to issue a warning when |
|
Thank you very much @d-maurer for your detailed feedback.
Looking forward to any comments. |
|
drfho wrote at 2024-4-22 10:03 -0700:
...
2. And now, when adding the change to Zope _main_ branch, maybe the prohibitive consequences to very crucial components that often motivate the usage of Zope (like Plone or ZMS) were not obvious. But now we know that important multipiers like Plone or ZMS may get blocked from using the Zope version 5.9.1.
3. So, instead of just cutting off important components like CMFCore (and even other Products?) it might be more appropriate to shift this Zope change back to its branch or adding backwards compatibility to the new code, maybe generating a log info like "will be soon deprecated".
Looking forward to any comments.
I plan to implement the approach from my latest comment:
check whether the parameter passed to `zpublish_wrap` is indeep
callable (as it should)
if not, return the parameter unchanged (instead of a wrapper for it)
but issue a warning.
Hopefully, the warning will cause maintainers of the affected products
(`Products.CMFCore` and `Products.CMFPlone`) to fix the `constructors` abuse.
Source comments in `Products.CMFCore.utils` indicate that the original
developpers have been aware of using `constructors` in a not fully proper way.
|
|
I created #1205 to provide a work around for the design bug in |
|
I have a draft PR in Plone to test with a checkout of Zope and using its versions.cfg: I already see some changes are needed because some tests use a mock version of a request that has no |
This avoids a deprecation warning for using a non callable constructor in Zope higher than 5.9. See report in zopefoundation/Zope#1202
|
The full Plone 6.0 test suite is finished, and the good news is that only the I would be in favour of my Zope PR to make Zope more lenient here, because I expect several Zope/Plone add-ons will have the same problem. But this case is easy to fix in |
|
Maurits van Rees wrote at 2024-4-23 13:45 -0700:
...
I would be in favour of [my Zope PR](#1206) to make Zope more lenient here, because I expect several Zope/Plone add-ons will have the same problem. But this case is easy to fix in `plone.dexterity`, so that is also an option.
I do not think that the approach in this PR is optimal:
`ZPublisher.BaseRequest.DefaultPublisher` is essentially there
to allow an application the customization of the traversal steps
(intermediate, final).
Because the publication control is a security feature, we may not
want that it is inadvertantly lost because an application wants some
changes regarding the traversal steps.
This suggests to move the `ensure_publishable` call out of `DefaultPublisher`
into `BaseRequest.traverse` with the following consequence:
however an application decides to determine the objects to get
published, `traverse` will then check whether it will allow
the publication.
Of course, an application might want to take over control of
the publication policy. Should the need arrive, we can follow
the "Publisher" example: provide a default implementation with
the possiblity to override with an adapter registration.
Advantages:
* all publication control in `BeseRequest.traverse`,
not split over several classes
In particular, the problem you have observed will disappear.
* security relevant publication control independent from
traversal step control -- which can give higher security
Disadvantage:
Formerly, the publication control has been part of `DefaultPublishTraverse`,
i.e. an application could have changed it by registering an adapter;
this will be lost - i.e. we introduce an incompatibility.
I can work on this proposal should we consider it the way to go.
|
Plone has dozens of places where an You are right though that it is likely that I doubt if moving the check to
Would it make sense to move Maybe fall back to calling My main fear is that changes are being introduced in Zope that seem fine at first glance, and are needed in the long run, but that end up breaking stuff, forcing Plone 6.0 to stick to Zope 5.9 for a long time until the dust has settled, or maybe even forever when too much breaks. Current master seems okay now though, after recent fixes from you and others. |
|
Maurits van Rees wrote at 2024-4-24 08:35 -0700:
> Disadvantage: Formerly, the publication control has been part of `DefaultPublishTraverse`, i.e. an application could have changed it by registering an adapter; this will be lost - i.e. we introduce an incompatibility.
Plone has dozens of places where an `IDefaultPublishTraverse` adapter is implemented. For example [here in `plone.namedfile`](https://github.com/plone/plone.namedfile/blob/6.3.0/plone/namedfile/scaling.py#L442-L475) the `publishTraverse` method is used to get an image or a scale of an image. So if you mean you want to remove support for defining an `IDefaultPublishTraverse`, this would break Plone in lots of places.
Of course, I do not mean to remove support for `IPublishTraverse` adaptation:
I want to move the publication control (i.e.
currently the "has docstring"-, later "is marked for publication"-check)
out of it into `BaseRequest.traverse`.
In your referenced `plone.namedfile` adaptation
("https://github.com/plone/plone.namedfile/blob/6.3.0/plone/namedfile/scaling.py#L442-L475"), I see that you have removed the publication control
(i.e. the "has docstring" test).
Was this "by purpose" (i.e. you do not want any publication control)
or accidentally (publication control is sumewhat orthogonal to
traversal and therefore easily forgotten)?
The first case would speak against, the latter for my proposal.
You are right though that it is likely that `ensure_publishable` never gets called in this case. That is perfectly fine in `plone.namedfile`, because the images and scales are very much meant to be published, but there may be other cases where we do want the check. I have good hope though that in all current cases in Plone, all such adapters already only return objects that are really meant for being published.
I doubt if moving the check to `BaseRequest.traverse` is good: that sounds like the publishable check would be done whenever any code traverses to an item, even when this is not the item that would in the end get published.
Yes, but that is precisely the case since ages --
the "docstring requirement" does not apply only to the final published
object but to any traversed object (unless an
`IPublishTraverse` adapter says something else).
You likely are not aware of this because almost all container objects
have a docstring.
But maybe that case follows a completely different code path. For clarity let me explain what I mean by this case, because it is easy to have a different understanding:
* The browser requests localhost:8080/Plone/statistics_view.
* `statistics_view` is a browser view that I just made up, and for this view `ensure_publishable` gets called. Good.
* This browser view has a `__call__` method that does this: `total = len(self.context.restrictedTraverse("portal_catalog").getAllBrains()); return f"total items: {total}"`. This line does traversal, but here we should **not** call `ensure_publishable`, because the `portal_catalog` is not the item that gets published.
When I remember right, `restrictedTraverse` performs an access
(not a publication) check not only to the final but also to all
intermediate traversed objects.
`BaseRequest.DefaultPublishTraverse` does something similar for
the URL traversal (checks all traversed objects, not only the final one).
Would it make sense to move `ensure_publishable` to the adapters, and call it in `BaseRequest.traverseName` after calling `publishTraverse`? So something like this:
```
if IPublishTraverse.providedBy(ob):
ob2 = ob.publishTraverse(self, name)
if hasattr(ob, "ensure_publishable"):
ob.ensure_publishable(ob2)
else:
adapter = queryMultiAdapter((ob, self), IPublishTraverse)
if adapter is None:
adapter = DefaultPublishTraverse(ob, self)
ob2 = adapter.publishTraverse(self, name)
if hasattr(adapter, "ensure_publishable"):
adapter.ensure_publishable(ob2)
```
Maybe **but**
the publication control is a security feature; it should be active
by default and only be made inactive based on an explicit decision,
not accidentally because you have registered an adapter to control
a different aspect.
I am with you if you would like to give an adapter a way
to disable publication control, something like:
```
if IPublishTraverse.providedBy(ob):
adapter = object
else:
adapter = queryMultiAdapter((ob, self), IPublishTraverse)
if adapter is None:
adapter = DefaultPublishTraverse(ob, self)
ob2 = adapter.publishTraverse(self, name)
disable_publication_control = getattr(adapter,
"disable_publication_control"
False)
if callable(disable_publication_control):
disable_publication_control = disable_publication_contral(req, ob2)
if not disable_publication_control:
...ensure_publishable(ob2)
```
On the other hand, it is easy to mark an object (`ob2` above)
as designed for publication. Thus, the additional complexity might
not be necessary.
Maybe fall back to calling `DefaultPublishTraverse.ensure_publishable` if this method is not defined on `ob` or `adapter`.
Okay from me in principle as long as publication control is active by default
and only deactivated based on an explicit decision (not accidentally,
inadvertantly).
**BUT** do we really need additional complexity here?
It is so easy to mark an object (most likely its class) as
designed for publication. Why should traversal adapters require
a customization?
If we really need publication control customization,
we should provide it orthogonal to the traversal,
e.g. be definition an `IPublicationControl` interface
and a corresponding adapter.
My main fear is that changes are being introduced in Zope that seem fine at first glance, and are needed in the long run, but that end up breaking stuff, forcing Plone 6.0 to stick to Zope 5.9 for a long time until the dust has settled, or maybe even forever when too much breaks. Current master seems okay now though, after recent fixes from you and others.
You already have created a `plone.dexterity` PR to resolve its
`_ensure_publishable` related test problems -- and reported
that the other Plone components have no such problems.
Thus, there is no need for a change in `Zope` at the moment.
Maybe, we consider my proposal
(ensure publication control is active by default, independent
of traversal related adaptation) for `Zope 6`.
We a new major version, some incompatibilities may be acceptable.
|
Understood now. I misinterpreted a comment.
Hard to say. Maybe a bit of both. At least whatever we return is always some browser view that either returns an original image or a scale, so they are always meant to be published. In I suspect this is true in 99% of the cases in core Plone: in a
Okay, I did not know that. There is always something new to learn in Zope. Thanks.
That sounds good to me. Thanks for your thoughts and fixes @d-maurer and others. I think we can close this issue now. |
|
Maurits van Rees wrote at 2024-4-24 11:42 -0700:
...
In `plone.restapi` I see a lot of cases like [this one](https://github.com/plone/plone.restapi/blob/848dff077a29cc5ceb52fc2373b3bf74d51cef49/src/plone/restapi/services/users/get.py#L265), where the traversal code consumes a name from the url, and then returns `self`, in which case publication control is not needed:
But it does not hurt (and publication control is cheap).
|
Hello @d-maurer,
I am sorry to report the following error launching zope:
after merging "Preliminary step to deprecate docstring based publication control" #1197 into main-branch 3b32e2e Zope fails starting with a type error calling Products.CMFCore
I am not sure where the problem may be located:
a bug due to the code-change of Zope or Products.CMFCore must get compatible with the new code?
Any hints/comments?
Log
List of Versions
The text was updated successfully, but these errors were encountered: