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.

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

Monkey patch incompatible with z3c.relationfield 1.1 #45

Open
mauritsvanrees opened this issue Oct 25, 2023 · 4 comments
Open

Monkey patch incompatible with z3c.relationfield 1.1 #45

mauritsvanrees opened this issue Oct 25, 2023 · 4 comments

Comments

@mauritsvanrees
Copy link
Member

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
Copy link
Member

ksuess commented Oct 25, 2023

"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
Copy link
Member

ksuess commented Oct 26, 2023

explanation of the patch by @do3cc in a5b69d8#r14620521

@ksuess
Copy link
Member

ksuess commented Oct 26, 2023

PR zopefoundation/z3c.relationfield#25 to revert the change on RelationValue.isBroken

@do3cc
Copy link
Member

do3cc commented Oct 27, 2023

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"

mauritsvanrees added a commit to plone/plone.app.multilingual that referenced this issue Mar 11, 2024
Without this, the tests fail because we get latest z3c.relationfield 1.1, even though latest Plone 6.1 pins 1.0.

Sample test failure:

```
Error in test test_relation_list_gets_translated (plone.app.multilingual.tests.test_lif.TestLanguageIndependentRelationField.test_relation_list_gets_translated)
Traceback (most recent call last):
  File "/Users/maurits/.pyenv/versions/3.11.7/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/Users/maurits/.pyenv/versions/3.11.7/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/Users/maurits/.pyenv/versions/3.11.7/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/tests/test_lif.py", line 222, in test_relation_list_gets_translated
    a_ca = api.translate(self.a_en, "ca")
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/api.py", line 46, in translate
    manager.add_translation(target_language)
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/manager.py", line 135, in add_translation
    translated_object = translation_factory(language)
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/factory.py", line 99, in __call__
    cloner(new_content)
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/dx/cloner.py", line 26, in __call__
    ILanguageIndependentFieldsManager(self.context).copy_fields(obj)
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/dx/cloner.py", line 78, in copy_fields
    copied_relation = self.copy_relation(
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/src/plone/app/multilingual/dx/cloner.py", line 42, in copy_relation
    if not relation_value or relation_value.isBroken():
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/.tox/test/lib/python3.11/site-packages/z3c/relationfield/relation.py", line 105, in isBroken
    return self.to_id is None or self.from_object is None
  File "/Users/maurits/community/plone-coredev/6.1/src/plone.app.multilingual/.tox/test/lib/python3.11/site-packages/plone/app/relationfield/monkey.py", line 18, in get_from_object
    self._from_id = intids.register(self.__dict__["from_object"])
KeyError: 'from_object'
```

For that KeyError, see plone/plone.app.relationfield#45.

But the issue that the current commit fixes, is that we should be using the proper constraints, instead of getting the latest from PyPI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants