Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows versioning of the Plone Site type for the portal working copy to work #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Allows versioning of the Plone Site type for the portal working copy to work #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5faa043
Allows versioning of the "Plone Site" type
wesleybl Oct 2, 2024
9753c81
Allows the "Plone Site" type to be cloned
wesleybl Oct 2, 2024
de7f30c
Add changes
wesleybl Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Products/CMFEditions/StandardModifiers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

from AccessControl.class_init import InitializeClass
from Acquisition import aq_base
from Acquisition import ImplicitAcquisitionWrapper
from OFS.ObjectManager import ObjectManager
from plone.folder.default import DefaultOrdering
from Products.BTreeFolder2.BTreeFolder2 import BTreeFolder2Base
Expand Down Expand Up @@ -673,6 +674,15 @@ def getOnCloneModifiers(self, obj):
parent_id = id(aq_base(parent))

def persistent_id(obj):
# Avoid: TypeError: Can't pickle objects in acquisition wrappers.
if isinstance(obj, ImplicitAcquisitionWrapper):
return True
# Allows Plone Site to be serialized with pickle.
if (
hasattr(aq_base(obj), "portal_type")
and aq_base(obj).portal_type == "Plone Site"
):
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the ImplicitAcquisitionWrapper probably comes from a child object, not the parent pointer. And I expect #117 will solve this so that we don't need this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davisagli after the merge of #117, I rebased it with the master branch here. After that, the code:

  if isinstance(obj, ImplicitAcquisitionWrapper):
      return True

is no longer needed. Thanks!

But the code:

  if (
      hasattr(aq_base(obj), "portal_type")
      and aq_base(obj).portal_type == "Plone Site"
  ):
      return

is still needed. Without it, I get the error below when trying to checkout in Volto:

2024-12-17 18:20:39,118 ERROR   [Zope.SiteErrorLog:36][waitress-2] AttributeError: http://localhost:3000/@workingcopy
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 181, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 390, in publish_module
  Module ZPublisher.WSGIPublisher, line 284, in publish
  Module ZPublisher.mapply, line 98, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module plone.rest.service, line 21, in __call__
  Module plone.restapi.services, line 19, in render
  Module plone.restapi.services.workingcopy.create, line 44, in reply
  Module plone.app.iterate.base, line 64, in checkout
  Module zope.event, line 33, in notify
  Module zope.component.event, line 27, in dispatch
  Module zope.component._api, line 146, in subscribers
  Module zope.interface.registry, line 458, in subscribers
  Module zope.interface.adapter, line 918, in subscribers
  Module plone.app.iterate.subscribers.versioning, line 28, in handleBeforeCheckout
  Module plone.app.iterate.archiver, line 36, in save
  Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 300, in save
  Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 455, in _recursiveSave
  Module Products.CMFEditions.ArchivistTool, line 268, in prepare
  Module Products.CMFEditions.ModifierRegistryTool, line 217, in beforeSaveModifier
  Module Products.CMFEditions.StandardModifiers, line 392, in beforeSaveModifier
  Module Products.CMFEditions.StandardModifiers, line 358, in _beforeSaveModifier
AttributeError: 'NoneType' object has no attribute 'objectIds'

The strange thing is that in Plone Classic the checkout works without it. So, it's something related to plone.restapi.

Copy link
Member Author

@wesleybl wesleybl Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually both ifs are needed yet. Without the first if, checkout works but checkin gives an error:

024-12-17 18:54:16,453 ERROR   [Zope.SiteErrorLog:36][waitress-2] TypeError: http://localhost:3000/working_copy_of_plone/@workingcopy
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 181, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 390, in publish_module
  Module ZPublisher.WSGIPublisher, line 284, in publish
  Module ZPublisher.mapply, line 98, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module plone.rest.service, line 21, in __call__
  Module plone.restapi.services, line 19, in render
  Module plone.restapi.services.workingcopy.update, line 30, in reply
  Module plone.app.iterate.dexterity.policy, line 36, in checkin
  Module zope.event, line 33, in notify
  Module zope.component.event, line 27, in dispatch
  Module zope.component._api, line 146, in subscribers
  Module zope.interface.registry, line 458, in subscribers
  Module zope.interface.adapter, line 918, in subscribers
  Module plone.app.iterate.subscribers.versioning, line 33, in handleAfterCheckin
  Module plone.app.iterate.archiver, line 36, in save
  Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 300, in save
  Module Products.CMFEditions.CopyModifyMergeRepositoryTool, line 455, in _recursiveSave
  Module Products.CMFEditions.ArchivistTool, line 267, in prepare
  Module Products.CMFEditions.ArchivistTool, line 223, in _cloneByPickle
