Products.cmfplone: Update to Zope 4.0b7

Created on 11 Oct 2018 · 23Comments · Source: plone/Products.CMFPlone

Zope 4.0b6 is out (https://community.plone.org/t/zope-4-0b6-released/7345/1). Updating the extends to the new version-pinns fails in multiple ways (in 5.2 coredev but also in py3 and py3_on_py2).

Here we collect issues that need to be adressed/fixed:

[x] Validation of PythonIdentifier #2591
[x] Changed __repr__ for Zope objects #2547
[x] Update the versions to https://raw.githubusercontent.com/zopefoundation/Zope/4.0b7/versions.cfg

high

Source

pbauer

Most helpful comment

There will be a 4.0b7 in the near feature containing some bug fixes for 4.0b6.

icemac on 16 Oct 2018

👍2

All 23 comments

There will be a 4.0b7 in the near feature containing some bug fixes for 4.0b6.

icemac on 16 Oct 2018

👍2

For #2547 there might be a simple solution which is discussed in the ticket.

thet on 16 Oct 2018

With https://github.com/plone/buildout.coredev/pull/537 I combined the update to 4.0b6 with the fixed ZODB and fixes for #2591. The jenkins-jobs results will show all cases where __repr__ breaks the tests:

pbauer on 18 Oct 2018

All failing tests are all caused to the change of __repr__. Once https://github.com/zopefoundation/Zope/issues/379 is done we can safely update to 4.0b6 without touching all the failing doctests:

Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope2_page_templates_good_format_attr_str
Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope2_page_templates_good_format_attr_unicode
Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope2_page_templates_good_str
Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope2_page_templates_good_unicode
Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope3_page_templates_normal
Products.CMFPlone.tests.test_safe_formatter.TestSafeFormatter.test_cook_zope3_page_templates_using_format
Products.ATContentTypes.tests.events.txt
Products.Archetypes.tests.events.txt
Products.CMFPlacefulWorkflow.tests.exportimport.txt
Products.CMFQuickInstallerTool.tests.actions.txt
Products.CMFQuickInstallerTool.tests.install.txt
Products.CMFQuickInstallerTool.tests.profiles.txt
five.intid.README.rst
plone.app.contentlisting.tests.integration.rst
plone.app.versioningbehavior.tests.doctest_behavior.txt
plone.app.viewletmanager.tests.storage.rst
plone.registry.field.rst
plone.registry.registry.rst
src.plone.app.blob.README.txt
src.plone.app.imaging.tests.traversal.txt
src.plone.app.testing.layers.rst
plone.app.contentrules.tests.test_action_logger.TestLoggerAction.testProcessedMessage

pbauer on 18 Oct 2018

I did a new testrun that includes the changes in https://github.com/zopefoundation/persistent/pull/98:

pbauer on 20 Oct 2018

I did a new testrun with the newest versions from https://raw.githubusercontent.com/zopefoundation/Zope/master/versions.cfg and the branch https://github.com/zopefoundation/Zope/pull/387 which restores the old __repr__ in OFS. There may be exciting new failures.

One failure is already adressed in https://github.com/zopefoundation/Products.GenericSetup/pull/75

pbauer on 24 Oct 2018

Products.CMFQuickInstallerTool.tests.actions.txt: CMFQuickInstallerTool seems to need the new OFS.SimpleItem.PathReprProvider as first base class, so its __repr__ is in the MRO before persistent.Persistent.
plone.app.contentlisting.tests.integration.rst: Same here for plone.app.contenttypes.content.Folder
plone.app.versioningbehavior.tests.doctest_behavior.txt: Same here for plone.dexterity.content.Container.
plone.contentrules.README.rst: <builtins.MoveToFolderAction object at is current for persistent >= 4.4.3.
Products.Archetypes.tests.translate.txt is broken because of the changes in zope.i18n 4.6 (aka plurals support)

icemac on 25 Oct 2018

I don't understand where the __repr__ of persistent.Persistent takes precedence over the one from OFS.SimpleItem. Here is the chain as I understand it:

plone.app.contenttypes.content.Folder(Container)
plone.dexterity.content.Container(OFSContainer, CMFCatalogAware, PortalFolderBase, PortalContent, DefaultDublinCoreImpl, Contained)
Products.CMFCore.PortalFolder.PortalFolderBase(DynamicType, OpaqueItemManager, Folder)
OFS.Folder.Folder(PathReprProvider,...)
OFS.SimpleItem.PathReprProvider

Can you give me a hint?

pbauer on 25 Oct 2018

persistent.Persistent is implemented in C. This fact seems to change the MRO so it always comes before the classes implemented in Python. See https://github.com/PyCQA/pylint/issues/2188 where I got to know about this fact.

icemac on 25 Oct 2018

🤦‍♂️

pbauer on 25 Oct 2018

I see that changing the first base-class of plone.dexterity.content.Container to PathReprProvider fixes the reprs for folderish dexterity content. It feels super-ugly though. Having a completely different repr for Item and Container is a no-go though, so I'll probably add that.

Are we planning to add the physical path like in PathReprProvider to the __repr__ of persistent.Persistent at some point? I can see value in having the oid and connection-info on top the full-python path and the physical path.

Also: That base-classes implemented in C ruin the MRO is just very terrible.

pbauer on 25 Oct 2018

Are we planning to add the physical path like in PathReprProvider to the __repr__ of persistent.Persistent at some point?

"path" is not a concept that Persistent has.

jamadden on 25 Oct 2018

👍1

With the following four PR's all remaining issues should be fixed:

When the following builds of https://github.com/plone/buildout.coredev/pull/542 are green and Zope 4.0b7 is released we can merge the PRs, update the versions-link and start a release of 5.2a1.

pbauer on 26 Oct 2018

Because Persistent's __repr__ first tries to delegate to a _p_repr, if present, I wonder if this could be solved without manually changing all those other classes' MRO by having PathReprProvider also set _p_repr (_p_repr = __repr__). That way, when the weird C MRO comes into play and finds Persistent's __repr__, Persistent can still find a correct _p_repr.

jamadden on 26 Oct 2018

@jamadden Thanks for the tip, I'll test that.

pbauer on 27 Oct 2018

Looks good. I'll wait for comments. Not requiring to add a new base-class that is only available in the newest Zope (even to deprecated Code such as Archetypes) would be much better.

pbauer on 27 Oct 2018

@jamadden there are surprising test-errors in https://github.com/zopefoundation/Zope/pull/392:

File "/home/travis/build/zopefoundation/Zope/src/Products/Five/browser/tests/pages.txt", line 291, in pages.txt
Failed example:
    aq_parent(aq_inner(context))
Expected:
    <Folder at /test_folder_1_> 
Got:
    <Folder at test_folder_1_>

Could it be that the C-implementation of returning _p_repr swallows the leading slash? My C-fu is non-existent.

pbauer on 27 Oct 2018

Weird: The issue is actually in PathReprProvider.

When calling the same method as obj.__repr__() the output of '/'.join(self.getPhysicalPath()) is '/test_folder_1_' (correct).

But when calling the same method as as obj._p_repr() the output of the same same code '/'.join(self.getPhysicalPath()) is 'test_folder_1_' (without the leading /). I need to go watch Disney on Ice now but I'll continue investigating later today.

pbauer on 27 Oct 2018

This can be reproduced in the test test_cook_zope3_page_templates_using_format:

aq_parent(aq_inner(self)) returns None when using __repr__, but when using _p_repr is returns <Application at >. So in the first case self seems to be not properly aq-wrapped. I re-added PathReprProvider to OFS.Folder.Folder in https://github.com/zopefoundation/Zope/pull/392/commits/b52bdd9617d8ef300e11a723301fd01d257414dc to work around that.

pbauer on 27 Oct 2018

My fix in https://github.com/zopefoundation/Zope/pull/392 passes in Zope itself but fails in Plone in several tests (https://jenkins.plone.org/job/pull-request-5.2-3.6/34/testReport/) with similar issue as above:

(Pdb++) repr(self.folder)
'<Folder at f1>'

instead of

(Pdb++) repr(self.folder)
'<Folder at /plone/f1>'

What now happens is that __repr__ of persistent.Persistent delegates to _p_repr of OFS.SimpleItem which again calls __repr__ of OFS.SimpleItem. Somewhere in that mess the object loses the ability to access its parent with aq_parent. I did not really find where that happens. Unless someone else can find the issue here I suggest to drop https://github.com/zopefoundation/Zope/pull/392 and instead add the additional base-classes as prepared in

plone/plone.dexterity#93
plone/Products.Archetypes#121
plone/plone.app.folder#22

My main reason (except for not being able to fix it) would is that debugging this issue is not obvious and would probably trip others up who encounter related issues in the future.

pbauer on 28 Oct 2018

We'll not use _p_repr in Zope for now since I could not figure out where exactly acquisition got lost and how to fix that. Instead I'll add PathReprProvider to Dexterity and Archetypes (i.e. the pull-requests mentioned above).
Since the coredev builds are currently broken because of a dependency-bump in Zope and Zope 4.0b7 will be released tomorrow, I'll also let coredev depend on https://raw.githubusercontent.com/zopefoundation/Zope/master/versions.cfg for a day to fix the tests.

pbauer on 29 Oct 2018

👍1

All merged. The issue with _p_repr is continued in https://github.com/zopefoundation/persistent/issues/101

pbauer on 1 Nov 2018

Coming into this late because Philip asked me to look at it. :) I'm probably missing something...

I wonder if a better fix would have been to try to address why Persistent was showing up too early in the MRO. For example, by swapping the order of the base classes in LocalSiteManager.

Persistent is kinda like object in that it seems to want to be at the end of the MRO. Otherwise, maybe it shouldn't be overriding something so basic as __repr__.