Hi #marcoscano, thanks so much for this useful and very well done module.
We are using Entity_usage extensively for tracking entity_usage of entities (Media_entities …)
But in our specific case /real world we are using Revisions with Workbench Moderation (new Content Moderation in Drupal Core 8.2).
and testing the module we realized the actual dev of entity_usage tracking logic fails OnEntityUpdate, both for EntityReference and EntityEmbed cases.
The matter is that with Workbench/Content Moderation the last revision of an entity is not necessarily the default one (it might a temporary, unpublished one, but with vid higher of the published/original/default one).
I thus produced the attached patch that adds fixing working logics to the Revisions with moderation entity usage tracking,
without breaking the actual code.
In brief, the attached patch covers (fixes) the following use cases, just OnEntityUpdate,
both for entity reference and entity embed
(for sake of simplicity I just mention the entity_reference case, but all the following is same valid for entity_embed too)
- the published (default/original) host entity has no media_entity referenced:
• a new tmp revision is created (not default revision) with a new media_entity referenced: 1 increment of usage (it is actually the same too);
• the same tmp revision is updated still as un-published (not default revision), without any change to the same media_entity reference: no increment of usage should happen (it is actually incremented of 1, instead);
• the same tmp revision is updated as published entity (default revision): no increment of usage should happen, as the media_entity is already in count (it is actually incremented of 1, instead);
- the published (default/original) host entity has a media_entity referenced (lets say the previous one):
• a new tmp revision is created (not default revision) with the media_entity removed: no decrement of usage, as the media_entity is still referenced in the published/original/default revision (it is actually decremented of 1);
• a new tmp revision is created (not default revision) with the same media_entity referenced again: no increment of usage should happen, as the media_entity is already in count (it is actually incremented of 1, instead);
• the same tmp revision is updated to published entity (default revision), without the media_entity (un referenced): a decrement of usage should happen (it is actually decremented too, but the entity_usage might still be more than 1, due to the previous incorrect increments);
We tested the above scenarios also with multiple and concurrent entity references and embeds ... and it works (it seems).
We also tested that the more traditional scenarios with no revisions, and with un-moderated revisions: all still works the same, and they do, due to defensive conditionals on the actual existing code.
We wrote code Drupal standards compliant and added comments.
The attached patch needs review, further eventual amends / re-factoring and possible final inclusion in the module.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | entity_usage_revisions_support-2821411-9.patch | 12.31 KB | itamair |
| #2 | entity_usage_revisions_support-2821411-2.patch | 11.71 KB | itamair |
Comments
Comment #2
itamair commentedComment #7
marcoscanoThanks for testing the module and reporting feedback!
At the moment the module does not support out-of-the-box a revisions / content moderation workflow. I agree that it would be interesting to add this support at some point, though.
I couldn't get a chance to look at your patch yet, but my starting feeling about this is that we should take either one of these two approaches:
- Either it is possible to extensively support revisions (ie cover most workflow moderation use cases) in a generic way, in order not to introduce too much complexity to the tracking logic (not being too specific in the code)
- Or create a sub-module that would exclusively deal with revisions and moderation workflows, once although it is a very common scenario, not all sites will need support for revisions.
Did you get a chance to see why the automated tests are failing?
Comment #8
marcoscanoComment #9
itamair commented#marcoascano, I agree with you with the most, but consider that the use cases I mentioned should cover the typical revisions (with content moderation) cases,
so these patches/code shouldn't be considered as a specific fix to our use cases needs, but a more general solution.
In my previous patch I missed some defensive code to face the case a content entity hasn't any previous revision id, so this here attached new patch should solve the tests matters.
Comment #10
itamair commentedComment #11
itamair commentedComment #12
itamair commentedComment #13
itamair commentedPS: From this entity_usage code amends of mine, I am minding it would better to use ContentEntityInterface (instead/in place of EntityInterface), in the most of the classes (I would say in every),
to have the correct availability of methods like get, getFieldDefinition, getFieldDefinitions that are defined only on ContentEntityInterface -> FieldableEntityInterface (my IDE coding standards highlights these warnings).
Indeed this module would work just on ContentEntities, and not ConfigEntity ones ... isn't it?
Comment #14
marcoscanoIt seems that core has some of the same issues when having to decide how to deal with multiple revisions / translations pointing to the same entity (when dealing with file usage). See #2729953: Clarify handling of multiple references to the same file from entities, #2821423: Dealing with unexpected file deletion due to incorrect file usage and all child issues.
We should keep an eye on how this is being solved in core and definitely go the same path at some point.
Comment #15
marcoscanoIn principle this was fixed in #2949853: Support revisions / moderation workflows, please re-open it if you believe that's not the case.