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.

Here's a start

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.

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

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.

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.

Adding the related Diff module issue.

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

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

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.

Next try to fix the tests

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

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.

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

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. 🤦‍♂️

Patches including and after 64 need #2927077: $entity->toUrl('revision-*') should fill revision parameter on all applicable routes.

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 {
   

   Might be out of scope for this issue, but I have recently been thinking about how route provider classes have an increasingly complex hierarchy which is starting to get hard to work with. I notice that over at Entity API, the route provider for revisions is a completely separate class. Maybe we need to think about either: a) declaring multiple, separate route providers (which is already possible, it's just a matter of how we slice up the classes), or b) providing traits for entity type-specific route providers to then use to assemble their functionality. 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 #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types.

   Though given my comment above on the hierarchy of route providers, I maybe need to rethink #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types 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 #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types 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.

2. This code in VersionHistoryController crashes: ignore, my frankencoding was to blame.

3. getRevisionDescription() is crashing with: 'The timestamp must be numeric' 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.

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.

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 #3002770: Provide authors with tools to manage transcripts and captions/subtitles for local video and audio
• 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 did not upload correctly. Reuploading.

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.

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 #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types.

#2350939: Implement a generic revision UI 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.

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

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.

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.

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 #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types. 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 #2976861: add an entity Links handler, to provide menu links/tasks/actions for entity types 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 #3153559: [PP1] Switch Node revision UI to generic UI for switching Node over, immediately postponed.

Things like #2906568: Provide users with the option to select an appropriate moderation state when reverting to a previous revision 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 #3153559: [PP1] Switch Node revision UI to generic UI as well.

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

Next question:

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

This will fail to render revisions for any entities that do not implement a language. Is that deliberate? I see that's already been addressed.

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