Preliminary step to deprecate docstring based publication control

[d-maurer]

This PR addresses #774.

It introduces the decorator ZPublisher.zpublish to explicitly mark a class, method or function definition as designed to be publishable by ZPublisher. If a class is marked in this way, its instances become publishable.

In the long term, zpublish decoration should replace the implicit publication control via the existence of a docstring. If the environment variable ZPUBLISHER_DEPRECATE_DOCSTRINGS has a non-empty value, then a DocstringWarning (derived from DeprecationWarning) is issued when a object gets published because it has a docstring. This should help component authors to find locations where explicit zpublish decoration may become necessary in the future. If ZPUBLISHER_DEPRECATE_DOCSTRINGS has a non-empty value and Python's warning filter does not yet have a special rule for ZPublisher.BaseRequest.DocstringWarning, then such a rule is added. If the value is a meaningful filter action, then it, otherwise "default", is used as action for this rule.

Typically, zpublish is used to mark a class or function/method as designed for publication. However, a decoration with zpublish(False) explicitly marks the class (more precisely its instances) or function/method explicitly as not publishable, overriding a potentially existing docstring.
Following the review suggestion from @perrinjerome, zpublish(methods=...) where ... is either a single request method name or a sequence of request method names marks a method as publishable only for the mentioned request methods.

The full signature of zpublish is publish=True, *, methods=None. If methods in not None, publish must be True. In this case, methods is either a single request method name or a sequence of request method names; those request method names specify the request methods for which the object is publishable.

ZPublisher contains the related functions zpublish_marked and zpublish_wrap. zpublish_marked checks whether there is a zpublishable mark for an object, zpublish_wrap can return a zpublishable signature preserving wrapper for a callable that lacks its own zpublishable mark. This is used by registerClass to automatically add zpublishable marks to a registered class and its constructors.

zpublish uses an attribute for publication control. Therefore, derived classes inherit the control by default, unlike as for docstrings.
Another consequence is that (other) decorators must preserve attributes of a decorated function; otherwise, the attribute set by zpublish may get lost.
Currently, the decorator AccessControl.requestmethod.requestmethod does not fulfill this requirement.

[perrinjerome]

It's really good to see this happening !

Did you consider also defining the request methods in zpublish ? something similar to this:

@zpublish(methods=('PUT', 'POST'))
def only_publishable_for_put_and_post_methods(self):
  ...

@zpublish
def publishable_for_any_methods(self):
  ...

