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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anushka-mp’s picture

Status: Active » Needs review
FileSize
5.84 KB

The 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.

miro_dietiker’s picture

Status: Needs review » Needs work

Oh this would be so nice! :-)
Some early feedback..

  1. +++ b/src/Form/RevisionOverviewForm.php
    @@ -103,16 +103,16 @@ class RevisionOverviewForm extends FormBase {
           'nid' => array(
    ...
    +        '#value' => $entity->nid->value,
    

    Oh there's still nid :-)

  2. +++ b/src/Form/RevisionOverviewForm.php
    @@ -142,7 +142,15 @@ class RevisionOverviewForm extends FormBase {
    +      $vids = array_reverse($node_storage->revisionIds($entity));
    ...
    +        ->sort('revision_id')
    

    The sequence is inverse.

  3. +++ b/src/Routing/RouteSubscriber.php
    @@ -19,14 +19,20 @@ class RouteSubscriber extends RouteSubscriberBase {
    +      if($entity->hasLinkTemplate('version-history')){
    

    Codestyle...

  4. +++ b/src/Routing/RouteSubscriber.php
    @@ -19,14 +19,20 @@ class RouteSubscriber extends RouteSubscriberBase {
    +            '_controller' => '\Drupal\diff\Controller\NodeRevisionController::revisionOverview'
    

    NodeRevisionController => EntityRevisionController

Anushka-mp’s picture

Dynamic 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)

Berdir’s picture

  1. +++ b/diff.module
    @@ -76,7 +77,7 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
     
    -    case 'diff.revisions_diff':
    +    case 'diff.revisions_diff.node':
           return '<p>' . t('Comparing two revisions:') . '</p>';
     
    

    This should match any route name that starts with diff.revision_diff.

  2. +++ b/diff.module
    @@ -86,3 +87,21 @@ function diff_help($route_name, RouteMatchInterface $route_match) {
    +/**
    + * Returns URL for each entity type revisions diff overview.
    + * @param string $entity_type_id
    + * @param int $entity_id
    + * @param int $left_vid
    + * @param int $right_vid
    + * @param string $filter
    + * @return Url
    + */
    +function diff_generate_url($entity_type_id, $entity_id, $left_vid, $right_vid, $filter = NULL) {
    +  return Url::fromRoute('diff.revisions_diff.' . $entity_type_id, array(
    +    'node' => $entity_id,
    +    'left_vid' => $left_vid,
    +    'right_vid' => $right_vid,
    +    'filter' => $filter,
    +  ));
    

    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..

  3. +++ b/src/DiffBreadcrumbBuilder.php
    @@ -19,7 +19,7 @@ class DiffBreadcrumbBuilder extends PathBasedBreadcrumbBuilder {
       public function applies(RouteMatchInterface $route_match) {
    -    if ($route_match->getRouteName() == 'diff.revisions_diff') {
    +    if ($route_match->getRouteName() == 'diff.revisions_diff.node') {
    

    Same here, needs to apply on all routes starting like that.

  4. +++ b/src/Form/RevisionOverviewForm.php
    @@ -103,16 +106,17 @@ class RevisionOverviewForm extends FormBase {
    -    $type = $node->getType();
    +    $type = $entity->bundle();
    

    $type should be renamed too.

  5. +++ b/src/Form/RevisionOverviewForm.php
    @@ -123,29 +127,42 @@ class RevisionOverviewForm extends FormBase {
    +    if ($entity->getEntityType()->id() == 'node') {
    +      $vids = array_reverse($entity_storage->revisionIds($entity));
    +    }
    +    else {
    +      $vids = \Drupal::service('entity.query')->get($entity->getEntityTypeId())
    

    As discussed, we *should* remove the node specific part, but the entity query doesn't actually work yet ;)

  6. +++ b/src/Form/RevisionOverviewForm.php
    @@ -196,7 +219,7 @@ class RevisionOverviewForm extends FormBase {
               $route_params = array(
    -            'node' => $node->id(),
    +            'node' => $entity->id(),
                 'node_revision' => $vid,
               );
    

    sounds like this needs to be more dynamic too.

  7. +++ b/src/Routing/RouteSubscriber.php
    @@ -19,14 +20,35 @@ class RouteSubscriber extends RouteSubscriberBase {
    +        $route = $collection->get('entity.' . $entity_type->id() . '.version_history');
    +        $route->setOption('_diff_entity_type', $entity_type->id());
    +        $route->setOption('parameters', array($entity_type->id() => array('type' => 'entity:' . $entity_type->id())));
    +        $route->addDefaults(
    +          array(
    +            '_controller' => '\Drupal\diff\Controller\NodeRevisionController::revisionOverview'
    +          )
    

    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.

lhangea’s picture

Ok, 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 ?

miro_dietiker’s picture

See this from ContentEntityBase.php

  public function setNewRevision($value = TRUE) {

    if (!$this->getEntityType()->hasKey('revision')) {
      throw new \LogicException(SafeMarkup::format('Entity type @entity_type does not support revisions.', ['@entity_type' => $this->getEntityTypeId()]));
    }

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.

plach’s picture

lhangea’s picture

lhangea’s picture

I 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:

The only problem is access, just default to _entity_access: view for now.

If I set the requirements to that I get some error:

Notice: Undefined offset: 1 in Drupal\Core\Entity\EntityAccessCheck->access() (line 47 of core/lib/Drupal/Core/Entity/EntityAccessCheck.php).

Maybe tomorrow I will fix that too. For now I left the requirements to:

$requirements = array(
  '_permission' => 'access content'
);

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.

lhangea’s picture

Status: Needs work » Postponed
FileSize
17.61 KB
Berdir’s picture

The 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.

lhangea’s picture

Actually 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.

Berdir’s picture

Status: Postponed » Needs work

The 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.

lhangea’s picture

Yep, I was following that issue. Very nice.

miro_dietiker’s picture

Oh yeah, party, party, party!

Hope we can unlock the revision handler soon, too, and implement fancy stuff on top of all that. :-)

lhangea’s picture

Worked 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:

  • [entity_type_id].revision_show
  • [entity_type_id].revision_revert_confirm
  • [entity_type_id].revision_delete_confirm

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 :)

Berdir’s picture

There'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.

lhangea’s picture

So, 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 ?

Berdir’s picture

Here'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.

dawehner’s picture

IMHO 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:

  • Provide a generic diff controller (see this patch, which leverages some of the entity godness we already have)
  • Provide a route provider to provide the required routes automatically
  • Provide a revision history listing with diff support
  • Get rid of the custom code for node
dawehner’s picture

FileSize
12 KB

This time with a patch

miro_dietiker’s picture

As 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?

dawehner’s picture

As 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?

Exactly, 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.

dawehner’s picture

Added a subissue to just resolve the diff controller, not the listing itself.

dawehner’s picture

jonathanshaw’s picture

Title: Offer a revisions tab for all entities » [Meta] Offer a revisions tab for all entities
Status: Needs work » Active
Issue tags: +Needs issue summary update

Now 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.

jonathanshaw’s picture

miro_dietiker’s picture

Yeah 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.

Musa.thomas’s picture

hello 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 ?

mpolishchuck’s picture

Hello 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:

  • Controller for revision tab works directly with route match, because there is no route enhancer (like for entity view or entity form) to convert entity_type_id parameter to something universal and applicable to universal revision tab controller. But implementing this route enhancer here is not relevant actually...
  • Revision overview form mostly based on node revision overview, which means can contain some improper solutions (I mean they need for node, but don't need for another entities)
  • Requires entity routes with predefined suffixes, such as entity.entity_type.version_history, entity.entity_type.revision, entity.entity_type.revision_revert, entity.entity_type.revision_delete. All custom entities what I've seen implement these routes, but I didn't see any universal tools in Drupal core to declare them, which means if developer don't copy-paste revisions logic from node, he is free to define his own routes. This can be a problem
  • Requires manual adding of the revisions-diff link template
  • Requires manual adding of the html_diff route provider
  • Does not contain any tests

How to use:

  1. Add route provider to your entity: html_diff => Drupal\diff\Routing\DiffRouteProvider
  2. Add template link to your entity: revisions-diff => /my-entity/{my_entity}/revisions/view/{left_revision}/{right_revision}/{filter}

I hope this patch will be helpful for someone

DamienMcKenna’s picture

It's also worth noting that the Entity API module has a similar issue: #2625122: [Meta] Implement a generic revision UI

uceap-web’s picture

Testing #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?

  • The application did not respond in time.
  • Maximum execution time of 120 seconds exceeded (vendor/caxy/php-htmldiff/lib/Caxy/HtmlDiff/HtmlDiff.php:853)
  • PHP Memory Limit: 256M (pantheon professional plan)
  • max_execution_time: 120
  • Drupal version: 8.6.2
miro_dietiker’s picture

Status: Active » Needs work
Issue tags: +Needs tests

Did a quick look into the code provided without testing, thus likely incomplete:

+++ b/src/Form/EntityRevisionOverviewForm.php	2018-09-13 01:32:24.009672579 +0300
@@ -0,0 +1,395 @@
+      ->allRevisions()
...
+    foreach ($revision_ids as $key => $revision_id) {
...
+      if (isset($revision_ids[$key + 1])) {
+        $previous_revision = $entity_storage->loadRevision($revision_ids[$key + 1]);

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.

dpi’s picture

Assigned: Unassigned » dpi
AaronMcHale’s picture

If #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.

jidrone’s picture

Hi @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.

AaronMcHale’s picture

@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.

acbramley’s picture

#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.

dpi’s picture

Assigned: dpi » Unassigned
hctom’s picture

I 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:

  • Use getRawParameter() instead of getParameter() in EntityRevisionController::revisionOverview() because the parameter is upcasted now and would result in a fatal error, when the entity object is passed to entity storage's load() method
  • Using the correct route name for entity.{$entity_type}.revision_revert_translation_confirm in EntityRevisionOverviewForm::buildForm()
  • Using the correct route name for entity.{$entity_type}. revision_revert_confirm in EntityRevisionOverviewForm::buildForm()
  • Using the correct route name for entity.{$entity_type}.revision_delete_confirm in EntityRevisionOverviewForm::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.

m.stenta’s picture