search

plone / plone.app.relationfield

Plone support for z3c.relationfield
https://pypi.org/project/plone.app.relationfield
3 stars 8 forks source link

Monkey patch incompatible with z3c.relationfield 1.1 #45

Open mauritsvanrees opened 11 months ago

mauritsvanrees commented 11 months ago

We have a monkey patch that gives wrong results with latest z3c.relationfield, containing this PR by @ksuess.

Let me copy my comment from a buildout.coredev PR where I try to update z3c.relationfield from 1.0 to 1.1. This breaks seven tests. All have the same source, a monkey patch from plone.app.relationfield. So maybe this monkey patch needs to be removed or adapted. Sample error from the plone.app.relationfield tests themselves:

  File "/srv/python3.11/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/srv/python3.11/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/srv/python3.11/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp311/plone.app.relationfield-3.0.3-py3.11.egg/plone/app/relationfield/tests/test_widget.py", line 52, in test_datamanager_set_nonempty
    self.assertEqual(dm.get(), self.person.addresses)
                     ^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp311/plone.app.relationfield-3.0.3-py3.11.egg/plone/app/relationfield/widget.py", line 164, in get
    if rel.isBroken():
       ^^^^^^^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp311/z3c.relationfield-1.1-py3.11.egg/z3c/relationfield/relation.py", line 105, in isBroken
    return self.to_id is None or self.from_object is None
                                 ^^^^^^^^^^^^^^^^
  File "/home/jenkins/.buildout/eggs/cp311/plone.app.relationfield-3.0.3-py3.11.egg/plone/app/relationfield/monkey.py", line 18, in get_from_object
    self._from_id = intids.register(self.__dict__["from_object"])
                                    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^
ksuess commented 11 months ago

"test_widget" of plone.app.relationfield fails as this is a low level test where relations are not fully postinitialized as they are for field parents that are subscribed to IIntIdAddedEvent and zope.lifecycleevent.IObjectModifiedEvent. Instead the test is just on the widgets data manager. Which is fine, but breaks if the z3c.relationfield RelationValue.isbroken checks not only RelationValue.to_id but also RelationValue.from_object. Checking RelationValue.from_object to decide if a relation is broken made sense to me from a higher level perspective. I proposed the change in z3c.relationfield while fixing the inspection of relations which produced false sets of broken and valid relations. But now with the failing test I think it is up to the (Plone) inspection code (plone.restapi and for Classic CMFPlone) and not up to the z3c.relationfield code, which should stay innocent about the relation source. I will create a reverting PR for z3c.relationfield .

ksuess commented 11 months ago

explanation of the patch by @do3cc in https://github.com/plone/plone.app.relationfield/commit/a5b69d8e1ad66a7d3d71ad772a62346761371742#r14620521

ksuess commented 11 months ago

PR https://github.com/zopefoundation/z3c.relationfield/pull/25 to revert the change on RelationValue.isBroken

do3cc commented 11 months ago

Looking at my old patch I see two mistakes:

  1. I should not have referenced the bug ticket by mere number, but by a probably today non existing URL.
  2. My monkey patch is incomplete. After applying the patch, newly created RelationValue Objects really don't have a key "from_object" in their dict. I should catch that case and return None, so that the original behaviour is not changed.

But I don't feel comfortable creating PRs for code I don't use at all any more. I think @ksuess Code should work and my old monkey patch should return None instead of a KeyError if the dict does not contain a key "from_object"