TypeError: Can't pickle objects in acquisition wrappers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if ( hasattr(aq_base(obj), "portal_type") and aq_base(obj).portal_type == "Plone Site" ):
    return

prevents including references to the portal when an object is serialized while cloning. I need to understand better where that reference is in order to see if we want to remove it or restore it, but I haven't been successful at debugging this yet.

Also if we do need special logic to handle this, this isn't the right place for it, because it's not (I think) about parent pointers.

Copy link
Member

@davisagli davisagli Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding my notes since I need to stop for the day:

  • the problem seems to be with the _components attribute of the portal (the site manager)
  • it's not always a problem... if I restart the instance in between creating the working copy and checking it in, then it succeeds
  • I suspect we need a new modifier to prevent including the site manager when storing or retrieving versions of the portal (it's quite surprising/broken if you restore an old version and it also resets the site manager to an old set of components!)
  • but I think this is somewhat orthogonal to working copy support. plone.app.iterate currently always tries to create a new revision after a checkin, even if the content type doesn't have versioning enabled. That's silly! We should fix https://github.com/plone/plone.app.iterate/blob/master/plone/app/iterate/subscribers/versioning.py#L31 so that it only calls archiver.save if archiver.isVersionable() is true. Then I think we won't need to enable versioning for the Plone Site type in order to support working copies.
  • There's a separate bug in the SkipParentPointers modifier. It needs to call aq_inner(obj) before getting the obj's parent, to make sure it's not an object aq-wrapped in itself! This is what led to the AttributeError: 'NoneType' object has no attribute 'objectIds' error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think this is somewhat orthogonal to working copy support. plone.app.iterate currently always tries to create a new revision after a checkin, even if the content type doesn't have versioning enabled. That's silly! We should fix https://github.com/plone/plone.app.iterate/blob/master/plone/app/iterate/subscribers/versioning.py#L31 so that it only calls archiver.save if archiver.isVersionable() is true. Then I think we won't need to enable versioning for the Plone Site type in order to support working copies.

@davisagli I'm not remembering exactly now but I think I've come to the conclusion that archiver.isVersionable() will be true only if versioning is enabled for the content type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem seems to be with the _components attribute of the portal (the site manager)

Plone Site has several attributes of type ImplicitAcquisitionWrapper. Not only _components.

if id(aq_base(obj)) == parent_id:
return True
return None
Expand Down
4 changes: 4 additions & 0 deletions Products/CMFEditions/profiles/default/repositorytool.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,9 @@
<policy name="at_edit_autoversion" />
<policy name="version_on_revert" />
</type>
<type name="Plone Site">
<policy name="at_edit_autoversion" />
<policy name="version_on_revert" />
</type>
Comment on lines +29 to +32
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether I should do an upgrade step for this or just leave it for new portals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave it just for new portals, but we should mention that in the release note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the text Allows versioning of the Plone Site type for the portal working copy to work. is this not clear? Do you have any other suggestions?

</policymap>
</repositorytool>
24 changes: 24 additions & 0 deletions Products/CMFEditions/tests/test_ContentTypes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,30 @@ def testNewsItem(self):
self.assertEqual(content.text.raw, "text v1")
self.metadata_test_one(content)

def test_plone_site(self):
portal_repository = self.portal_repository
content = self.portal
content.title = "content"
content.subject = ["content"]
content.description = "content"
content.contributors = ["content"]
content.language = "content"
content.rights = "content"
portal_repository.applyVersionControl(content, comment="save no 1")
content.title = "contentOK"
content.subject = ["contentOK"]
content.description = "contentOK"
content.contributors = ["contentOK"]
content.language = "contentOK"
content.rights = "contentOK"
portal_repository.save(content, comment="save no 2")
obj = portal_repository.retrieve(content, 0).object
self.metadata_test_one(obj)
obj = portal_repository.retrieve(content, 1).object
self.metadata_test_two(obj)
portal_repository.revert(content, 0)
self.metadata_test_one(content)

def testImage(self):
self.folder.invokeFactory("Image", id="image")
portal_repository = self.portal_repository
Expand Down
1 change: 1 addition & 0 deletions news/113.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allows versioning of the Plone Site type for the portal working copy to work. @wesleybl
Loading