Implement a generic revision UI

Yes, we have no revision ui for block content, even though it supports revisions

Wait, but do we need to expose a UI for everything? Or is this just a 'in case you want it' kinda thing.

I don't think block content revision ui belongs in core. But a reusable base class I could add on contrib sounds efficient.

I agree with what has been said before. Having a generic base class that will only be used for node revisions by default offers developers a good starting point for making their own revision UI when they want to.

It should be usefull for custom entities as well, not just block content. I dont think we would need to expose a UI by default but a base class like larowlan says would be very helpful.
I'm looking forward to see some more opinions.

Here's a start

The last submitted patch, 6: generic-revision-trait-2350939.1.patch, failed testing.

Ok, green - any reviewers?

Thanks for bringing this back up! I had already forgotten about it.

In general I'm not sure we should make this a route controller, and considering the complexity and the fact the trait calls entityManager() which is not defined anywhere in the trait (and I know why), making this a trait sounds messy as well. Can we make this an entity handler base class? The code in its current form isn't usable for non-entities anyway. Besides, using a class rather than a trait for something this complex will allow us to build slightly more solid code (no dark magic entityManager() for instance).

In any case, here is a short review of the patch:

1. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  public function revisionShow($revision_id) {
   +    $entity = $this->entityManager()->getStorage($this->getRevisionEntityTypeId())->loadRevision($revision_id);
   

   We generally start methods with a verb.

2. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +    if (isset($page[$this->getRevisionEntityTypeId() . 's'][$entity->id()]['#cache'])) {
   

   unset() works fine if the value to unset does not exist yet, so we don't need a condition here.

3. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +   * Page title callback for an entity revision.
   

   One-line summaries should also start with a verb.

4. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  /**
   +   * Determines if the user has permission to revert revisions.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to check revert access for.
   +   *
   +   * @return bool
   +   *   TRUE if the user has revert access.
   +   */
   +  abstract protected function hasRevertRevisionPermission(EntityInterface $entity);
   +
   +  /**
   +   * Determines if the user has permission to delete revisions.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to check delete revision access for.
   +   *
   +   * @return bool
   +   *   TRUE if the user has delete revision access.
   +   */
   +  abstract protected function hasDeleteRevisionPermission(EntityInterface $entity);
   

   Did you check if this can be done through access controllers? I understand it's probably difficult at the least; I just want to make sure that approach has been researched.

5. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  abstract protected function buildRevertRevisionLink(EntityInterface $entity, $revision_id);
   +
   +  /**
   +   * Builds a link to delete an entity revision.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to build a delete revision link for.
   +   * @param int $revision_id
   +   *   The revision ID of the delete link.
   +   *
   +   * @return array
   +   *   A link render array
   +   */
   +  abstract protected function buildDeleteRevisionLink(EntityInterface $entity, $revision_id);
   

   The same goes for these link builders and entities' list builder classes.

6. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  abstract protected function getRevisionDetails(EntityInterface $revision, $is_current = FALSE);
   

   Maybe rename this to getRevisionDescription(), or something similar that indicates the output is human-readable?

7. +++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
   @@ -0,0 +1,33 @@
   + * Defines an interface for entities with timestamped revisions.
   

   This interface is not in an entity namespace, so we should not mention entities anywhere in the file. There is no technical reason for this interface to be entity-specific in its current form, except for my next comment.

+++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
@@ -0,0 +1,33 @@
+   * @return \Drupal\Core\Entity\EntityInterface
+   *   The called entity.

Should be @return $this.

We tried to start something similar in context of Diff contrib:
#2452523: [Meta] Offer a revisions tab for all entities →

We would be very happy to see this happen in core.
Note our discussions that the revision summary is a problem and there should be a handler that allows alteration in contrib.
A problem is here that i see you referring to $revision->revision_log that is only present in node revision entities from Node.php baseFieldDefinitions().
All other entities will provide an empty description.
I'm confused the patch above only implements the whole functionality on Node only? I thought the issue is about displaying a revision tab for every entity type.

The issue is essentially just about making the code generic.

But yes, without using it somewhere else, at least in a test module (we could try to integrate it automatically in entity_test for all revisionable entity types there?), we won't see stuff like the revision_log and uid that are node specific...

Rebased.
Did all changes exept the 3, that I didn't know what to put there, was like this before, and can't get a verb for that.
And 4, 5, that I don't know how to do that.
Also in RevisionControllerTrait, I'm not sure if everything it's fine there.

The last submitted patch, 13: implement_a_generic-2350939-13.patch, failed testing.

The last submitted patch, 13: implement_a_generic-2350939-13.patch, failed testing.

Fixed the failing test, basically improved the reroll, added changes that were made in this time.

mkalkbrenner → queued 16: implement_a_generic-2350939-16.patch for re-testing.

The last submitted patch, 16: implement_a_generic-2350939-16.patch, failed testing.

I like this approach. But it has to be adjusted to the latest changes introduced by #2465907: Node revision UI reverts multiple languages when only one language should be reverted → .

+++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
@@ -0,0 +1,218 @@
+        $links = [];
+        if ($revert_permission) {
+          $links['revert'] = $this->buildRevertRevisionLink($entity, $vid);
+        }
+        if ($delete_permission) {
+          $links['delete'] = $this->buildDeleteRevisionLink($entity, $vid);
+        }

In order to increase the re-usage of revisionOverview() the list of links should be moved to a separate method with the trait.

I did the rebase and moved the links to another method.

I think we should go one step further. The operation link targets NodeRevisionDeleteForm, NodeRevisionRevertForm,
NodeRevisionRevertTranslationForm seem to be easily adjustable for different entities as well. We should therefor provide base classes or traits, too.

Just some nitpicks:

1. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,249 @@
   +        '%date' => format_date($entity->getRevisionCreationTime())
   

   new code should use \Drupal\Core\Datetime\DateFormatter::format().

2. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,249 @@
   +  protected function getLinks(EntityInterface $entity, $revision_id) {
   

   I suggest to name this method getOperations() or getOperationLinks().

3. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -283,4 +190,112 @@ public function addPageTitle(NodeTypeInterface $node_type) {
   +              'url' => $has_translations ?
   +                Url::fromRoute('node.revision_revert_translation_confirm', ['node' => $entity->id(), 'node_revision' => $revision_id, 'langcode' => $langcode]) :
   +                Url::fromRoute('node.revision_revert_confirm', ['node' => $entity->id(), 'node_revision' => $revision_id]),
   

   leading whitespaces

What you mean exactly? Like having a TranslatableRevisionControllerTrait that extends from RevisionControllerTrait for those entities that are translatable and add the methods there or just extract buildRevertRevisionTranslationLink from buildRevertRevisionLink in RevisionControllerTrait?

Those small fixes are solved in this patch.

@edurenye:
The goal of this issue is to isolate the re-usable parts of node revision handling and make them available for all entities. But the current patch only targets the NodeController that lists node revisions. But reverting a revision is actually done by NodeRevisionRevertForm and NodeRevisionRevertTranslationForm.
If you want to implement a UI for any entity revisions, you need to implement their logic again. Therefor I suggest to create corresponding traits or something like a EntityRevisionRevertBaseForm.

We discussed about this topic in the #DrupalCampVienna2015, and we agreed to implement the generic revision UI in entity API and then move it to Core.

Here is the issue I created for that: #2625122: [Meta] Implement a generic revision UI →

This would land in 8.1 at the earliest.

Drupal 8.1.0-beta1 → was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Where, if at all, does this fit into the Worfklow Initiative? Does the content_moderation approach over in #2725533: Add experimental content_moderation module → supersede this?

Let's see what they say :)

One part, for example the generic revert controller could be totally brought in already into core.

Drupal 8.2.0-beta1 → was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Adding the related Diff module issue.

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

This is now feature complete in contrib entity module. We can start moving it to core. Media would also really benefit from it.

This is far away from done, but i think i collected all relevant implementation and test classes into this patch.

Test are still failing...

The last submitted patch, 35: 2350939-35.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Some small push. Hopefully more people will jump in to push this forward.

The last submitted patch, 37: 2350939-37.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

1) Drupal\FunctionalTests\Routing\RevisionRouteAccessTest::testRevisionRouteAccess
Unable to install modules entity_test, user, entity, block due to missing modules entity.

This should get the tests running.

The last submitted patch, 39: 2350939-39.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Next try to fix the tests

The last submitted patch, 42: 2350939-42.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Fix last failing test.

Thanks everyone for working on this, it would be a great step towards cutting out the work required to get to a node-like custom entity type.

My review is as follows. I think the biggest outstanding thing to resolve was mentioned back in #10, that is we shouldn't lock in and assume a set of magically named permissions for each entity type.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    // @todo Expand the entity storage to load multiple revisions.
   

   We should link to #1730874: Add support for loading multiple revisions at once → here.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
   +      if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)
   +          ->isRevisionTranslationAffected()
   +      ) {
   

   Not sure splitting this on to multiple lines makes it more readable.

3. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    $build[$entity->getEntityTypeId() . '_revisions_table'] = [
   +      '#theme' => 'table',
   +      '#rows' => $rows,
   +      '#header' => $header,
   +    ];
   

   Is adding the entity type ID into render array keys just making this harder to override?

4. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    // We have no clue about caching yet.
   +    $build['#cache']['max-age'] = 0;
   

   We should add a follow up here at least.

5. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,200 @@
   +    if ($this->hasDeleteRevisionAccess($entity_revision)) {
   +      $links['delete'] = $this->buildDeleteRevisionLink($entity_revision);
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function hasDeleteRevisionAccess(EntityInterface $entity) {
   +    return $this->currentUser()->hasPermission("delete all {$entity->id()} revisions");
   +  }
   

   We should probably add access controls to the delete route and then simply base visibility/access of the links themselves on those same access controls. If I deny access to a route outside of just the permission, that should propagate to all links to that route.

6. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  /**
   +   * Creates a new RevisionOverviewController instance.
   +   *
   +   * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
   +   *   The date formatter.
   +   */
   +  public function __construct(DateFormatterInterface $date_formatter, RendererInterface $renderer) {
   

   I disagree we even need to document these params, but if we document one we should probably document the other.

7. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  public function revisionOverviewController(RouteMatchInterface $route_match) {
   +    return $this->revisionOverview($route_match->getParameter($route_match->getRouteObject()->getOption('entity_type_id')));
   +  }
   

   I wonder why we need this method at all. Can't "revisionOverview" be the entry point?

8. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +          'message' => ['#markup' => $markup, '#allowed_tags' => Xss::getHtmlTagList()],
   

   getRevisionLogMessage is the "value" column from a "string_long" field. I think that'd already be considered unsafe markup and would be escaped? Is "allowed_tags" making this more or less permissive?

9. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionOverviewController.php
   @@ -0,0 +1,160 @@
   +  protected function hasRevertRevisionAccess(EntityInterface $entity) {
   +    return AccessResult::allowedIfHasPermission($this->currentUser(), "revert all {$entity->getEntityTypeId()} revisions")->orIf(
   +      AccessResult::allowedIfHasPermission($this->currentUser(), "revert {$entity->bundle()} {$entity->getEntityTypeId()} revisions")
   +    );
   +  }
   

   I do agree with the previous comments, why isn't this $entity->access('revert'), with access control handlers deciding if such controls are based on a permission or not.

10. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -322,7 +322,7 @@ protected function urlRouteParameters($rel) {
    -    if ($rel === 'revision' && $this instanceof RevisionableInterface) {
    +    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface) {
           $uri_route_parameters[$this->getEntityTypeId() . '_revision'] = $this->getRevisionId();
         }
    

    We could possibly do #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → ahead of this issue if we wanted to reduce the scope here.

11. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionAccessCheck.php
    @@ -0,0 +1,162 @@
    +      $_entity = $route_match->getParameter($entity_type_id);
    ...
    +      $_entity_revision = $route_match->getParameter($entity_type_id . '_revision');
    

    Not sure why these are prefixed with "_"?

12. +++ b/core/lib/Drupal/Core/Entity/EntityRevisionAccessCheck.php
    @@ -0,0 +1,162 @@
    +  protected function checkAccess(ContentEntityInterface $entity, AccountInterface $account, $operation = 'view') {
    

    This method is a good example of why delegating to an access control handler might be a good idea. This hard codes bundle based granularity and also seperate permissions for update/delete/revert. What if my entity type is simple and I just want a single "administer revisions" permission to cover the whole lot?

13. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php
    @@ -24,6 +24,7 @@
    + *       "revision" = "Drupal\Core\Entity\Routing\RevisionRouteProvider",
    

    Given these are also providing HTML routes, maybe this should be RevisionHtmlRouteProvider for consistency?

14. +++ b/core/tests/Drupal/FunctionalTests/Routing/RevisionRouteAccessTest.php
    @@ -0,0 +1,84 @@
    + * @runTestsInSeparateProcesses
    + *
    + * @preserveGlobalState disabled
    

    What are these annotations for?

Re: #45.1, the strikethough tells me that has already landed 🙂

Thanks for the review @Sam152!
Had just a bit of time, so here's just a bit of progress on that:

• #45.1 - Yay, done (I hope)
• #45.2 - Done
• #45.3 - Possibly, changed to just using 'entity_revisions_table'.

Setting to needs review for the tests to run, but obviously this is still Needs work to address the rest of #45.

OK some more progress, did all I could but most likely someone else will need to help out here :)

#45.5 Had a look, but unsure how to do this,
RevisionRouteProvider::getRevisionRevertRoute() is already adding:

$route->addRequirements([
  '_entity_access_revision' => "$entity_type_id.update",
]);

Is this where we should do this?
#45.6 I agree, replaced with {@inheritdoc}
#45.7 I dont see why not, done (I hope)
#45.8 I don't know, left as is.
#45.9 - Same deal as for #45.5
#45.11 No reason that I can think of, fixed.
#45.12 I agree.
#45.13 Done.
#45.14 Dont know :)

The last submitted patch, 49: 2350939-49.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Reverting the changes related to #45.7, apparently we cant use a method from a trait?

Still needs work for:

• #45.4
• #45.5
• #45.8
• #45.9
• Decide on #45.10
• #45.14

And perhaps think of a different solution than just using the method from the trait for #45.7

There is also an issue about replacing the revision table with a view #1863906: [PP-1] Replace content revision table with a view →

Just want to throw my support behind this and the idea of using a View for the revision list, it would definitely make life easier for custom content entities, as well a reduce duplication in core and automatically fix the referenced issues for example where custom blocks have no revision UI.

Bottom line is if we can do it for the Field UI, then why not the Revision UI.

It's actually not so easy. We can write generic controllers/forms/handlers that are entity type agnostic and provide defaults for them, but we can *not* automatically generate views for all entity types, we have no API for that. Every entity type would have to create and provide that view themself and there's nothing that ensures that they are consistent. Best-case scenario there is that drupal console or so could generate those views.

Views are also much harder to extend for modules like diff.

> Bottom line is if we can do it for the Field UI, then why not the Revision UI.

Field UI has nothing to do with views, that's exactly why it can be generic.

Yeah, I believe #1863906: [PP-1] Replace content revision table with a view → to be blocked, see my comment #56 → there.

Fair points, although I can still see this working.

One possible solution would be for isntead of the Views being pages with rotues, the Views are essentially Blocks, then we create some kind of generic Controller that creates the pages needed utilising the Blocks. That way either like with the Fied UI in the Cotnent Entity configuration you just specify which route to use to generate the Revision Routes and Pages from, or you implmenet the generic Controllers and pass in whatever arguments are needed.

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -322,7 +322,7 @@ protected function urlRouteParameters($rel) {
-    if ($rel === 'revision' && $this instanceof RevisionableInterface) {
+    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface) {

I think #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → has a better fix if ($this instanceof RevisionableInterface && strpos($rel, 'revision') === 0) {.

After thinking about this quite a bit and looking at what you get when you create a custom Content Entity using Drupal Console, I propose the following plan:

1. We postpone this issue on #1863906: [PP-1] Replace content revision table with a view → and re-purpose it so that it's not Node specific but instead works for all Content Entities.
2. To achieve the above it will invovle converting the View from a Page to an Embed and then in the Revision Controllers for Node and other core module use views_embed_view() to embed it, passing the correct parameters that the View can then use as relationships or contextual filters. This also means that the View doesn't need to care about permissions that the Content Entity has defined.
3. Once the above is committed and fixed we continue with this issue, build on the patch here to include the new View and continue to abstract away any boilerplate code that Content Entities have to deal with.
4. Once committed and done, figure out anywhere that we can replace per Entity code with these changes, as well as encourage Drupal Console (and others?) to adopt this.

Hiding old patches.

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Changing my mind regarding #54, #57 and #60, let's not worry about Views, if a particular Entity Type wants to use Views then they can but it would be easier here to just go the current approach of using a ListBuilder/Controller.

Just doing a quick review based on full patch in #51:

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Derivative/RevisionsOverviewDeriver.php
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getDerivativeDefinitions($base_plugin_definition) {
   +    $exclude = ['node'];
   

   I'm assuming this is done just to avoid having to deal with any custom logic in the Node module along with all of the Core and Contrib modules which modify the Node Revision UI? But maybe we can make it work?

2. +++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
   

   I feel like we'd be better off just modifying the DefaultHtmlRouteProvider and AdminHtmlRouteProvider classes instead and relying on the value of the Entity Type Key show_revision_ui to determine whether to inject the new Routes. This means the new Routes are already in place for core Entities that use revisions, such as Custom Blocks and Media, and makes it easier for Contrib and Custom modules to integrate. Alternatively we could add a new Entity Type Annotation Key in the event an Entity Type doesn't want these new Routes but still wants the existing UI, this approach might be better for backwords compatibility with Contrib as well.
   
   

3. We should provide a revision_delete Route, and translation_revert for Entity Types which support revisions with different translations.
   
   
4. We should support allowing Entity Types to provide their own handlers for Controllers and Forms, so an Entity Type could optionally specify these handler keys in the Entity Type Annotation, otherwise if the Entity Type doesn't specify them we just use the Controllers/Forms we provide here. This might also mean we don't need the exclude for Node that I mentioned in point 1 above.
   
   
5. I noticed there's quite a few references written in plaintext in the code to class paths e.g. '_controller' => '\Drupal\Core\Entity\Controller\EntityViewController::viewRevision', might be better to use Class Constants instead, e.g. '_controller' => EntityViewController::class . '::viewRevision'

Thanks
-Aaron

Rerolled #51.

NW for #52.

We still need answers for #10 too, with respect to permissions issues.

Do we have a more generic issue for 'support revision related values for $op' in entity access control handlers?

Added an issue to address the revision access api, as this issue shouldn't be the place we do that, for the sake of scope

I think we should postpone this on that - I couldn't find an existing issue - thoughts?

Agree with #67, it makes sense to wait until we have clarification around access controls before doing anything more here.

Agreed.

Is there a link to the issue re: revision access api?

Don't know how I missed it in #67 above. 🤦‍♂️

Actually 64 is messed up, the change introduced as seen in the interdiff makes all revisions shown in the UI refer to the default revision.

Tagging for Drupal Camp Scotland 2019, hoping I'll have time to progress this a bit more during the Contribution Day.

Here is what this patch does:

• Moved version_history to be above revision in RevisionHtmlRouteProvider as this seems to make more sense.
• RevisionHtmlRouteProvider::getRoutes renamed route variables to be consistent with DefaultHtmlRouteProvider.
• In RevisionHtmlRouteProvider started using ::class instead of writing the full class paths as strings. (#62.5)

Partly changed my mind about #62.2 but we will need to communicate that the additional route provider needs to be added for Entities.

That's all I had time to do today at Drupal Camp Scotland Contribution Day but there are a bunch of additional things I want to do so leaving this assigned to me and I'll continue working on this, also due to an annoying technical bug and now time constraints only have an interdiff on this occasion. Future changes will include:

• Need to add a AdminRevisionHtmlRouteProvider.
• Add title callbacks for use in RevisionHtmlRouteProvider.
• Investigate #62.1
• Introduce some handler classes and forms which can be used in the Entity Annotation, along with an interface. (#62.4)
• Add routes suggested in #62.3

Thanks for pushing this forward, @AaronMcHale! 👏I like what I'm seeing so far, and am interested to see where you're taking this!

Thanks @Wim Leers :)

I discovered that this is also blocked on #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → as the version_history route will throw an exception which that issue will fix.

Building on my previous interdiff:

1. Introduces a new handler class key version_history which is required for the version_history route.
2. Renamed RevisionOverviewController to VersionHistoryController.
3. This patch introduces a new interface VersionHistoryControllerInterface, the purpose of this interface is to ensure that any version_history handler class provides the most basic of methods to allow the version_history route to be generated. Currently it has only two methods renderVersionHistory and versionHistoryTitle, these are used for the _controller and _title_callback of the version_history route. I introduced this interface because revision routes have existed in a lot of places for a long time so I didn't want us making any assumptions about what a module might want to pass as the handler class, whether that be a dedicated Controller or a Controller class with other purposes, this really needs to be a easily compatible drop-in replacement.
4. Introduced RevisionAdminHtmlRouteProvider which works exactly like the existing AdminHtmlRouteProvider but for version_history and revision_revert routes.
5. Renamed RevisionsOverviewDeriver to VersionHistoryDeriver.
6. Removed $exclude check from VersionHistoryDeriver and replaced with a check for a new option on the version_history route named add_core_version_history_local_task, this seems like the most backwords compatible approach that's safe for both core and contrib. Essentially this prevents two "Revisions" tabs from appearing for modules which provide their own version_history route and local task.
7. Removed calls to deprecated functions in RevisionRevertForm.
8. Added a new method RevisionHtmlRouteProvider::getHandlerClassWithImplementationChecks which is being used to validate the handler class and throw a RuntimeException with a friendly message if the class doesn't implement VersionHistoryControllerInterface, I also made it reusable in case it's needed for another route. Example message: Attempted to get the version_history Handler Class for Entity Type block_content however the class specified Drupal\Core\Entity\Controller\VersionHistoryController is not an instance of Drupal\Core\Entity\Controller\VersionHistoryControllerInterface.

Some notes, thoughts and ideas:

1. With the introduction of VersionHistoryControllerInterface I'm tempted to say we don't really need RevisionControllerTrait and it could just be merged into VersionHistoryController as I don't see any real benefit to having both, in most cases a module which needs to introduce some customisations would likely just override VersionHistoryController anyway.
2. I wonder if we can use _entity_form instead of _form for the revision_revert_form route?
3. Still need to add new routes suggested in #62.3.
4. Might be better to introduce a new Twig Template instead of using inline templating in VersionHistoryController and RevisionControllerTrait.
5. I wonder how close we can get VersionHistoryController and RevisionControllerTrait to the structure of EntityListBuilder and EntityListBuilderInterface, maybe even just extending those.
6. Might be able to improve VersionHistoryController::versionHistoryTitle to include the Entity Label, similar to what Node does.

Will continue working on the above, manual testing done using the block_content Entity Type. I included a patch of the Entity Annotation which can be applied to enable the features in the main patch. Automated tests will need to be updated although didn't have time today and it makes sense to wait until all blockers are resolved and this isn't postponed anymore, since test bot won't test it anyway while postponed.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryControllerInterface.php
   @@ -0,0 +1,34 @@
   \ No newline at end of file
   

   Missing newline.

2. +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -196,7 +196,7 @@ public function toUrl($rel = 'canonical', array $options = []) {
   -    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   +    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   

   The patch over at #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → has looser logic and checks any link that starts with 'revision':

   > However, entity annotations can provide other revision-specific routes, such as "revision_revert" or "revision_delete"

   Should that apply here too?

3. +++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
   @@ -0,0 +1,168 @@
   +class RevisionHtmlRouteProvider implements EntityRouteProviderInterface {
   

   Ignore, I was reading the code incorrectly!

4. /**
    * Provides local tasks for the revision version_history route.
    */
   class VersionHistoryDeriver extends DeriverBase implements ContainerDeriverInterface {
   

   I don't want to derail this issue, but it would be nice if the changes here that add derived links for entities were in tandem with 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work .

   Though given my comment above on the hierarchy of route providers, I maybe need to rethink 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work to allow for multiple links providers, rather than just one.

Here's a reroll of #77 for 8.6.x (and possibly prior versions too), in case it's useful to anyone else.

Thanks for the input Joachim, regarding your points in #78:

1. Thanks for pointing that out, I've added the new line to the end of VersionHistoryControllerInterface on my local branh, so when I submit my next patch that'll be addressed. (Hopefully I'll have time this coming weekend to continue on this)

2. Yeah I think it's best to just address this over on #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → , I'm happy for this issue to also be postponed on that unless there's a strong feeling that we shouldn't take that approach, but let's continue any further discussion over on that issue and come to an agreement there.

4. Yeah I'm aware of 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work and I fully support introducing these new Entity Link Handlers. I think it definitely makes sense to consider it here, but I don't know if it's totally necessary to block this issue on that one since VersionHistoryDeriver still functions properly. What I will do though is add a note and @todo to VersionHistoryDeriver saying that it is a temporary workaround until #2976861 is in. That way if #2976861 does land before this then we can simply swap it out here, otherwise we can just address in a follow-up and update the @todo with a link to the follow-up issue. Again doing that locally for now but will include it with my next patch.

Many thanks,
-Aaron

> What I will do though is add a note and @todo to VersionHistoryDeriver saying that it is a temporary workaround until #2976861 is in.

That sounds good to me. Thanks!

A bit more feedback on this patch:

1. * Gets the name of the handler class specified and performs compliance checks to make sure
   * it is a valid instance.

   Line too long.

2.   return $handler_class;
   } else {
     throw new \RuntimeException("Attempted to get the $handler Handler Class for Entity Type {$entity_type->id()} however the class specified $handler_class is not an instance of $implementing_class");
   

   coddled else

3. private function getHandlerClassWithImplementationChecks(EntityTypeInterface $entity_type, $handler, $implementing_class) {

   I'm confused by why we need this helper method. There's perhaps a case to be made for all entity handlers to get checked against the interface they ought to be implementing. But that's a wider issue. (And also one that leads to other issues to with declaring handler types in a formal manner, so we can declare the interface they need to have.)

4. if ($entity_type->hasLinkTemplate('version-history') && $version_history_class = $this->getHandlerClassWithImplementationChecks($entity_type, 'version_history', VersionHistoryControllerInterface::class)) {

   This pattern isn't used by the other entity route providers -- they hardcode the controller class they use for their routes. I think it's best not to invent too many new patterns at once.

   There's a case to be made in a follow-up for introducing a "controllers" annotation that mirrors the "route_provider" annotation, as I for one have often subclasses route providers just to switch the controller class set in route callbacks.

5. $revert_form_class = $entity_type->getFormClass('revision_revert');
   if ($entity_type->hasLinkTemplate('revision-revert-form') && $revert_form_class != NULL) {

   I've found with my own work with controllers that it's good DX to throw exceptions for this sort of thing rather than fail silently. Tell the developer that they have their entity type annotation set up incorrectly when they do 'drush cr' rather than when they load pages.

   I don't think there is a use case for there not being a revision revert route, since without the form class defined, the page at ENTITYTYPE/x/revisions crashes because it can't find the 'entity.ENTITYTYPE.revision_revert_form' route.

   So, throw an exception if the form class is missing. The same sort of thing probably applies to the other routes, but this is the one that's bitten me first!

6. Also, I'm not seeing a test entity type with a 'revision_revert' form class defined, so it looks like the test coverage is incomplete.

7. public function access(Route $route, AccountInterface $account, RouteMatchInterface $route_match = NULL) {

   Rather than inheritdoc, follow the pattern of EntityAccessCheck and document what this access checker expects in the route.

8. protected function checkAccess(ContentEntityInterface $entity, AccountInterface $account, $operation = 'view') {

   Missing docs.

9. $operation = $route->getRequirement('_entity_access_revision');

   AFAIK, an access checker needs to be declared as a service, with the requirements key, '_entity_access_revision' in our case, also declared in services.yml. E.g.:

   access_check.entity_revision:
     class: Drupal\Core\Entity\EntityRevisionAccessCheck
     arguments: ['@entity_type.manager', '@current_route_match']
     tags:
       - { name: access_check, applies_to: _entity_access_revision }
   

   Without this, I get an access denied on the routes.

   Though, why do we need @current_route_match as an injection?

10. if (empty($route_match)) {
      $route_match = $this->routeMatch;
    }

    Why do we need this? The access() method gets it as a parameter.

11. * @return string
     *   Returns a string to provide the details of the revision.
     */
    abstract protected function getRevisionDescription(ContentEntityInterface $revision, $is_current = FALSE);

    but VersionHistoryController::getRevisionDescription() doesn't return a string but an array.

A few more things:

1. I don't understand the purpose of RevisionControllerTrait.

ignore, my frankencoding was to blame.

ignore, this was because I had older revisions of the entity that predated the addition of the revision_created field, and so the value was NULL.

Would love to see this thing fixed. Crossing my fingers for you and all the others to see this soon. Commenting just to be able to see when it's ready with ease.

Thanks for the afford.

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Thanks for all your work on this!

Inside of Drupal\Core\Entity\Controller\RevisionControllerTrait::revisionOverview() at this point:

        if ($revision->isDefaultRevision()) {
          $row[] = [
            'data' => [
              '#prefix' => '<em>',
              '#markup' => $this->t('Current revision'),
              '#suffix' => '</em>',
            ],
          ];
          foreach ($row as &$current) {
            $current['class'] = ['revision-current'];
          }
        }
        else {
          $links = $this->getOperationLinks($revision);
          $row[] = [
            'data' => [
              '#type' => 'operations',
              '#links' => $links,
            ],
          ];
        }

You are opting to print links if it's not the current revision, but I could see a case where you'd want to copy the current revision to be the latest if you have multiple drafts ahead of your current revision.

If for example we start with the following revisions:

• 4 (Draft)
• 3 (Draft)
• 2 (Current/Default/Published)
• 1

and I need to make an update to 2, I could copy it to be the next revision in the sequence:

• 5 (Copy of 2 - Current/Default/Published)
• 4 (Draft)
• 3 (Draft)
• 2
• 1

I could make some edits and publish:

• 6 (Current/Default/Published)
• 5 (Copy of 2)
• 4 (Draft)
• 3 (Draft)
• 2
• 1

I could then continue work where I left off:

• 7 (Copy of 4 - Draft)
• 6 (Current/Default/Published)
• 5 (Copy of 2)
• 4 (Draft)
• 3 (Draft)
• 2
• 1

This is actually currently limited in node revision UI and the update action is explicitly blocked in _access_node_revision: 'update', which means if you create new drafts, and decide to create a draft based on what's currently published, you'd have to delete each draft in front of the currently published one. I believe you've already fixed this issue in _access_entity_revision, but the overview form is still hiding the action.

One other thought is we might consider changing the 'Revert' naming, something like 'Copy as Latest Version' would match both the 'Latest Version' tab and the 'Copy of the revision from ...' log message. There is some additional discussion about that here: #2899719: Revision/version language on revision listing page is misleading with content moderation enabled → .

Thanks again, this would be a significant improvement.

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

This is particularly important for media entities. The media bundles configured in Standard profile have a minimal set of fields, but there are lots of use cases for additional fields. For example:

• License, copyright, or access rights, etc.
• Details of how/when a recording was made.
• Participants or subjects of a recording.
• Various fields relating to learning management systems.
• Transcripts of audio/video media. These are something we need to support in core to address WCAG and ATAG time-based media accessibility criteria. Being addressed in 🌱 Provide authors with tools to manage transcripts and captions/subtitles for local video and audio Active
• Long descriptions for complex images

Pushing this issue along a bit. Made updates that do not require a great deal of additional discussion.

Remaining Tasks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -196,7 +196,7 @@ public function toUrl($rel = 'canonical', array $options = []) {
   -    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   +    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   

   The patch over at #2927077: $entity->toUrl('revision-*') → should fill revision parameter on all applicable routes. has looser logic and checks any link that starts with 'revision':

   > However, entity annotations can provide other revision-specific routes, such as "revision_revert" or "revision_delete"

   Should that apply here too?

2. private function getHandlerClassWithImplementationChecks(EntityTypeInterface $entity_type, $handler, $implementing_class) {
   

   I'm confused by why we need this helper method. There's perhaps a case to be made for all entity handlers to get checked against the interface they ought to be implementing. But that's a wider issue. (And also one that leads to other issues to with declaring handler types in a formal manner, so we can declare the interface they need to have.)

3. if ($entity_type->hasLinkTemplate('version-history') && $version_history_class = $this->getHandlerClassWithImplementationChecks($entity_type, 'version_history', VersionHistoryControllerInterface::class)) {
   

   This pattern isn't used by the other entity route providers -- they hardcode the controller class they use for their routes. I think it's best not to invent too many new patterns at once.

   There's a case to be made in a follow-up for introducing a "controllers" annotation that mirrors the "route_provider" annotation, as I for one have often subclasses route providers just to switch the controller class set in route callbacks.

4. Also, I'm not seeing a test entity type with a 'revision_revert' form class defined, so it looks like the test coverage is incomplete.

5. public function access(Route $route, AccountInterface $account, RouteMatchInterface $route_match = NULL) {
   

   Rather than inheritdoc, follow the pattern of EntityAccessCheck and document what this access checker expects in the route.

6. protected function checkAccess(ContentEntityInterface $entity, AccountInterface $account, $operation = 'view') {
   

   Missing docs.

7. operation = $route->getRequirement('_entity_access_revision');
   

   AFAIK, an access checker needs to be declared as a service, with the requirements key, '_entity_access_revision' in our case, also declared in services.yml. E.g.:

   access_check.entity_revision:
     class: Drupal\Core\Entity\EntityRevisionAccessCheck
     arguments: ['@entity_type.manager', '@current_route_match']
     tags:
       - { name: access_check, applies_to: _entity_access_revision }
   

   Without this, I get an access denied on the routes.

   Though, why do we need @current_route_match as an injection?

8. if (empty($route_match)) {
     $route_match = $this->routeMatch;
   }
   

   Why do we need this? The access() method gets it as a parameter.

9. * @return string
    *   Returns a string to provide the details of the revision.
    */
   abstract protected function getRevisionDescription(ContentEntityInterface $revision, $is_current = FALSE);
   

   but VersionHistoryController::getRevisionDescription() doesn't return a string but an array.

Patch for #88 ✨ Implement a generic revision UI Fixed did not upload correctly. Reuploading.

The last submitted patch, 89: 2350939-89-cleanup-generic-revision-ui.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

1. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php
   @@ -0,0 +1,167 @@
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   ...
   +    $this->revision->save();
   

   I've found a bug with this when using https://www.drupal.org/project/block_content_revision_ui →

   The bug is around the EntityChangedConstraint and the fact that we're not updating the revision's changed time before saving it.

   If you look at NodeRevisionRevertForm it explicitly sets this when reverting. Without this, reverting to a Draft when an entity has a published revision causes the entity to be un-editable due to the EntityChangedConstraint triggering.

2. +++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
   @@ -0,0 +1,179 @@
   +use mysql_xdevapi\Exception;
   

   This needs to be removed and fixed.

3. +++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
   @@ -0,0 +1,179 @@
   +   * @param EntityTypeInterface $entity_type
   ...
   +   * @return Route|null
   

   Return and param object types should be fully namespaced, there are other places in the patch this needs fixing as well.

Here are some more observations.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
   @@ -0,0 +1,196 @@
   +      if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)->isRevisionTranslationAffected()) {
   

   What if the entity is not translatable? We need a fallback condition for this.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
   @@ -0,0 +1,163 @@
   +  public function renderVersionHistory(RouteMatchInterface $route_match) {
   +    return $this->revisionOverview($route_match->getParameter($route_match->getRouteObject()->getOption('entity_type_id')));
   

   Instead of route match why can't we just pass $entity here?

Working on feedback since 77.

HI all, I've recently been looking at the contrib Entity API → module, at least more than I have done in the past, which has a revision UI solution implemented, and as such will likely be widely used on many Drupal sites. Looking at the solution implemented in Entity API it looks like the approach taken here is broadly the same, and I think if we can maintain that similarity here, with an approach which is already in use and tested in the wild, then I think everyone wins, because it means the upgrade path is easy for modules.

Just wanted to throw that out there.

Thanks,
-Aaron

Updated summary with issues blocked on.

Properly reverted EntityBase changes in favor of #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. →

The last submitted patch, 97: 2350939-97-generic-revision-ui.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wooh! Really, nice work @dpi. The interdiff in #96 is bigger than actual patch.

+1 for removing VersionHistoryLocalTasks let's keep the patch as simple as possible.

It looks like this is fine, since its exactly what Node does.

I was getting run time error for a non-translatable entity with no langcode key. We already have tests for entity_test_rev but it has langcode key so I'm happy with current approach and tests.

Long review, actioned points, and progress:

General changes

• Some PHP syntax changes now that Drupal 9 is target.
• CS fixups: missing docblocks, inheritdoc where overriding method, removing errenous inheritdocs from constructors, \t() to $this->t() where applicable, short array syntax, inject when otherwise a method would pull from the global container.
• Style changes: use chaining where possible, early exit (instead of nesting).
• Changed usage of ContentEntityInterface to RevisionableInterface. Nothing here is strictly for content.
• Where a revision is expected, variable name changed to $revision (from $entity or $entity_revision).
• Where an revisionable entity is expected, but the exact revision is not meaningful, $entity is used.
• Using asserts as typehinting and sanity checking.
• Removed use of "earlier" and "older", since revisions are not necessarily chronological, and there may be forward revisions.
• Updated interface docs so they dont describe implementation.

RevisionControllerTrait

• Consolidated hasRevertRevisionAccess, buildRevertRevisionLink, hasDeleteRevisionAccess, buildDeleteRevisionLink since access and its cacheability are combined concerns. hasDeleteRevisionAccess and hasRevertRevisionAccess were making use of undefined permissions. This issue is already postponed on figuring out an access control solution in #3043321: Use generic access API for node and media revision UI → . Ive switched out these permissions for the operation based approach there.
• This should be pulled into Node, or removed (Node switches to VersionHistoryController).

• +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
  @@ -0,0 +1,196 @@
  abstract protected function getRevisionDescription(ContentEntityInterface $revision, $is_current = FALSE);
  

Removed $is_current, can already be pulled from $revision->isDefaultRevision(). The parameter itself is currently unused in the implementation.

• +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
  @@ -0,0 +1,196 @@
  @todo Follow pattern in <a href="https://www.drupal.org/project/drupal/issues/2899719">https://www.drupal.org/project/drupal/issues/2899719</a>.
  

Removed todo since it doesnt add value. This patch doesnt depend on it.

• +++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
  @@ -0,0 +1,196 @@
  $build['#cache']['max-age'] = 0;
  

Added dependency on current language and entity.

VersionHistoryController

• Reordered methods in order of public, then protected.
• Optimised getRevisionDescription to build $context optimising for readability.
• Other changes are outlined in General above.

VersionHistoryControllerInterface

This pattern isn't used by the other entity route providers -- they hardcode the controller class they use for their routes. I think it's best not to invent too many new patterns at once.

Agreed. There is only a loose contract between route provider and the controller. No need for an interface. In the least we dont need to be inventing this here.

Removed handler logic and interface.

EntityBase

We could possibly do #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. ahead of this issue if we wanted to reduce the scope here.

• +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
  @@ -197,7 +197,7 @@ public function toUrl($rel = 'canonical', array $options = []) {
  if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
  if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
  

Reverted this change. This patch is blocked by #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → .

EntityRevisionAccessCheck

• Added @todo to remove this class since this issue is blocked on #3043321: Use generic access API for node and media revision UI → .
• Removed unused countDefaultLanguageRevisions().
• Removed unused resetAccessCache().

RevisionRevertForm

• Switched to implement EntityFormInterface so the class can be used with _entity_form.
  I tried using EntityConfirmFormBase but that only works with config, we want to be compatible with content and config.
• Changed logger channel to entity type the same way as ContentEntityDeleteForm and others do it, instead of using Node's content channel.
• Removed unused Request variable.

• +++ b/core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php
  @@ -0,0 +1,167 @@
  // The revision timestamp will be updated when the revision is saved. Keep
  // the original one for the confirmation message.
  

Fixed errant placement of comment (copied from node).

VersionHistoryLocalTasks

• Changed class name from VersionHistoryDeriver to VersionHistoryLocalTasks to be in line with others in core.

• +++ b/core/lib/Drupal/Core/Entity/Plugin/Derivative/VersionHistoryDeriver.php
  @@ -0,0 +1,80 @@
  $this->derivatives["entity.$entity_type_id.version_history"] = [
  

Changed ID, 'entity.' part is not necessary. Same as other core implementations.

• +++ b/core/lib/Drupal/Core/Entity/Plugin/Derivative/VersionHistoryDeriver.php
  @@ -0,0 +1,80 @@
  'title' => t('Revisions'),
  'base_route' => "entity.$entity_type_id.canonical",
  'weight' => 20,
  

Moved static values to the base plugin definition in system.links.yml

• +++ b/core/lib/Drupal/Core/Entity/Plugin/Derivative/VersionHistoryDeriver.php
  @@ -0,0 +1,80 @@
  // two "Revisions" tabs for Entity Types which already define their own revision routes and local task
  if ($version_history_route == NULL || $version_history_route->getOption('add_core_version_history_local_task') == NULL) {
  

A workaround was added to the local task deriver, passing around a add_core_version_history_local_task option in the route: primarily to workaround Node. We shouldnt have to support this kind of thing long term.

Ive added back the workaround for Node. Integrating Node into this patch remains a todo for this issue.

Personally I'm not a fan of this deriver, some thoughts on direction:

1. Remove Node local task definition, and ultimately provide it with this deriver.
2. Dont automatically create local tasks with a deriver. Entities should provide their own tasks, just as they already have to manually opt into functionality here with revision templates and form classes. Thusly remove this local task deriver and each entity type adds their own in a each links.tasks.yml
3. Further to 2, implement this later as a link handler in 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work .

✨ Implement a generic revision UI Fixed point 4 also has opinions about the deriver.

RevisionAdminHtmlRouteProvider

This one is a little odd, Entities are supposed to have the option of using this one instead of RevisionHtmlRouteProvider? The only difference being admin theme?

I think if entities want to override this they can subclass RevisionHtmlRouteProvider themselves and add the _admin_route option.

Removing this customisation and defaulting to _admin_route=false since thats how Node is doing. Though node has a site builder option to change this.

RevisionHtmlRouteProvider

• Switched from _form to _entity_form for revert form.

I've found with my own work with controllers that it's good DX to throw exceptions for this sort of thing rather than fail silently. Tell the developer that they have their entity type annotation set up incorrectly when they do 'drush cr' rather than when they load pages.

Another case of we dont need to invent that here: "if a form handler class doesnt exist then an exception should be thrown". This should be a new issue if a special exception should be thrown. Right now, a InvalidPluginDefinitionException is thrown if a user attempts to access the route when the form class does not exist.

+++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
@@ -0,0 +1,179 @@
+use mysql_xdevapi\Exception;

Removed errant import.

Addressing other queries from above:

Made updates that do not require a great deal of additional discussion.

Many of these were changes to typehinting in docs from fully qualified to just the symbol name. This is not consistent with coding standards. Reverted.

@jibran What if the entity is not translatable? We need a fallback condition for this.

It looks like this is fine, since its exactly what Node does.

Copied docs over from Node.

Instead of route match why can't we just pass $entity here?

Parameter name is different per entity type so I dont think we cant resolve that here.

1. I don't understand the purpose of RevisionControllerTrait.

Probably so \Drupal\node\Controller\NodeController::revisionOverview can pull from it without using VersionHistoryController. I think we can get remove the trait if Node switches to the new VersionHistoryController

The bug is around the EntityChangedConstraint and the fact that we're not updating the revision's changed time before saving it.

Added setting time on revert.

Todos

• buildDeleteRevisionLink is implemented, but revision-delete-form is not implemented in this patch yet.
• This issue is PP2. Blocked on both: #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → #3043321: Use generic access API for node and media revision UI → . The patch also relies on the patch in each of these.
• Swap Node over to use this generic implementation, or in the least the trait. (especially to fix local task workaround)
• Tests covering revert and delete link templates.
• Tests generally need to be cleaned up/modernised.
• Figure out what to do about local tasks: Generate them automatically or force each entity type to add their own. (see above)
• We should change the term "Revert" to something that doesnt imply changing revision to somethign in the past. E.g synonymous with Git's checkout. This term is also problematic because of #3023194: [PP-2] Add parallel revisioning support → , and when switching to other versions when Content Moderation is in use.

97 test fails are anticipated since it relies on patch in #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. →

For those keeping track, Block Content Revision UI → is updated to use the latest patch.

+++ b/core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php
@@ -0,0 +1,300 @@
+    if ($this->revision instanceof EntityChangedInterface) {

I think we should also check for RevisionLogInterface here and call setRevisionUserId and setRevisionCreationTime too?

Seems like we need a D8.9 patch as well.

Fatal error: Class Drupal\Core\Entity\Form\RevisionRevertForm contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Entity\EntityFormInterface::setEntityManager) in core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php on line 24

buildDeleteRevisionLink is implemented, but revision-delete-form is not implemented in this patch yet.

It can be a follow up.

This issue is PP2. Blocked on both: #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → #3043321: Use generic access API for node and media revision UI → . The patch also relies on the patch in each of these.

Is #3043321: Use generic access API for node and media revision UI → a hard blocker or can we provide a temp layer and remove it without a need for BC break?

Swap Node over to use this generic implementation, or in the least the trait. (especially to fix local task workaround)

Should be a follow up.

Updated issue summary of with items needing direction/decisions.

Here I have tried to fix failed tests of patch #97.

@ravi.shankar its not possible to fix the tests until the blockers are committed. Changes in your patch are not needed.

Fatal error: Class Drupal\Core\Entity\Form\RevisionRevertForm contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Entity\EntityFormInterface::setEntityManager) in core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php on line 24

Fixed Drupal 8 compat

-

Also fixed a minor CS issue.

The last submitted patch, 106: 2350939-106-generic-revision-ui.patch, failed testing. View results →

Addressing

Items needing discussion

I've also asked some other framework managers to chime in.

Whether to continue blocking this issue on #3043321: Implement a generic revision access API, an access checker will be added and immediately deprecated.

I think we need to postpone on that issue, providing an access check here is a convenience to those who want a single patch, but that's not how we do core development - so make that a blocker

Whether Node should use this in this issue.

Typically we need something that is using it in core for the patch to go in, otherwise it ends up getting out of date. However using it for node will require a lot of deprecation. So if we're going to add it for at least one entity-type here, then adding it to node perhaps should be a follow-up because the deprecations will be complicated. I'll ask some other framework managers.

Whether we should generate local tasks, or if entities should add their own.

If we don't do this, then how do users find the pages? Field UI generates it for entity-types so it feels like this is in the same vein. Speaking of Field UI, which can be turned off - would making this an experimental module that you could turn on/off help with velocity here? Would that even be possible? I guess not otherwise it would exist in contrib.

Revert terminology updated to be agnostic of target revision' time.
Should we adopt Version terminology (per patch)?

These are both questions for the UX team - can we get some screenshots here (tagging as such) and then when we have them, tag it for usability review and flag it with the UX team (#ux in slack).

Whether Node should use this in this issue.

I would suggest a follow-up patch for Node, but it would be good to see a working patch there prior to this one getting committed. That way we validate the API here covers Node, without doubling or tripling the patch size.

Whether we should generate local tasks, or if entities should add their own.

I think we should generate local tasks, the less that modules have to do when defining a new entity type the better.

Revert terminology updated to be agnostic of target revision' time.

We currently have 'Set as current revision' for forward revisions, and revert for older revisions. In Drupal 7 and earlier 'revert' was also shows for forward revisions when using contrib modules that created forward revisions. I think this should be a separate UX issue if we want to change it again.

Should we adopt Version terminology (per patch)

Is this in the UI or code? Or both? If it's in the UI it should be a separate UX issue again. In code I guess if we want to do that, we need to decide whether we're adding new code with 'version' and gradually updating the old code behind it - this goes all the way down to entity table naming.

One thing I will say for revision is that especially with draft revisions, it accurately describes tracking changes to something (whether or not each individual change is published). Also it doesn't conflict with software version. So at least in code, I'm not sure there's a compelling reason to change it at all and it might make things less clear, but for the interface we should open an issue.

Thank you for your replies.

Whether Node should use this in this issue.

Typically we need something that is using it in core for the patch to go in, otherwise it ends up getting out of date. However using it for node will require a lot of deprecation. So if we're going to add it for at least one entity-type here, then adding it to node perhaps should be a follow-up because the deprecations will be complicated. I'll ask some other framework managers.

Whether Node should use this in this issue.

I would suggest a follow-up patch for Node, but it would be good to see a working patch there prior to this one getting committed. That way we validate the API here covers Node, without doubling or tripling the patch size.

We can safely do media here and node in the follow-up but PP-3 node issue using this patch is a good idea.

#2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes. → is in

Thanks for moving this forward @larowlan @catch.

Whether we should generate local tasks, or if entities should add their own.

I think we should generate local tasks, the less that modules have to do when defining a new entity type the better.

If we don't do this, then how do users find the pages? Field UI generates it for entity-types so it feels like this is in the same vein.

Similarly to entity routing before route providers were a thing, modules would be required to provide their own .local_tasks.yml until 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work . The patch as is tries adds a deriver as an intermediate between these and would ultimately be deprecated.

We currently have 'Set as current revision' for forward revisions, and revert for older revisions. In Drupal 7 and earlier 'revert' was also shows for forward revisions when using contrib modules that created forward revisions. I think this should be a separate UX issue if we want to change it again.

Should we adopt Version terminology (per patch)

Is this in the UI or code? Or both? If it's in the UI it should be a separate UX issue again. In code I guess if we want to do that, we need to decide whether we're adding new code

We're creating a bunch of classes with both Revert and Version, arriving at a decision earlier would be preferable to changing symbols in the future.

Working on reworking and improving test coverage, removal of access checker, adding delete revision form.

Regarding Local Tasks, I agree3 with @larowlan and @catch, even if it's a temporary solution I think it's better to provide it, because:

• As a module developer who uses revisionable content entities, anything which lowers the amount of work I have to do to implement a functioning revision UI is a win.
• As already pointed out, Field UI already does this so there's president for doing this. Layout Builder also does this but that's slightly different because unlike the Field UI.* I don't see a problem with having it then deprecating it for removal in a follow-up once 📌 add an entity Links handler, to provide menu links/tasks/actions for entity types Needs work lands, it's not like one extra deprecation is going to add any extra work for a D10 release, I'm sure there will be many more deprecations to come before D10.

* Layout builder doesn't have to be opted-in by each entity type, any content entity can use it without any additional effort, which is why I see that use case as a little bit different.

Thanks,
-Aaron

Created 📌 [PP1] Switch Node revision UI to generic UI Needs work for switching Node over, immediately postponed.

Things like ✨ Provide users with the option to select an appropriate moderation state when reverting to a previous revision Needs work require the $form_state to be passed to the prepareRevision function. I've added that in this now for better compatibility later. NodeRevisionRevertForm also has this so would be required for 📌 [PP1] Switch Node revision UI to generic UI Needs work as well.

The last submitted patch, 117: 2350939-117-generic-revision-ui.patch, failed testing. View results →

Fixes the deprecation warnings. Not sure about the other failure, seems unrelated?

Trying to piece this together with a custom entity to try it out, and I can't extend the route provider.

As one of the ideas here is to reduce boilerplate, would it not make sense to have a RevisionHtmlRouteProviderTrait that is then used by the RevisionHtmlRouteProvider? Happy to have a go at adding one in.

@Darvanen take a look at Block Content Revision UI → for an up to date example.

This module uses alters, but entities themselves would aim to add these to their own annotation. Which of course is not trait-able.

Thanks @dpi :)

+++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
@@ -0,0 +1,172 @@
+      if ($revision->hasTranslation($currentLangcode) && $revision->getTranslation($currentLangcode)->isRevisionTranslationAffected()) {

I see that's already been addressed.

Actions #101, we still need functional tests for these forms.

We need to set the revision creation time after formatting the log and message. The original comment about the creation time being set when the revision is created is incorrect, in manual testing without calling setRevisionCreationTime the creation timestamp was still showing the old revision's time.

The last submitted patch, 124: 2350939-124.patch, failed testing. View results →

Working on this again.

Progress, and another tome...

Created a change record → , with up to date info on what is required of entity types that wish to make use of this new functionality. Test entities in the patch are a fully working implementation.

Contents of operations column largely is now decided based on route access and context.

Revision delete form added, functionality ported from Node implementation (NodeRevisionDeleteForm).

With this patch, we don't need to postpone on #3043321: Use generic access API for node and media revision UI → if we're happy with the operations approach. I’ve manage to put together the patch and tests without needing to add permissions. Test entities are making use of an established pattern of granting entity operation base on entity labels. For regular [non-test] entity types, all they need to do is implement an access controller responding to these standard revision operations, defer to permissions if they like. Or even override or replace the operations approach by extending the route provider.

On permissions

Entities are responsible for granting access based on operations. There is no automation for creating permissions, then deferring to the permissions based on operation. I don't think we should be forcing modules to implement permissions, whether automatically or manually. How access is determined for these new revision routes should be left up to the implementer. I've added simple examples to the change record → . Its a matter of adding a few lines responding to revision operations.

Technical changes:

• Brings across more forgotten functionality of Node revisions: revision page title for entities supporting logs.
• Added completely new tests. Covers most cases and scenarios. removed old [disabled] tests.
• Adds revision delete form.
• Since we're keeping local task generation, instead of adding a Node-opt out to core, inverted so Node opts itself out. (See node_local_tasks_alter. Tests added for this.
• A backwards compatibility shim added to EntityViewController::viewRevision to allow the revision page title route callback to work. Related 🐛 Node revision view page title no longer displays custom title with date Needs work .
• Revision access checker has been gutted, per above, now only restricts to revert/delete-revision routes to protect model against deleting/reverting default revision. Implementors may override or remove if they desire.
• Improved abstracting revision list so it can can be usefully extended, e.g buildRow, naming table rows, for revision table. This should hopefully avoid the need for projects like Diff → , and Node in the future, needing to fully replace instead of subclass/alter.

Further work

• I wonder if the RevisionControllerTrait is useful anymore: should its functionality be brought into VersionHistoryController.
• Unpostpone!
• Feedback on how translations fit in.

Screenshots

A cursory look through the interdiff is looking really good, extremely thorough tests, commentary and change record.

Very nice work @dpi!!

Really nice work with the patch. I have updated config_revision module to use the latest patch in 8751ff7. I followed the change notice and the changes were straight forward to implement. I ran into just one issue with the patch.

+++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
@@ -0,0 +1,183 @@
+      if ($revision->hasTranslation($currentLangcode) && $revision->getTranslation($currentLangcode)->isRevisionTranslationAffected()) {

This is returning false for non-translatable entities.

Re #129: looks like we need to bring #2951528: Empty results for revisions of entities without langcode → over into this patch.

Addressed #130.

We need to update IS after #127.

We’re gonna need tests for 131

Here is the test only patch. Turns out we are already testing it.
Test only patch is #127+interdiff.
The patch is #13+interdiff.

1. +++ b/core/modules/system/tests/modules/entity_test_revlog/src/Entity/EntityTestWithRevisionLog.php
   @@ -26,13 +26,13 @@
   + *   translatable = FALSE,
   
   @@ -63,7 +63,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setTranslatable(TRUE)
   

   These changes are not needed but I just made them to make it clear that this is not a translatable entity along with its fields.

2. +++ b/core/modules/system/tests/modules/entity_test_revlog/src/Entity/EntityTestWithRevisionLog.php
   @@ -26,13 +26,13 @@
   - *     "langcode" = "langcode",
   

   As per #2634230: Remove langcode entity key → , this was the only change needed to fail \Drupal\FunctionalTests\Entity\RevisionVersionHistoryTest::testOrder().

I don't think we provide an operation, form, or route for 'Revert to earlier revision of a translation' aka as node.revision_revert_translation_confirm in node.

The blocker is in

xjm and acbramley pointed out that I was confused with #111 :)

I've got a bit of a strange issue with this patch where a specific block isn't showing some revisions that have been reverted in the revision list.

Debugging into it, I found that $revision->getTranslation($currentLangcode)->isRevisionTranslationAffected() is returning NULL in RevisionControllerTrait::loadRevisions. Not too sure how this is happening as other blocks of the same bundle are fine going through the same process.

Please test and review, thanks!

I add to this issue the same patch, compatible with version 8.9.7 of the Drupal core.

Start testing.

The last submitted patch, 140: drupal_core-8.9.7-2350939-140.patch, failed testing. View results →

I add to this issue the same patch, compatible with version 8.9.7 of the Drupal core and compatible with version 7.0 of PHP.

@acbramley revisions that don't show under some conditions, especially in multilingual / translation contexts, is highly confusing. But beside the seemingly buggy behavior of isRevisionTranslationAffected() which is maybe a separate issue, we should maybe discuss improving the presentation and filtering of language specific revisions in a separate issue... i have the feeling that simply outputting all of them and applying a client side filter with the option "all" (in addition to specific languages) would sometimes be much more helpful...

@miro_dietiker yeah I ended up tracking it down to an edge case issue which was when reverting to a published revision that had the exact same field values as the currently published revision. I guess that particular flag is NULL when no field values have changed when saving a revision 🤷‍♂️

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

#143 misses setEntityManager() missing in RivisionRevertForm due to which there is a fatal error on reverting revisions.
just added the missing function in #143

Above patches 139/143/147 should be ignored as this issue is targeted for Drupal 9.

Hello shubhangi1995,
Thanks for the patch you proposed in comment 147, but it doesn't work on a Php 7.0 project:
To reproduce the error, you would need to be in version 7.0 of php and version 8.9.7 of the Drupal core, and to go to a url of the type "/block/{block_content}/revision/{block_content_revision}/revert", then you'll get the following error that will appear "The website encountered an unexpected error. Please try again later".

For this reason, I propose a new patch that I tested before on my local environment and including the same type of correction that shubhangi1995 made.

Translated with www.DeepL.com/Translator (free version)

To complement #96 ✨ Implement a generic revision UI Fixed (and, by the way, thank you so much for that 8.9.x patch → , it was very useful), I needed to add the following:

\core\lib\Drupal\Core\Entity\Form\RevisionRevertForm.php

Add at line 12:

use Drupal\Core\Entity\EntityManagerInterface;

Add at line 301:

	public function setEntityManager(EntityManagerInterface $entity_manager) {
		$this->entityManager = $entity_manager;
		return $this;
	}

After these additions I was able to use the Block Content Revision UI →

Thank you again.

Just as an addition, the Revision History page and revision form page aren't being displayed as Admin paths

\core\lib\Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider.php

in the different functions like

• getVersionHistoryRoute
• getRevisionViewRoute
• getRevisionRevertRoute

you can add the following line:

->setOption('_admin_route', TRUE)

to get all those paths as Admin paths.

Hi,

I added a few modifications to patch revisioning-2350939-96-generic-revision-ui.patch which was the one that allowed me to have all this working.

With my corrections I got it working ok, it took a lot of work to find out what was happening, especially without knowing the core in depth.

Anyways, feel free to test. I applied it to Drupal 8.9.12

cd siteRoot
git apply 0001-Patching-revisioning-for-all-entities-mixing-differe.patch

NOTE: this is to be used with Block Content Revision UI →

Good luck!

Reroll of #134 ✨ Implement a generic revision UI Fixed against 9.2.x-dev

Can we please have an issue summary update here, there's no record of what the missing item is

#154 rolled for 9.1.x

Here's the summary that got deleted around 15 October. I haven't edited it.

Fixed the formatting of issue links

Does the patch in #156 include blocks revisions UI? Are there additional steps needed for testing?

@smustgrave nope but you can use https://www.drupal.org/project/block_content_revision_ui → in combination with the patch :)

Thanks for pointing that out! Is there any plan for this ticket or another to implement a UI for blocks, taxonomy, and media revisions?

@smustgrave not in this issue, there will be follow-ups once this is merged.

+++ b/core/lib/Drupal/Core/Entity/Controller/RevisionControllerTrait.php
@@ -0,0 +1,184 @@
+  abstract protected function buildRevertRevisionLink(EntityInterface $revision): ?array;

+++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
@@ -0,0 +1,187 @@
+  protected function buildRevertRevisionLink(RevisionableInterface $revision): ?array {

Can you spot the difference?

Shouldn't the Change Record note that in order to see the "create new revision" checkbox and log message similar to Block Content and Node entities, a custom entity annotation should include:

/**
 * show_revision_ui = TRUE,
 *   route_provider = {
 *     ...
 *     "revision" = "Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider",
 *   }
 */

Otherwise, yes the CR enables the Revision tabs and local actions ... but a developer cannot create a new revision via the entity add/edit forms without additionally adding this annotation entry.

Re #165: while that is absolutely relevant, that does already exists in Core and wasn't implemented in this issue, thereby not something that has changed between the relevant versions, so I suspect it's probably out of scope in the context of the change record. I would imagine most custom entities which use revisions probably already include that?

@aaronmchale I am in the process of slowly learning D8/B9 development best-practices. I am making my first custom revisionable entity.

The official documentation for Drupal 8 does not show `show_revision_ui` anywhere in the Docs:
* https://www.drupal.org/docs/8/api/entity-api/making-an-entity-revisionable →
* https://www.drupal.org/docs/drupal-apis/entity-api/converting-a-content-... →

I was able to piece together information from this issue, and the Block Content Revision UI, and Linky Revision UI modules as examples of what to implement for this functionality along with the CR. But all these documentation references omit `show_revision_ui` usage for enabling this aspect of the revision ui for ones custom entity. I eventually looked at core Block Content and Node and saw the `show_revision_ui` annotation was included there.

The CR appears to be documentation showing how to enable a complete revision UI for a custom entity, and while this is a legacy annotation item it would seem like including this annotation within the CR would benefit other newbie D8/D9 developers since there is no prior complete documentation on how to achieve showing a "complete" revision UI for revisionable entities.

If nothing else hopefully my comments will help others who are also fumbling around in the dark. Thanks to all for your efforts.

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

@tenken Yeah that's a great example of where the documentation hasn't evolved much since the early days of D8, despite the Entity API continuing to evolve, definitely something that needs addressing.

#2612222: Provide a better way to alter entity revision overview → will require changes here if it gets in first.

Rerolled on top of #3043321-78: Use generic access API for node and media revision UI → .

The code in checkAccess remains slightly different for node and media but both are largely similar. Isn't there a way we can put that in a method on the base class and call it from both node and media's access control handler's checkAccess(). I'm thinking that every contrib module wanting to support revisions will end up copy-pasting that large block of code and we're back at square one regarding code duplication.

Isn't there a way we can put that in a method on the base class and call it from both node and media's access control handler's checkAccess(). I'm thinking that every contrib module wanting to support revisions will end up copy-pasting that large block of code and we're back at square one regarding code duplication.

That's a great idea for a follow-up :)

We are still on 9.2 which just released today. Unfortunately the 9.2 patch no longer seems to apply.

I tried to do a reroll, but I need clarification on the relationship between this issue and #3043321: Use generic access API for node and media revision UI → -- are patches on this issue meant to be applied "after" applying a patch from #3043321? -- that's how I interpreted the comment on #171, but I'm not sure if I understood correctly.

If "yes", we'll need to use a newer version of the patch on #3043321 -- but, they seem to be actively discussing how to proceed over there, so idk if that means we need to wait, or if we should just use the latest patch ( #98 → ), or...

(And regarding 9.2.x, I think it gets even muddier -- but I'm not going to ramble in that direction until I know more.)

ANYWAY I would be happy to help with a reroll, if someone doesn't beat me to it, once I know a little more.

This patch should work with 9.2.0 if anyone needs it.

#177 seems to be a re-roll of #171 which also includes #3043321-78: Use generic access API for node and media revision UI → .

Here is a re-roll of #164 for 9.2.x and 9.3.x.

That was my mistake. #178 applies perfectly to 9.2
Thanks!

The last submitted patch, 178: 2350939-178.patch, failed testing. View results →

Fix deprecations

Create a MR from this issue with Dogit with command dogit git 2350939 . ">=8.8.0" -vvvrd -e 79 -e 152 -e 177.

Diff created by Gitlab matches patch above, other than index hash range metadata.

Patch #181 is no longer working for Drupal 9.2.4. Can this be revised please?

@inteeka-help what do you mean it's not working? We were running the patch on 9.2.4 and now on 9.2.5 and it works fine.

Please provide some more detail about your issues :)

I added a 9.2.x test at #181, the patch fails to apply for that.

@John Pitcairn it applies with fuzz so should be fine for local sites. Feel free to re-roll it if needed :)

Here is a rebuilt patch to apply cleanly to 9.2.x.

#3043321: Use generic access API for node and media revision UI → just got committed so this issue is now un-blocked!

My guess is it will need a re-roll, but I've queued #189 against 9.3.x just to see what happens.

Exciting! 🥳

The last screenshots here are from July 2020, posted by @dpi in #127. I think we should update the issue summary (tag already present) to include screenshots of the current state, to make it easier to start reviewing and contributing to this issue.

So cool to see that for the first time ever this is now within reach!

Not sure if it will be easier to reroll the latest patch and untangle it from the revision access issue or go back to 2350939-171-do-not-test.patch. Reviewing that patch a bit.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -251,6 +270,29 @@ public function title(RouteMatchInterface $route_match, EntityInterface $_entity
   +   *   The title for the entity revision view page.
   +   */
   +  public function revisionTitle(RouteMatchInterface $route_match, RevisionableInterface $_entity_revision): TranslatableMarkup {
   +    $revision = $this->doGetEntity($route_match, $_entity_revision);
   +    $titleArgs = ['%title' => $revision->label()];
   

   I'm not sure if doGetEntity() is really the right thing to use here? it just uses the first entity route parameter that it finds, but don't revision routes have both the default entity and the revision?

   With the explicit entity revision upcasting that we have now, wouldn't it be easier to move this to the new revision controller and directly use the upcasted object? we don't have to worry about BC then.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -121,12 +122,26 @@ public static function trustedCallbacks() {
       *   A render array.
       */
   -  public function viewRevision(EntityInterface $_entity_revision, $view_mode = 'full') {
   -    return $this->view($_entity_revision, $view_mode);
   +  public function viewRevision(EntityInterface $_entity_revision, $view_mode = 'full', RouteMatchInterface $routeMatch = NULL) {
   +    $page = $this->view($_entity_revision, $view_mode);
   +
   +    // Remove buildTitle pre-render added by view() if the route has a title
   +    // callback.
   +    if (isset($routeMatch) && ($route = $routeMatch->getRouteObject()) && $route->getDefault('_title_callback')) {
   +      foreach ($page['#pre_render'] ?? [] as $key => $preRender) {
   +        if (is_array($preRender) && (($preRender[0] ?? NULL) instanceof static) && (($preRender[1] ?? NULL) === 'buildTitle')) {
   +          unset($page['#pre_render'][$key]);
   +        }
   +      }
   +    }
   +
   +    return $page;
      }
   

   not really happy about this, but will need to think more about a better solution.

   update: more thoughts after seeing more of this patch.

   I'd consider adding a new method and deprecate this one entirely. Then we could just always do that buildTitle() thing. I'm also not sure if it really makes sense to reuse view(). is it really worth adding those html head links on revision pages? If not, then we'd add almost more code (and certainly more comlex code) to remove this property than copying the method without that section. And even if we want to keep it, we could have that part in a helper method.

3. +++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
   @@ -0,0 +1,187 @@
   + *
   + * This controller leverages the revision controller trait, which is agnostic to
   + * any entity type, by using \Drupal\Core\Entity\RevisionLogInterface.
   + */
   +class VersionHistoryController extends ControllerBase {
   

   I'm wondering if we should try and build this to be more flexible out of the box?

   For example, there's the fairly common diff module that replaces this to make it a form that can be submitted. So right now, it basically has to duplicate all of it again, risking that thinks break if core changes.

   that would mean defining this as a form without actually having any form elements, and maybe some hooks.

   Not sure, just thinking out loud.

4. +++ b/core/lib/Drupal/Core/Entity/Routing/RevisionHtmlRouteProvider.php
   @@ -0,0 +1,172 @@
   +
   +    if ($version_history_route = $this->getVersionHistoryRoute($entity_type)) {
   +      $collection->add("entity.$entityTypeId.version_history", $version_history_route);
   +    }
   +
   +    if ($revision_view_route = $this->getRevisionViewRoute($entity_type)) {
   +      $collection->add("entity.$entityTypeId.revision", $revision_view_route);
   +    }
   

   why does one route use version history and 3 use revision something? shouldn't that be consistent?

5. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
   @@ -73,7 +73,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
          ->setTranslatable(TRUE)
   -      ->setSetting('max_length', 32)
   +      ->setSetting('max_length', 64)
          ->setDisplayOptions('view', [
            'label' => 'hidden',
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php
   @@ -56,6 +62,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    
        $fields['name']->setRevisionable(TRUE);
   +    $fields['name']->setSetting('max_length', 64);
        $fields['user_id']->setRevisionable(TRUE);
   

   I'm not sure I understand why this is necessary, but it has probably been discussed above.

   but certainly at least the second change is unnecessary if the parent is changed?

6. +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php
   @@ -65,6 +65,26 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +    elseif ($operation === 'revert') {
   +      // Disallow deleting latest and current revision.
   +      return AccessResult::allowedIf(!$entity->isDefaultRevision() && !$entity->isLatestRevision() && in_array('revert', $labels, TRUE));
   +    }
   +    elseif ($operation === 'delete revision') {
   +      // Disallow reverting to latest (a pointless exercise).
   +      return AccessResult::allowedIf(!$entity->isLatestRevision() && in_array('delete revision', $labels, TRUE));
   +
   +    }
   

   shouldn't these things be covered by default somehow?

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -77,14 +88,21 @@ class EntityController implements ContainerInjectionInterface {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $date_formatter argument is deprecated in drupal:9.1.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3160537', E_USER_DEPRECATED);
   

   this needs updating to 9.4.0 and 11.0.0 now

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -251,6 +270,29 @@ public function title(RouteMatchInterface $route_match, EntityInterface $_entity
   +    else {
   

   this else isn't needed, we returned above

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -345,4 +387,14 @@ protected function loadBundleDescriptions(array $bundles, EntityTypeInterface $b
   +  final protected function dateFormatter(): DateFormatterInterface {
   +    return $this->dateFormatter;
   

   why is this method needed?

4. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -121,12 +122,26 @@ public static function trustedCallbacks() {
   +    $page = $this->view($_entity_revision, $view_mode);
   +
   +    // Remove buildTitle pre-render added by view() if the route has a title
   +    // callback.
   +    if (isset($routeMatch) && ($route = $routeMatch->getRouteObject()) && $route->getDefault('_title_callback')) {
   +      foreach ($page['#pre_render'] ?? [] as $key => $preRender) {
   +        if (is_array($preRender) && (($preRender[0] ?? NULL) instanceof static) && (($preRender[1] ?? NULL) === 'buildTitle')) {
   +          unset($page['#pre_render'][$key]);
   +        }
   +      }
   +    }
   

   would it not be simpler to add a new optional argument to ::view $add_title or similar instead of this juggling dance?

5. +++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
   @@ -0,0 +1,187 @@
   +      $username = [
   +        '#theme' => 'username',
   +        '#account' => $revision->getRevisionUser(),
   +      ];
   +      $context['username'] = $this->renderer->render($username);
   

   are we missing collecting cacheability metadata from the user here

Now the generic access issue is in, this needs a reroll

Here's a plain reroll of 171-do-not-test and an interdiff.

The parts shown in the interdiff are where the patch wouldn't apply and I manually edited in the rejected hunks.

Leaving on NR so we can see how the tests go but true status is NW based on #192 and #193.

Oh. The interdiff patch utility also rejected this hunk when making the interdiff, which was the only other place I had to make an edit to recreate the patch:

***************
*** 75,81 ****
      $entity_test_rev->setNewRevision(TRUE);
      $entity_test_rev->isDefaultRevision(TRUE);
      $entity_test_rev->save();
-     $this->drupalGet('entity_test_rev/' . $entity_test_rev->id() . '/revision/' . $entity_test_rev->revision_id->value . '/view');
      $this->assertRaw($entity_test_rev->label());
      $this->assertRaw($get_label_markup($entity_test_rev->label()));
  
--- 75,81 ----
      $entity_test_rev->setNewRevision(TRUE);
      $entity_test_rev->isDefaultRevision(TRUE);
      $entity_test_rev->save();
+     $this->drupalGet($entity_test_rev->toUrl('revision'));
      $this->assertRaw($entity_test_rev->label());
      $this->assertRaw($get_label_markup($entity_test_rev->label()));
  

And it looks like I left some .orig files in there... working on removing those now.

I think interdiff is quitting when it throws an error, so take that file with a grain of salt. I'm not sure how else to approach it.

Also I think I can make this NW and queue the test manually, going to try that.

@darvanen see https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... → for why you can't create an interdiff from a reroll

@mstrelan thanks!

In that case, per the recommendation on that page, here's a plain diff of the two patches.

@darvanen is this ready for another review?

No, sorry, it was just a painful re-roll. I didn’t have time to address the feedback.

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Fixes #192.5 and #193.1-4

The rest of #192 seems kinda up in the air so not really sure what to action and what not to. #193.5 - I'm unsure about this since it's rendering a theme hook, shouldn't that theme hook add the cacheability? Also not sure if it's possible to bubble cacheability from a single cell in a table (probably does work).

This seems to work for me. Can this be marked reviewed?

Actually question when it comes to views. Nodes have "Get the actual content from a content revision" filter. Should this be made generic too or per entity?

#205 sounds like a good follow-up to me.

Agree with #205 and #206, sounds like it would be worth exploring, but definitely needs its own follow-up issue, as the scope of this issue is already quite wide.

Sounds good. Then the latest patch with https://www.drupal.org/project/media_revisions_ui → is working great for me. Would say this can be marked as reviewed

Not a comprehensive review but found a few things. I'm not sure what to do about the 'revert' language stuff, on the one hand UI issues shouldn't block this, but on the other it's turning one-off UI terminology into an API with the same language, so making the problem worse.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -77,14 +88,21 @@ class EntityController implements ContainerInjectionInterface {
        $this->urlGenerator = $url_generator;
   +    if (!$date_formatter) {
   +      @trigger_error('Calling ' . __METHOD__ . ' without the $date_formatter argument is deprecated in drupal:9.4.0 and will be required in drupal:11.0.0. See https://www.drupal.org/node/3160537', E_USER_DEPRECATED);
   +      $date_formatter = \Drupal::service('date.formatter');
   +    }
   

   Could probably just be for Drupal 10, especially since it's a constructor deprecation.

2. +++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
   @@ -0,0 +1,187 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function buildRevertRevisionLink(RevisionableInterface $revision): ?array {
   +    if (!$revision->hasLinkTemplate('revision-revert-form')) {
   +      return NULL;
   +    }
   +
   +    $url = $revision->toUrl('revision-revert-form');
   +    // Merge in cacheability after
   +    // https://www.drupal.org/project/drupal/issues/2473873.
   +    if (!$url->access()) {
   +      return NULL;
   +    }
   +
   +    return [
   +      'title' => $this->t('Revert'),
   +      'url' => $url,
   +    ];
   +  }
   +
   

   Don't want to derail this, however 'revert' has bothered me in this UI for years, since what this actually does is 'clone and publish' the revision it refers to. Reverting would be going back to before this change, which is not what happens at all.

3. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionDeleteForm.php
   @@ -0,0 +1,315 @@
   +
   +    // When there is more than one remaining revision, redirect to the version
   +    // history page.
   +    if ($this->revision->hasLinkTemplate('version-history')) {
   +      $query = $this->entityTypeManager->getStorage($entityTypeId)->getQuery();
   +      $remainingRevisions = $query
   +        ->accessCheck(FALSE)
   +        ->allRevisions()
   +        ->condition($this->revision->getEntityType()->getKey('id'), $this->revision->id())
   +        ->count()
   +        ->execute();
   +      $versionHistoryUrl = $this->revision->toUrl('version-history');
   +      if ($remainingRevisions > 1 && $versionHistoryUrl->access($this->currentUser())) {
   +        $form_state->setRedirectUrl($versionHistoryUrl);
   +      }
   +    }
   +
   +    if (!$form_state->getRedirect()) {
   +      $canonicalUrl = $this->revision->toUrl();
   +      if ($canonicalUrl->access($this->currentUser())) {
   +        $form_state->setRedirectUrl($canonicalUrl);
   +      }
   

   Why not redirect to the revision page even if there's only one revision left? We show it now, so the redirect will work, saves some logic, and consistent behaviour every time.

4. +++ b/core/lib/Drupal/Core/Entity/Form/RevisionRevertForm.php
   @@ -0,0 +1,339 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getQuestion() {
   +    if ($this->getEntity() instanceof RevisionLogInterface) {
   +      return $this->t('Are you sure you want to revert to the revision from %revision-date?', ['%revision-date' => $this->dateFormatter->format($this->getEntity()->getRevisionCreationTime())]);
   +    }
   +    return $this->t('Are you sure you want to revert the revision?');
   +  }
   +
   +  /**
   

   As above - we're not reverting the revision, we're reverting to the revision (via a new copy of it).

FWIW I've always read 'revert' in this context as reverting the *state of the entity* to that contained in the revision... I may be in the minority, but I sense a bikeshedding coming on - though if it gives this feature a little momentum I won't complain :)

I agree with point #210.3

@darvanen I agree, I hope we can divert that discussion to a separate issue and get this across the line without the bikeshedding! :)

Yeah I think only #1 and #3 need to be dealt with here, will try to find an existing issue discussing 'revert'.

@catch there are quite a few issues that touch on various aspects of the terminology discussion, but the most relevant one I'm aware of is: #2899719: Revision/version language on revision listing page is misleading with content moderation enabled →

Also re #210.1, see #193.1 ✨ Implement a generic revision UI Fixed , might need to discuss with @larowlan.

Scrap that. I misread the quoted code due to Gmail highlighting.

@darvanen November was before we'd finalised exactly which deprecations we'd accept for the 9.4.x branch, but we decided to treat it as a normal minor release due to the sheer amount of blocking changes required for 10.0.x, so 10.0.x is the right branch to target for removal.

Tagging novice for #210.1, the rest of #210 we can address in #2899719: Revision/version language on revision listing page is misleading with content moderation enabled → , and also for 9.4.0 release highlights.

Could still use an issue summary update and screenshots too.

Just a few things that come to mind while scanning the patch:

1. I don't quite understand the reason for RevisionControllerTrait, practically it's only used once, in VersionHistoryController. Why not just merge it into VersionHistoryController, I can't see one being used without the other and the choice of what goes where seems to be somewhat arbitrary. Even if a contrib module (let's say Diff for example) was to provide its own revision UI, I suspect the logical thing to do would be to extend VersionHistoryController. So unless there's a specific reason for a separate trait, to me it would seem cleaner to just have everything in the one class.
2. I've added a comment at #3153559-17: [PP-2] Switch Node revision UI to generic UI → noting we will need to do something about the node_local_tasks_alter() hook implementation that this issue will add, just so we don't forget it's there.
3. We potentially still need a follow-up issue for @smustgrave suggestion in #205.

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

I had to +1 the issue with the "revert" terminology. Why not "restore"? To me, that communicates what "revert" was trying to but doesn't imply rolling back in time but rather bringing it back to the state it was in at that point in time. My $.02 :)

This is not actually in 9.4.0 so removing "9.4.0 release highlights".

I'll work on #219 and also #210.1 + #210.3 that were left.

I addressed the points from #219 (including creating a followup issue for #205 #3284730: Update node's view filter "Get the actual content from a content revision" → ) and the remaining points from #210.

Is there a good way to manually test this?

I've applied the patch and I'm testing out contrib modules that depend on it ( Media Revisions UI → and Block Content Revision UI → ). I could update the IS and add screenshots as noted in #218 ✨ Implement a generic revision UI Fixed once I have a clearer path forward.

While testing the most recent patch from #224 with existing contrib modules Media Revisions UI and Block Content Revision UI, I noticed that the Revert button was being added to the Current revision, so I added the check for default revision before the operations links are added to the table. I also updated the markup for Current revision; see Interdiff.

I originally noted this issue (with screenshots) in Block Content Revision UI 🐛 Should not be able to Revert the Current revision Fixed .

Here is a comparison screenshot using contrib Media Revisions UI dev-3.x:

+++ b/core/lib/Drupal/Core/Entity/Controller/VersionHistoryController.php
@@ -291,16 +291,22 @@ protected function buildRow(RevisionableInterface $revision): array {
-      $row['operations']['data']['status']['#markup'] = $this->t('<em>Current revision</em>');
+      $row['operations']['data']['status'] = [
+        '#prefix' => '<em>',
+        '#markup' => $this->t('Current revision'),
+        '#suffix' => '</em>',

This change isn't necessary. It's fine to have simple HTML tags inside the t() call.

Thanks! I was trying to match it up with what's already in node, but it's tidier the way it was.

So here's the same patch with that one change removed with the interdiff against #224.

Is this patch supposed to supply actions on the revision UI? I'm not getting any with either #228 or #202.

This was taken while logged in as user 1:

@darvanen The action buttons should already be there out of the box; the patch doesn't add them. Can you provide steps to reproduce? For example, what entity are you looking at? What core version? Do you see the same behavior on a fresh site with core Node entity?

My steps:
I spun up a site using the Drupalpod chrome extension, with the patch applied. Core version 9.5.0-dev. I created a node of type Article, edited the node, go to the Revisions UI page, and I'm seeing the button displayed on Revisions UI.

@capysara Node already has a revisions interface, the point of this feature is to make an interface available to any revisionable entity.

I am using custom entities that were generated some time ago. If anyone else lands here looking for the answer to my question: the link template names are "revision-revert-form" and "revision-delete-form", not "revision_revert" and "revision_delete".

Does that mean that your issue is fixed now with the template names update--you can see the actions in your UI?

If not, for a custom entity, you might take a look at the two contrib modules that are using this patch and see how they implement the UI.

Yes thank you, changing the link template names solved my issue

Rerolled from #228

Got when trying to apply
error: patch failed: core/modules/system/tests/src/Unit/Menu/SystemLocalTasksTest.php:44
error: core/modules/system/tests/src/Unit/Menu/SystemLocalTasksTest.php: patch does not apply

Also looking for a good way to test if anyone could provide steps.

Testing patch #228 with media_revision_ui 3.x branch

Installed media_revision_ui
Ensured image media bundle has revisions enabled
Created an image bundle and created several revisions
Verified the revisions tab and all revisions existed
Verified I was able to revert

Marking as RTBC if anyone disagrees please move back.

Thanks everyone for working on this long standing issue.

An issue summary was asked for in #218 and I see a minor change was made later. The are decision to be made that are not shown as resolved. For this issue it would help to have steps to reproduce in the Issue Summary. If that were present it could be tagged for manual testing as well and good some more feedback.

This is tagged as needing screenshots. While there are screenshots in the later comments it is easier for reviewers and committers to find the latest ones in the issue summary.

There is a deprecation message in the patch for 9.4 and removal in 10.0. That will probably change to deprecated in 10.1.x, but check with catch or xjm to be sure.

And, are the change records for this up to date?

I am removing the novice tag because I don't think updating the IS and testing to get screenshots is suitable.

I've been thinking this also needs a usability review. larowlan asked for one in #108 with some specific question. Those question were answered in the following comments which is great but I am assuming that a usability review looks at other things as well. Therefor adding the needs usability review tag.