Problem/Motivation
Diff alters the revision tab of nodes to improve the UI.
It also tries to be entity generic so all fields can be configured in its diff behavior.
However, only the node has a revision tab. This tab is provided by NodeController.
For Drupal 8.0, this will not change.
If now every single entity providing modules is required to offer an own revisions tab, things will get ugly.
The tabs will be inconsistent and missing with most entity types.
Proposed resolution
If diff wants to provide a nice revision experience, we should
- Make diff offer a revisions tab if not yet present
- Create a core followup issue to add a revisions tab to all revisionnable entities
Remaining tasks
Decide about release target. Pushing to major since i understood we want to be entity general and not node centric.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-30-40.txt | 2.08 KB | hctom |
#40 | diff-support_for_all_entities-2452523-40.patch | 15.59 KB | hctom |
| |||
#30 | diff-support_for_all_entities-2452523-30.patch | 15.67 KB | mpolishchuck |
#21 | 2452523-21.patch | 12 KB | dawehner |
#16 | interdiff-9-16.txt | 33.19 KB | lhangea |
Comments
Comment #1
Anushka-mp CreditAttribution: Anushka-mp at MD Systems GmbH commentedThe first step in making diff compatible with other entity types.
- look for the version-history link template in the entity and alter the route. ( maybe we need to introduce an option to alter the link templates of entities to be compatible with diff)
- Node centric revisionOverview() method updated to support other entities, maybe we need to introduce new forms I'm not sure at this point.
- RevisionOverviewForm refactored and entity query added to get the revision ids of other entities.
Comment #2
miro_dietikerOh this would be so nice! :-)
Some early feedback..
Oh there's still nid :-)
The sequence is inverse.
Codestyle...
NodeRevisionController => EntityRevisionController
Comment #3
Anushka-mp CreditAttribution: Anushka-mp at MD Systems GmbH commentedDynamic routing introduced and node centric route removed. A method introduced to generate URL from these dynamic routes. now stuck with the core issue #2458543: Entity query age(EntityStorageInterface::FIELD_LOAD_REVISION) only gets current revision ID.
Work in progress patch is uploaded here.
(Started from the beginning, no interdiff)
Comment #4
BerdirThis should match any route name that starts with diff.revision_diff.
vid is node specific, the generic term is revision_id, so we should rename it everywhere.
Also documentation and stuff ;)
This method could live on the new service that you are creating in the paragraph issue..
Same here, needs to apply on all routes starting like that.
$type should be renamed too.
As discussed, we *should* remove the node specific part, but the entity query doesn't actually work yet ;)
sounds like this needs to be more dynamic too.
As discussed, this should check if that route exists and if not, create it.
The only problem is access, just default to _entity_access: view for now.
Comment #5
lhangea CreditAttribution: lhangea commentedOk, all content entities can have revisions since they all implement RevisionableInterface but ATM as far as I know only nodes can be saved as revisions (from the user interface). For example the modifications done to users can't be saved as revisions by default. I suppose that's sort of OK with us because we only provide the interface for revision comparison and how those revisions are actually created is handled by core/other contrib modules. Or, should we handle that part too ?
Comment #6
miro_dietikerSee this from ContentEntityBase.php
So if revision key is not defined in annotation, an entity does not support revisions.
See also modules/node/src/Entity/Node.php where that key is defined.
With the start of this issue i never intended to modify edit workflows. I understood that diff offers to investigate revisions and compare things passively.
A discussion to alter / improve the UI to create revisions is a completely different thing to me.
Comment #7
plachCreated a related meta issue: #2465901: [META] Make entity revision translation work.
Comment #8
lhangea CreditAttribution: lhangea commentedComment #9
lhangea CreditAttribution: lhangea commentedI worked on this issue and it's almost solved. Basically now we need to work on the #2458543: Entity query age(EntityStorageInterface::FIELD_LOAD_REVISION) only gets current revision ID core issu. After that issue is solved the diff should work fine for all revisionable entities.
Also re-rolled the patch.
Tried to address all the suggestions of Miro and Berdir and mostly solved them.
However there's an important one left:
If I set the requirements to that I get some error:
Maybe tomorrow I will fix that too. For now I left the requirements to:
just so that the diff works but that will be changed.
Other than that, it works for nodes as it worked before but now everything is generalized to entities rather than nodes as before.
Comment #10
lhangea CreditAttribution: lhangea commentedComment #11
BerdirThe syntax for _entity_access is
_entity_access: <entity_type>.view
(where entity type actually maps to the route argument, not the entity type, but that's the same almost all the time), otherwise the access check doesn't know on which object ->access() needs to be called.#2462731: Check RevisionLogInterface for summary and provide is a bit confusing, we might want to introduce such a handler here. Everything that might possibly be different based on the entity type should be wrapped in a method on that handler.. one for generating the routes, one for checking access, one to get the revision author, date, summary, .. Then we can have simple assumptions in a default handler and a customized implementation for nodes. On the other side, trying to do that all in a single issue might be too complicated.
Comment #12
lhangea CreditAttribution: lhangea commentedActually there should be more in that interdiff. The part with route subscriber and dynamic routes didn't get in the interdiff :).
And for _entity_access the syntax was incorrect, you're right.
As for the handler I would say after fixing this one to continue in the created issue as this patch is very big already.
Comment #13
BerdirThe core issue got in :)
The if/else to get the revision ID's can be removed as well now, the entity query should now work nodes as well.
Comment #14
lhangea CreditAttribution: lhangea commentedYep, I was following that issue. Very nice.
Comment #15
miro_dietikerOh yeah, party, party, party!
Hope we can unlock the revision handler soon, too, and implement fancy stuff on top of all that. :-)
Comment #16
lhangea CreditAttribution: lhangea commentedWorked a little bit on this. And made it work for other content entities like block_content :)
But...
If you try right now it won't work because:
We assume that every entity defines these 3 routes:
But they don't.
Only node module does this ATM. But, if they did it'd work just fine.
There are some other inconsistencies. Not very big like the above but still. E.g. I saw that block_content entity doesn't have fields like: revision_timestamp and revision_uid so the interface doesn't look as good as for nodes but maybe we can solve all this with the handlers we are talking about.
Basically the missing routes are the big ones and because of those it doesn't work ATM for other entities unless you comment those sections from the code. OR I could check for route existence and if it doesn't exist just don't display the links (Although I think it would be better to have them because what is the point of having revisions if we cannot look at them or revert them ? ).
In the patch the EntityRevisionController is treated as a new file even though it is just NodeRevisionController renamed. I have the git settings done correctly but the differences are higher than the git threshold for treating a file as renamed :)
Comment #17
BerdirThere's not much in those controller that isn't generic. So we could consider to provide generic implementations of those, just like we completely replace the revision overview anyway.
If we do that, then we need to think about route names for them though. Note that it's not entity_type_id.revision_show, it's module.revision_show, module and entity_type_id just happen to be the same string in many cases. So if we provide generic versions, we need to think about namespacing them correctly. Unfortunately, a lot of route names weren't standardized on entity.entity_type_id.link_template, but we could either use that, or something like diff.entity_type_id.revision_show, to avoid clashes with future core versions. The handler would then a) decide which routes need to be defined if any, and b) return the route name/object for each link. Yes, those handlers are going to have lots and lots of methods, but that really because core didn't manage to introduce any kind of consistency/standardization for revision UI's.
On rename: Try to experiment with -MX, where X is the amount of a file that still needs to be the same for git to treat it a move. -M40 will consider it a move when at least 40% of the file is still the same.
Comment #18
lhangea CreditAttribution: lhangea commentedSo, then it seems that in order to advance with this one we would need to work on handlers (at least on the blocking ones here which are the ones generating/providing the routes for viewing/reverting/deleting a revision).
If we (diff module) write those handlers are we going to write 1 handler / revisionable content entity ?
Are we going to allow some kind of pluggability ?
Comment #19
BerdirHere's a relevant core issue: #2350939: Implement a generic revision UI
I'm not exactly sure how the handler structure would look like. But you would start with a base class and an interface, and as needed, add entity type specific implementations. Hopefully with as few overrides as possible.
Comment #20
dawehnerIMHO this is yet another issue which tries to solve too many things at the same time.
You could totally split up this issue into at least 3 subissues and iterate quicker:
Comment #21
dawehnerThis time with a patch
Comment #22
miro_dietikerAs far as i understood we wanted to postpone this since the entity contrib module is about to add similar parts and diff should then build on top of it?
Comment #23
dawehnerExactly, this is the idea of the patch so far:
a) Just provide a diff controller, no actual revision listing controller, as this will be provided by entity api at some point and maybe extended by diff module?
b) Leverage some of the functionality like the conversion of revision IDs to entity revision objects on routes for this diff controller.
Comment #24
dawehnerAdded a subissue to just resolve the diff controller, not the listing itself.
Comment #25
dawehner.
Comment #26
jonathanshawNow that #2634212: Offer a diff controller for all entity types leveraging entity API module has landed, supplying both a genericEntityController and a DiffRouteProvider service, we can move forwards with other aspects of this.
For example:
- remove NodeRevisionController.php
- modify the routing.yml to use GenericentityController not NodeRevisionController
- identify which other parts of the #16 patch are still relevant
- identify what core and Entity issues we are still blocked on, if any
I suggest either this issue or a new issue become a meta issue to keep track of these subissues. At the least the present IS needs updating.
Comment #27
jonathanshawCreated #2759627: Remove redundant NodeRevisionController as a sub issue.
Comment #28
miro_dietikerYeah i'm happy to hear an update on the status of things...
Would be cool if someone could help moving forward with this aspect.
For a first stable release of diff, it would be great, but it's not critical for us.
We are focussing on clean configurability and fixing bugs for the release.
Comment #29
Musa.thomashello guys I'm not sure to understand what we need for a new custom entiy to get revision tab.
Is there any patch include in dev diff module version or i should apply one of this patch to RC-1 ?
Comment #30
mpolishchuck CreditAttribution: mpolishchuck at Alpha Web Group for Dazzle commentedHello everyone.
I've done some work for adding revision tab for all entities. This patch works for version rc2, but have several problems such as:
How to use:
I hope this patch will be helpful for someone
Comment #31
DamienMcKennaIt's also worth noting that the Entity API module has a similar issue: #2625122: [Meta] Implement a generic revision UI
Comment #32
uceap-web CreditAttribution: uceap-web commentedTesting #30 on a custom entity, it does works (thanks!), but sometimes I get a timeout error. Entity has lot of fields. I did increased resources while testing locally, and it does loads, but it takes a long time. Any suggestions?
Comment #33
miro_dietikerDid a quick look into the code provided without testing, thus likely incomplete:
This loops through them and loading each of them. No paging, no limit.
If you have a complex entity type, then this is very slow and eats a ton of resources like RAM.
We already dropped certain complexity to make the table working by no more generating a diff on the fly.
This can be improved by a factor of 2 by simply keeping the previous revision for the potential next revision if already loaded.
In fact the loaded previous revision then isn't even used since we dropped on-the-fly summary creation, see getRevisionDescription.
Maybe we should update the method, make the argument optional and not load it anymore.
Shouldn't this code replace our FormOverview as well?
Also core doesn't cache revision loads, so this really results in a crazy amount of queries as well loading each bit from the DB.
This core issue tries to improve the situation: #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load()
And then i still see zero test coverage.
Comment #34
dpiComment #35
AaronMcHaleIf #2350939: Implement a generic revision UI is committed this won't be necessary, might be worth postponing this issue and then either repurposing or closing this issue once that issue makes it in.
Comment #36
jidrone CreditAttribution: jidrone at Mobomo for Vibrent Health commentedHi @AaronMcHale,
Currently, the issue you related is postponed too, so what will happen with this improvement if we postpone it, is there someone working on this?
There is another issue from contrib Entity API module #2625122: [Meta] Implement a generic revision UI.
Comment #37
AaronMcHale@jidrone #2350939: Implement a generic revision UI is postponed on #3043321: Use generic access API for node and media revision UI and possibly also on #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes..
So once those two issues are in then work can continue on #2350939: Implement a generic revision UI, but there isn't much progress at the moment. If I had more time myself I'd try and push forward on those, so any help to get either of those two blocking issues moved forward would be great.
I'm not involved in the Entity API module issue #2625122: [Meta] Implement a generic revision UI so can't provide any context regarding the progress of that. Ideally it would be great if any available resources that people have could be focused on the Core issues, that way everyone benefits in the long run.
Comment #38
acbramley CreditAttribution: acbramley at PreviousNext commented#2350939: Implement a generic revision UI is in a very good state now, only blocked on 1 other issue #3043321: Use generic access API for node and media revision UI
We are using this generic UI for blocks and other entity types with modules like https://www.drupal.org/project/block_content_revision_ui
This issue should move towards extending the new generic revision UI instead.
Comment #39
dpiComment #40
hctomI came across this issue while trying out the media_revisions_ui module, where I also wanted to add diff support for within Drupal 9.2.6. The proposed patches almost work, so I created a new one that adds the following changes in order to make it work again:
getRawParameter()
instead ofgetParameter()
inEntityRevisionController::revisionOverview()
because the parameter is upcasted now and would result in a fatal error, when the entity object is passed to entity storage'sload()
methodentity.{$entity_type}.revision_revert_translation_confirm
inEntityRevisionOverviewForm::buildForm()
entity.{$entity_type}. revision_revert_confirm
inEntityRevisionOverviewForm::buildForm()
entity.{$entity_type}.revision_delete_confirm
inEntityRevisionOverviewForm::buildForm()
So this only makes the patch functional again, but does not add any proposed changes (e.g. those from comment #33) and no additional test coverage yet, but in my opinion it would be great to use this ticket to prepare the diff module to be generic enough to let all entity types can have support for it on their revision pages. Those changes should be added first, before reworking the current code that sometimes only targets nodes exclusively - which should be much easier then, when all the generic code is already available.
Comment #41
m.stenta#2350939: Implement a generic revision UI has landed in core! 🎉
Change record: https://www.drupal.org/node/3160443