Update to Zope 4.0b7

[pbauer]

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:

• Validation of PythonIdentifier Startup of Plone fails in Zope 4.0b6 on py3 #2591
• Changed __repr__ for Zope objects Track down all implications of the __repr__ changes of Zope objects #2547
• Update the versions to https://raw.githubusercontent.com/zopefoundation/Zope/4.0b7/versions.cfg

[icemac]

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

[thet]

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

[pbauer]

With plone/buildout.coredev#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:

• https://jenkins.plone.org/job/pull-request-5.2-3.7/7/
• https://jenkins.plone.org/job/pull-request-5.2-3.6/7/
• https://jenkins.plone.org/job/pull-request-5.2/1586/

[pbauer]

All failing tests are all caused to the change of __repr__. Once zopefoundation/Zope#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]

I did a new testrun that includes the changes in zopefoundation/persistent#98:

• https://jenkins.plone.org/job/pull-request-5.2-3.7/12/
• https://jenkins.plone.org/job/pull-request-5.2-3.6/12/
• https://jenkins.plone.org/job/pull-request-5.2/1591/

[pbauer]

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

• https://jenkins.plone.org/job/pull-request-5.2-3.7/21/
• https://jenkins.plone.org/job/pull-request-5.2-3.6/19/
• https://jenkins.plone.org/job/pull-request-5.2/1602/

One failure is already adressed in zopefoundation/Products.GenericSetup#75

[icemac]

• 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)

[pbauer]

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?

[icemac]

persistent.Persistent is implemented in C. This fact seems to change the MRO so it always comes before the classes implemented in Python. See pylint-dev/pylint#2188 where I got to know about this fact.

[pbauer]

🤦‍♂️

[pbauer]

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.

[jamadden]

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.

[pbauer]

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

• Restore old __repr__ plone.dexterity#93
• restore original __repr__ Products.Archetypes#121
• Restore orignal __repr__  plone.app.folder#22
• Fix tests in 5.2 plone.contentrules#9

When the following builds of plone/buildout.coredev#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.

• https://jenkins.plone.org/job/pull-request-5.2/1615/
• https://jenkins.plone.org/job/pull-request-5.2-3.6/30/
• https://jenkins.plone.org/job/pull-request-5.2-3.7/32/

[jamadden]

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.

[pbauer]

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

[pbauer]

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]

@jamadden there are surprising test-errors in zopefoundation/Zope#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]

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]

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 zopefoundation/Zope@b52bdd9 to work around that.

[pbauer]

My fix in zopefoundation/Zope#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 zopefoundation/Zope#392 and instead add the additional base-classes as prepared in

• Restore old __repr__ plone.dexterity#93
• restore original __repr__ Products.Archetypes#121
• Restore orignal __repr__  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]

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]

All merged. The issue with _p_repr is continued in zopefoundation/persistent#101

[jimfulton]

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__.