This would be a bit similar to how routes are configured in flask ( https://flask.palletsprojects.com/en/3.0.x/quickstart/#http-methods ). "this is publishable" and "this is publishable, but only for these http methods" are very related concepts, it might make sense to define the two at the same time. This would probably deprecate AccessControl.requestmethod.requestmethod.

[d-maurer]

Jérome Perrin wrote at 2024-2-17 04:59 -0800:

@perrinjerome commented on this pull request. It's really good to see this happening ! Did you consider also defining the request methods in `zpublish` ? something similar to this:

No -- this functionality is already provided by `AccessControl.requestmethod.requestmethod` (but we will need to fix this decorator because currently it does not preserve the wrapped attributes).

```py @zpublish(methods=('PUT', 'POST')) def only_publishable_for_put_and_post_methods(self): ... @zpublish def publishable_for_any_methods(self): ... ``` This would be a bit similar to how routes are configured in flask ( https://flask.palletsprojects.com/en/3.0.x/quickstart/#http-methods ). "this is publishable" and "this is publishable, but only for these http methods" are very related concepts, it might make sense to define the two at the same time. This would probably deprecate `AccessControl.requestmethod.requestmethod`.

I was happy with the `requestmethod` of `AccessControl`. But, we might also put request method control into `zpublish`.

... - Fix redirections to URLs with host given as IP-literal with brackets. Fixes `#1191 <#1191>`_. +- Introduce the decorator ``ZPublisher.zpublish` to explicitly ```suggestion - Introduce the decorator ``ZPublisher.zpublish`` to explicitly ```

Thank you for this fix.

... + @zpublish @security.protected(webdav_access) def listDAVObjects(self): return [] this method had no docstring before, it would become publishable with this change, I don't know if this is intentional.

It has not been intensional. Thank you.

[d-maurer]

Jérome Perrin wrote at 2024-2-17 04:59 -0800:

@perrinjerome commented on this pull request. It's really good to see this happening ! Did you consider also defining the request methods in `zpublish` ? something similar to this: ```py @zpublish(methods=('PUT', 'POST')) def only_publishable_for_put_and_post_methods(self): ... @zpublish def publishable_for_any_methods(self): ... ``` ... This would probably deprecate `AccessControl.requestmethod.requestmethod`.

I have rethought about your proposal and I agree now: `AccessControl` is the wrong place to check the request method, `ZPublisher` should do that. I will revise the PR. There will be one caveat: publishability is checked during traversal and `ZPublisher` calls an instance without the traversal to its `__call__` method. This means that the effective `zpublish` mark of `__call__` is the one for the instance. Therefore, we cannot have other methods with less strict request method requirements than `__call__`. If necessary, we could special case `__call__` access (as it is done for the `AccessControl` access rights) and check there a potential request method restriction.

[dataflake]

I really like this stuff. I always thought the docstring method was too obscure and contrived. It led to many methods and functions with empty or nonsense docstrings, just to trigger the publisher. The decorator is very explicit and easy to understand.

[d-maurer]

Jens Vagelpohl wrote at 2024-2-20 03:17 -0800:

... > @@ -502,6 +490,7 @@ def traverse(self, path, response=None, validated_hook=None): self.roles = getRoles( object, '__call__', object.__call__, self.roles) + self.ensure_publishable(object.__call__, True) This addition represents a behavior change, there was no such check in that part of the code before. Were you just correcting a long-standing omission or did you encounter specific problems?

The behavior is not changed: Formerly, `__call__` has been publishable with or without a docstring; if the `__self__` of the `__call__` was published, it could be called (because the access to `__call__` does not involve a traversal step and the docstring check was only performed in such a step). The PR (mostly) retains this behavior. It does so by changing the default used to look up the publishable mark from `None` to `True`. Thus the default is "publication allowed" unless there is an explicit counter configuration at the `__call__` definition. The motivation to potentially check `__call__` comes from the new request method limitation. We may want to say that `__call__` requires e.g. POST requests. For this, we need some checking hook.

... ```suggestion """the ``zpublish`` indication at *obj*.""" ```

Thank you! Fixed.

[perrinjerome]

This is great !

I tried a bit with ZPUBLISHER_DEPRECATE_DOCSTRINGS enabled and I noticed that some places in Zope are still using docstring based publication:

• all methods from OFS.role.Rolemanager,
  https://github.com/zopefoundation/Zope//blob/1b9f2c73a1f37321b2ca0216333945ef2dfb71b6/src/OFS/role.py#L32

• App.FactoryDispatcher.manage_main
  https://github.com/zopefoundation/Zope//blob/6b4f3c5711045566d4e4b55b818f64e833a604d9/src/App/FactoryDispatcher.py#L165 . This causes a warning when adding an OFS.Image.File from the ZMI.

---

Is there already a plan regarding docstring deprecation and ZPUBLISHER_DEPRECATE_DOCSTRINGS ? I imagine it could be something like:

• Next Zope 5 introduces ZPublisher.zpublish, allows both objects marked with ZPublisher.zpublish or with a docstring (with an optional warning if ZPUBLISHER_DEPRECATE_DOCSTRINGS is set)
• Zope and "main" packages are updated to use ZPublisher.zpublish, once this is done ZPUBLISHER_DEPRECATE_DOCSTRINGS is removed and warnings are issued every time (users can set a warnings filter if it's a problem)
• Zope 6 only allows objects marked with ZPublisher.zpublish.

---

About the warning itself, I see there is one warning for each URL, so if the same object is accessed from different places it will cause a warning each time. Maybe the warning should be not by URL but by object ? If we choose this, I don't know what's the best way, if we just str() the object it might be an acquisition wrapper which will have the acquisition chain ( something like <PythonScript at /script_python used for /folder> ) which would cause lots of different warnings, so
I was thinking maybe we need to special case the case of a method to print the class and the method. I'm not sure what would be the best way of doing this. I tried (just a little bit so this might be wrong) and this might be a starting point:

diff --git a/src/ZPublisher/BaseRequest.py b/src/ZPublisher/BaseRequest.py
index bce4601e0..63b76ab56 100644
--- a/src/ZPublisher/BaseRequest.py
+++ b/src/ZPublisher/BaseRequest.py
@@ -718,7 +718,7 @@ class BaseRequest:
                 "to `ZPublisher.zpublish` decorator or have a docstring to be "
                 "published.")
         if deprecate_docstrings:
-            warn(DocstringWarning(url))
+            warn(DocstringWarning(obj))
 
 
 def exec_callables(callables):
@@ -818,6 +818,7 @@ deprecate_docstrings = environ.get("ZPUBLISHER_DEPRECATE_DOCSTRINGS")
 
 class DocstringWarning(DeprecationWarning):
     def __str__(self):
-        return (f"The object at {self.args[0]} uses deprecated docstring "
-                "publication control. Use the `ZPublisher.zpublish` decorator "
-                "instead")
+        published = self.args[0]
+        return (f"{published.__module__}.{published.__qualname__} was published because it uses "
+                "deprecated docstring publication control. "
+                "Mark it publishable with `ZPublisher.zpublish` decorator instead.")

---

The PR says Preliminary support, all this can be addressed later maybe, this is already good as it is, thanks again.

[d-maurer]

Jérome Perrin wrote at 2024-2-20 23:24 -0800:

@perrinjerome approved this pull request.

I found a severe bug (with `zpublish_wrap` sometimes using an inappropriate signature); I am currently working on a fix.

I tried a bit with `ZPUBLISHER_DEPRECATE_DOCSTRINGS` enabled and I noticed that some places in Zope are still using docstring based publication: * all methods from `OFS.role.Rolemanager`, https://github.com/zopefoundation/Zope//blob/1b9f2c73a1f37321b2ca0216333945ef2dfb71b6/src/OFS/role.py#L32 * `App.FactoryDispatcher.manage_main` https://github.com/zopefoundation/Zope//blob/6b4f3c5711045566d4e4b55b818f64e833a604d9/src/App/FactoryDispatcher.py#L165 . This causes a warning when adding an `OFS.Image.File` from the ZMI.

I have fixed all `DocstringWarning`s revealed by running the (`Zope` local) test suite. Your trial shows that there are more (as had to be expected). I fear it will require a considerable effort to locate and fix them all.

... Is there already a plan regarding docstring deprecation and `ZPUBLISHER_DEPRECATE_DOCSTRINGS` ? I imagine it could be something like: - Next Zope 5 introduces `ZPublisher.zpublish`, allows both objects marked with `ZPublisher.zpublish` or with a docstring (with an optional warning if `ZPUBLISHER_DEPRECATE_DOCSTRINGS` is set) - Zope and "main" packages are updated to use `ZPublisher.zpublish`, once this is done `ZPUBLISHER_DEPRECATE_DOCSTRINGS` is removed and warnings are issued every time (users can set a warnings filter if it's a problem) - Zope 6 only allows objects marked with `ZPublisher.zpublish`.

It will be hard work to switch to `zpublish`. Therefore, I would prefer a longer deprecation period -- something like: Zope 5 introduces `zpublish`; "DEPRECATE_DOCSTRINGS" is off by default Zope 6 has "DEPRECATE_DOCSTRINGS" be default Zope 7 requires `zpublish` by default with the option to retain the Zope 6 behavior

About the warning itself, I see there is one warning for each URL, so if the same object is accessed from different places it will cause a warning each time.

Right. The current way is symmetric with the generated error messages and fits the case that problems are recognized during URL traversal. But it generates more warnings than necessary. I am open for alternatives. Important is that the warnings can come from instance publishing and from function/method publishing. Both cases need to be addressed appropriately. For functions, it may be interesting to learn which module has defined the function. There is one case which calls for URL based warnings -- but we likely can disregard it: in principle, a docstring (or `zpublish` annotation) can come from an instance itself rather than its class. In this case, it would be good to get the URL to the object (rather than its class).

The PR says **Preliminary** support,

The "preliminary" should stress that the PR does not deprecated docstring based publication control but only provides some support for a future deprecation.

all this can be addressed later maybe

I do not think we are in haste: we have time to refine this PR to ensure that the difficult migration to explicit publication control require the least effort possible. I do not mind if you (or others) actively work on improving the PR, e.g. by adding missing `zpublish` annotations or improving the warning generation. For this, you can directly push to its branch.

[d-maurer]

Jérome Perrin wrote at 2024-2-20 23:24 -0800:

... I tried a bit with `ZPUBLISHER_DEPRECATE_DOCSTRINGS` enabled and I noticed that some places in Zope are still using docstring based publication: * all methods from `OFS.role.Rolemanager`, https://github.com/zopefoundation/Zope//blob/1b9f2c73a1f37321b2ca0216333945ef2dfb71b6/src/OFS/role.py#L32 * `App.FactoryDispatcher.manage_main` https://github.com/zopefoundation/Zope//blob/6b4f3c5711045566d4e4b55b818f64e833a604d9/src/App/FactoryDispatcher.py#L165 . This causes a warning when adding an `OFS.Image.File` from the ZMI.

I have provided `zpublish` annotations for methods starting with `manage_` and replaced `AccessControl.requestmethod` by `zpublish` if this has been possible. If you like, you can make further trials.

[dataflake]

Question: Many places that have a zpublish decorator also have a security management decorator. Some places zpublish comes first, other places the security management decorator comes first. What's the best order? Does it matter?

[d-maurer]

Jens Vagelpohl wrote at 2024-2-22 01:25 -0800:

Question: Many places that have a `zpublish` decorator also have a security management decorator. Some places `zpublish` comes first, other places the security management decorator comes first. What's the best order? Does it matter?

Functionally, it does not matter (because the respective decorators preserve the function name and attributes). Should we use `AccessControl.requestmethod.requestmethod` together with `zpublish`, the `requestmethod` decoration must be applied first, i.e. ```python @zpublish @requestmethod(...) def ... ``` `requestmethod` (as it is now) does not preserve the function attributes and therefore will destroy the effect of a previous `zpublish`. Of course, `@zpublish @requestmethod(...)` should get replaced by `@zpublish(methods=...)`.

[d-maurer]

Jérome Perrin wrote at 2024-2-20 23:24 -0800:

... About the warning itself, I see there is one warning for each URL, so if the same object is accessed from different places it will cause a warning each time. Maybe the warning should be not by URL but by object ? If we choose this, I don't know what's the best way, if we just `str()` the object it might be an acquisition wrapper which will have the acquisition chain ( something like `<PythonScript at /script_python used for /folder>` ) which would cause lots of different warnings, so I was thinking maybe we need to special case the case of a method to print the class and the method. I'm not sure what would be the best way of doing this. I tried (just a little bit so this might be wrong) and this might be a starting point:

`DocstringWarning` now handles the cases described by the following table object provided info ============= ======================================================== method full class, method name, source location (if available) function full function name, source location (if available) instance full class name, source location (if available) without own docstring instance with URL own docstring

[d-maurer]

Apparently, additional commits after a review invalidate the review (which is reasonable). Therefore, I will rerequest reviews.
From my point of view, the PR is ready to be merged.

[perrinjerome]

@d-maurer I could not reply earlier, but I looked again and everything looks perfect, thank you !

[d-maurer]

Jérome Perrin wrote at 2024-2-25 05:50 -0800:

@d-maurer I could not reply earlier, but I looked again and everything looks perfect, thank you !

Thank you for your review and your ideas!

[davisagli]

@d-maurer Thank you for your work on this. It was one of my biggest frustrations with Zope.

@mauritsvanrees We should check how this affects Plone.