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

[wesleybl]

This is part of: plone/volto#5284

For the working copy of plone.app.iterate to work on the Plone Site type, it is necessary that this type is versionable. See:

https://github.com/plone/plone.app.iterate/blob/1908aa1f4e9c95c37143fae88655469b72ea451a/plone/app/iterate/browser/control.py#L88-L90

This also allows the "Plone Site" type to be cloned.

Products.CMFEditions uses the pickle module to clone objects. See:

Products.CMFEditions/Products/CMFEditions/ArchivistTool.py

Lines 210 to 229 in c80bc31

	def _cloneByPickle(self, obj):
	"""Returns a deep copy of a ZODB object, loading ghosts as needed."""
	modifier = getToolByName(self, "portal_modifier")
	callbacks = modifier.getOnCloneModifiers(obj)
	if callbacks is not None:
	pers_id, pers_load, inside_orefs, outside_orefs = callbacks[0:4]
	else:
	inside_orefs, outside_orefs = (), ()
	stream = BytesIO()
	p = Pickler(stream, 1)
	if callbacks is not None:
	p.persistent_id = pers_id
	p.dump(aq_base(obj))
	approxSize = stream.tell()
	stream.seek(0)
	u = Unpickler(stream)
	if callbacks is not None:
	u.persistent_load = pers_load
	return approxSize, u.load(), inside_orefs, outside_orefs

These changes prevent the error:

TypeError: Can't pickle objects in acquisition wrappers

When trying to serialize an ImplicitAcquisitionWrapper object with the pickle dump. This error occurred when trying to check in a Plone Site type with plone.app.iterate.

These changes also allow the Plone Site type to be serialized with the pickle. When the persistent_id method of SkipParentPointers returns something other than None, the object is not serialized. So we need to make sure it returns None for the Plone Site type.

[mister-roboto]

@wesleybl thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

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!

[wesleybl]

@jenkins-plone-org please run jobs

[wesleybl]

I ran the tests for this PR along with plone/plone.app.iterate#128

See: Plone Jenkins CI - pull-request-6.0-3.11 and Plone Jenkins CI - pull-request-6.0-3.8

[wesleybl]

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

[davisagli]

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

[wesleybl]

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?

[davisagli]

@wesleybl Sorry for my long delay on reviewing this...

[davisagli]

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.

[wesleybl]

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

[wesleybl]

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.

[davisagli]

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.

[davisagli]

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

[wesleybl]

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.

[wesleybl]

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.

[davisagli]

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