Avoid a plone.protect warning on index_html documents in portal root.#4281
Avoid a plone.protect warning on index_html documents in portal root.#4281
index_html documents in portal root.#4281Conversation
|
@thet thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
thet
force-pushed
the
thet/4279-index-html
branch
from
bb5d564 to
445420e
Compare
|
Note that it is possible even without this patch to rename an object to |
|
@jenkins-plone-org please run jobs |
1 similar comment
|
@jenkins-plone-org please run jobs |
ale-rt
left a comment
ale-rt
left a comment
There was a problem hiding this comment.
I tried to think about the consequence of this change.
__replaceable__ is used just here:
I did not spot any reason why an index_html object should be marked as replaceable in this method causing a write on read.
For this reason I approve :)
| # Acquire content from here. | ||
| _target = self.__getattr__("index_html") | ||
| result = aq_base(_target).__of__(self) | ||
| setattr(result, "__replaceable__", REPLACEABLE) |
There was a problem hiding this comment.
If you remove this one you have to remove also the one in https://github.com/plone/Products.CMFPlone/pull/4281/changes#diff-108b30c9523956cc50ba0e46e6221344a0f9c01ad8c53e663a11c83ca854f248R171
for consistency (even if it is probably dead code).
There was a problem hiding this comment.
@ale-rt happy to remove it! But in the case of the WebDAV __replaceable I thought it might be OK, as this is a NullResource and someone might want to replace it with real content named index_html. But honestly, I'm guessing because I don't really know what the __replaceable__ even is.
Btw. I know some people still using WebDAV and IMO it could be a killer feature of Plone if it would work fine.
There was a problem hiding this comment.
@thet @ale-rt __replaceable__ is used in https://github.com/zopefoundation/Zope/blob/c927fc8b004c624b1c86f359be00d138e98c34f7/src/OFS/ObjectManager.py#L122 to prevent giving a new item a name that can already be found by getattr -- unless the object is marked as replaceable.
So this was kind of there to handle exactly the problem that you're trying to solve (being able to replace the index_html even though it is a method). Except the _check_for_collision in plone.base is not as smart as checkValidId in Zope, and doesn't pay attention to __replaceable__.
tl;dr: I think this change is ok as long as we merge it together with the one you made in plone.base
There was a problem hiding this comment.
@ale-rt I'm undecided - is it now better to remove the __replaceable__ attribute setting for the WebDAV NullResource or not?
There was a problem hiding this comment.
I think that anyway having a Plone server behind Zserver is very unlikely.
If somebody has issues he will ask for a change.
ale-rt
left a comment
ale-rt
left a comment
There was a problem hiding this comment.
But first please fix the consistency issue.
`index_html` are often used to create default pages in containers. On the portal root this was prevented by a problem in a name collision checker in plone.base, fixed in plone/plone.base#107 and a plone.protect warning issued by a write-on-read by setting the `__replaceable__` attribute on the `index_html` object. Fixes: #4279
thet
force-pushed
the
thet/4279-index-html
branch
from
ebe7f8a to
ba5ba5e
Compare
|
GHA tests fail because we need the source checkout from plone/plone.base#107 |
|
@mauritsvanrees Could we have a new release of plone.base please? |
|
I have released |
5 participants
index_htmlare often used to create default pages in containers. On the portal root this was prevented by a problem in a name collision checker in plone.base, fixed in plone/plone.base#107 and a plone.protect warning issued by a write-on-read by setting the__replaceable__attribute on theindex_htmlobject.Fixes: #4279
NOTE: I think the
__replaceable__attribute can be dropped when the index_html returns a content object. As I understood it, a replaceable attribute marks a path as an virtual alias or something, which can be just deleted. It is also barely used.If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.
Closes #4279