Skip to content

Remove object link if user removes a related object in the objectrelationlist datatype attribute.#31

Closed
pkamps wants to merge 2 commits intomasterfrom
clear_relation_after_object_removal2
Closed

Remove object link if user removes a related object in the objectrelationlist datatype attribute.#31
pkamps wants to merge 2 commits intomasterfrom
clear_relation_after_object_removal2

Conversation

@pkamps
Copy link
Member

@pkamps pkamps commented Nov 28, 2016

== TESTING INSTRUCTIONS ==

  1. Create a node that has a objectrelationslist attribute.
  2. Add an object relation in that attribute
  3. Publish the node
  4. Confirm that the relation is listed on the relation tab
  5. Edit the newly created node
  6. Remove the object relation you previously created
  7. Publish the node
  8. Confirm that the relation is not listed on the relation tab

Without this pull request, you would still see the object relation listed in the relation tab.

@peterkeung
Copy link
Member

peterkeung commented Nov 28, 2016

Would the effect of this bug be that "related_objects" fetches return bad data? I'm surprised that this would have been an issue all these years.

I cannot reproduce this problem (without the patch) on a 5.4 install (Mugo.ca staging). To test, I edited a blog post and removed relations in both the Categories (which is a list of checkboxes) and Related Blog Posts (which is the default "browse mode"). The listing in the Relations tab was properly updated both times.

Is this potentially a problem only in specific types of object relation list displays? Or with a specific template override?

@dougplant
Copy link
Member

dougplant commented Nov 28, 2016 via email

@pkamps
Copy link
Member Author

pkamps commented Nov 28, 2016

I think following pull request broke that functionality:
ezsystems@b45c9d6

on mugo.ca stage I applied the patch and I can now reproduce the issue.

In any case, it probably needs to be fixed differently - so this pull request is probably invalid.

@pkamps
Copy link
Member Author

pkamps commented Nov 29, 2016

More details:

The commit from my previous comment was supposed to fix:
https://jira.ez.no/browse/EZP-24530
(pull request: ezsystems#1183)

Issue summary: Looks like the relation tab in the admin UI is only showing the object relations of the most current version - in a multi-language setup it is not showing all object relations.

So basically we see a regression issue due to that fix in 2015.

I think we can come up with a different fix, allowing us to still remove object relations which then get properly removed in ezcontentobject_link.

@pkamps
Copy link
Member Author

pkamps commented Nov 29, 2016

== IMPLEMENTATION DETAILS ==
The first version of this patch was just removing an object relation in context of the custom action: A user clicked the 'Remove relation' button. That's not going to work because another translation might have the same object relation - we would remove it then from the relation list.

My new approach is different. When an objectrelationlist attribute gets stored, it will update the entire list of object relations. Then it checks the attribute in all translation and builds a distinct list of object relations. So in case of storing a draft it will check all translation of the published version but for the current draft language, it takes the attribute value from the draft version.

Please note:

  • relations are in context of object versions but not in context of translations (so fetching object relations will return all relations for all translations)
  • removing the entire object relation list and the rebuilding it is not performant for very long lists of object relations - but I'm not sure if it would be a lot faster if I compare 2 lists in PHP and only submit the differences to the DB. Also, that approach was always in the code - I just fixed some
    edge cases.
  • the objectrelation attribute is most likely affected as well. But I already spend too much time on this. So it has to be another pull request (some other day).
  • I also tested the use case of setting the attribute to non-translatable, still works correctly with this patch

@peterkeung
Copy link
Member

This should be submitted to the eZ repo as well

@peterkeung
Copy link
Member

I've created an enterprise support ticket with eZ so they can review this issue.

@peterkeung
Copy link
Member

Response from eZ:

I tested the issue you reported on eZ Publish 5.4, fully updated to 5.4.9, but was not able to reproduce the bug - after removing the relation and publishing (steps #6 and #7), the relation is no longer listed on the "Relations" tab, as expected.

I checked the commit you mention, ezsystems@b45c9d6, and then checked the commit history for the file in question, /ezpublish_legacy/kernel/classes/datatypes/ezobjectrelationlist/ezobjectrelationlisttype.php. I found that there have been a few commits after the one you refer to, related to these public issues, and in this order:

  • [EZP-25065: non translatable object relations not updated when editing content with multiple translations|https://jira.ez.no/browse/EZP-25065], fix released in 5.4.6;
  • [EZP-25396: Obsolete object relations not cleaned up|https://jira.ez.no/browse/EZP-25396], fix released in 5.4.7;
  • [EZP-25848: PostgreSQL: query error in ezobjectrelation/list publish|https://jira.ez.no/browse/EZP-25848], fix also released on 5.4.7.

Sounds like it was probably fixed in https://jira.ez.no/browse/EZP-25396, although there is no commit there.

@pkamps
Copy link
Member Author

pkamps commented Jan 11, 2017

I can easily reproduce the issue with what there is in the master branch. If I remember correctly,

CSM is affected (fairly old ezp version)
mugo.ca is not affect (newer version)
very recent versions are affected again

I'm happy to show you the issue on a screenshare.

@pkamps
Copy link
Member Author

pkamps commented Jan 19, 2017

Closed in favour of:
#36

@pkamps pkamps closed this Jan 19, 2017
@pkamps pkamps deleted the clear_relation_after_object_removal2 branch May 11, 2017 06:47
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

Successfully merging this pull request may close these issues.

3 participants