Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When a Paragraph / Entity Reference Revisions field is present, it is always shown as changed. Even when there was no update on the paragraphs...
This makes the idea of a changed summary unusable for a Paragraphs based use case.
Proposed resolution
Investigate the use case, Support ERR / Composite recursion if needed.
This might ask for the same hierarchical display of labels such as the diff itself does.
But in short, make sure we have a meaningful message.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#32 | diff_2806119_always_32.patch | 2.27 KB | miro_dietiker |
#30 | paragraph_always-2806119-30.patch | 3.61 KB | johnchque |
#26 | interdiff-2806119-21-26.txt | 2.42 KB | johnchque |
#26 | paragraph_always-2806119-26.patch | 5.35 KB | johnchque |
#21 | Screenshot from 2016-10-06 10-57-25.png | 90.54 KB | johnchque |
Comments
Comment #2
miro_dietikerComment #3
johnchqueYes, debugging a bit realized that that happens because when a new revision is created of the host entity, the paragraphs also create a new revision so the reference is updated and the paragraphs field appear in the revision summary.
Comment #4
johnchqueBeing trying this but since the revision reference is updated and so, we would need to load the entity that is referenced and also the revision that was referenced to the previous host entity. Wouldn't the performance be affected with this? it would be like building the whole array for comparing in the revision listing and with all the revisions doing the same. Not really sure.
Comment #5
miro_dietikerIn the core issue about improving the revision logs, discussion is to autogenerate some message and persist it. I added that we should know if it was autogenerated or not.
#1807908: Autofill revision log with text that describes action like: German translation added or Spanish translation edited
So a quickfix would be to skip these kinds of ERR fields, but that would also lie as the summary would not contain relevant information if some Paragraph really changed....
My conclusion: If you want to work with Paragraphs, the summary doesn't do the job, even doesn't add value, could be even more confusion.
So i guess, we need to traverse into the composite entities and load them considering the specific revision.
And yes loading the entities revision will lead to huge IO, especially since the entity cache now doesn't work.
To keep complexity limited and summary short, we should only traverse into a nested child if the parent didn't change.
So beside specific fields that changed, we could add a better message for "Added Paragraph" or "Deleted Paragraph" and not list all their individual fields. But again, boosting complexity. ;-)
Thinking about usefulness, i think ALL or nothing. And if ALL is a performance problem, we need to allow disabling it as a setting.
We should still do all this, because the next step is to hook into entity presave and persist the autogenerated value to get rid off that load. And we want to learn from improving autogenerated messages. This is possibly the most important UX area to iterate on. The core proposal shows that experience in this area is lacking and usefulness without it is very limited. Lots of follow-ups. Many can be post-release.
Comment #6
johnchqueEarly patch. Tried for long time. Patch loops over the fields inside the entity. Right now broken cause sometimes we can add a Paragraph in a revision so the summary is not generated correctly. Need to investigate more.
Comment #8
johnchqueActually this makes it more readable.
Comment #10
johnchqueLet's see now. :) Interdiff added.
Comment #11
johnchqueAdded screenshot.
When a new paragraph is added the name of the field of the host entity is displayed, when the fields inside a Paragraph are updated the name of the fields in the Paragraphs are shown.
Comment #12
miro_dietikerThx, great to see the screenshot.
If two nested paragraphs use the same field, this creates a clash.
We either need to use key chaning (a summary parent key argument) or simply numeric keys that are appended.
As discussed directly, we need some inline comments explaining the specific (pretty complex) cases.
And finally, we will need test coverage.
Comment #13
johnchqueActually this can be much cleaner, I'm trying something now. Will upload a patch soon.
Comment #14
johnchqueWorked on this, but this makes me realize that loading the entity that a node was pointing and load the revision of the pointed revision and then comparing might be harder than expected.
Comment #15
miro_dietikerWe need a decision, preferrably a solution, for the release.
Comment #16
johnchqueOK, discussed with Miro, we will use the EntityParser code to make it recursive and nice for the user. :)
Comment #17
johnchqueStarted from 0. Now we have a better display of the changed fields.
Comment #19
johnchqueI was wondering if the initial revision should have a comment like "Initial revision." instead of "No changes." That would be much nicer IMO.
Comment #20
johnchqueUps, just realized there are some small problems when an entity is present in one revision and not in the other one, still needs work.
Comment #21
johnchqueThis should work nicer. And also test fixed.
Looks like this now:
Comment #22
johnchqueEDIT: Wrong comment.
Comment #23
johnchqueComment #25
tduong CreditAttribution: tduong at MD Systems GmbH commentedSmall small re-considerations:
I would say this
// Check if the revision has a log message.
and
// Auto generate a revision log.
above the
if ($auto_generated)
Then would add a new line.
Not sure what you meant, but maybe
// If there is a previous revision, load its values and add the changed
// ones to the summary.
// Unset the field, which value is not changed. Add it to the summary
// if it is changed or new.
And I would shorten this check as
// Add the remaining left values not present in the right entity..
And separate this foreach-block from the one above.
[]
Comment #26
johnchqueComments changed.
5. of the comment above cannot be done, that would mean change in the logic, after comparing a field no matter it has changed or not, if present in the right side, I unset it, so just remaining fields are added when looping on the left values.
Comment #27
johnchqueThis might change when #2810039: Diff for 'Sticky at top of lists' does not work is committed. Will need to check again after that one is committed.
Comment #29
miro_dietikerThat's a strange mix of array and string. We try to be uniform.
I still committed to help move forward, but i think we should improve it a bit.
Comment #30
johnchqueThis should be nicer and more readable. :)
Comment #31
miro_dietikerI then reviewed that terminology of variables (left, right, previous) and confirmed that this naming is fine.
left + right is with two references selected (could be a sequence).
But this method has one revision, plus optionally the previous one. left + right is very confusing especially if left is getting optional and sequence changes with right first.
This is double reset.
Comment #32
miro_dietikerReverting the renames.
Comment #33
johnchqueMuch better IMHO.
Comment #35
miro_dietikerCommitted. :-)