Provide a better way to alter entity revision overview

Please check if this is a bug in or a feature needed for Drupal 8. If so, the issue must be opened in the Drupal 8 queue first according to the backport policy. If it is relevant to Drupal 8, move it to branch 8.0.x-dev and tag it “Needs backport to D7”.

There may also be an existing Drupal 8 issue. Try to find it. If one exists, tag it “Needs backport to D7”. If it is open, offer a patch. If its status is “Fixed”, make its status “Patch (to be ported)”, move it to version 7.x-dev and upload your latest patch if one exists.

Re-roll against 8.5.x-dev

Running into a scenario where we want to alter the overview page for revisions.

Tried to apply the latest patch posted here but it was getting rejected for some reason. I've basically just copied the patch but used newer array syntax. No real significant change but I'm having success applying this patch--all credit goes to the people above.

Hm, after attempting to run the apply the patch and then hook into it, I realized that $this->alter() was not implemented. Here is a patch that implements the hook. Then in a custom module, I was able to successfully hook into the alter hook and get the context & build of the revisions overview page.

I don't think I implemented that correctly. I am now passing $build as a reference so it can be edited in the hook. $context is still just only available as a value.

1. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -274,9 +276,26 @@ public function revisionOverview(NodeInterface $node) {
   +    $this->alter('node_revision_overview_alter', $build, $context);
   

   Let's anticipate #2350939: Implement a generic revision UI and use a hook called entity_something().

   The 'node' context key should be called 'entity'.

   Also -- new hooks need to be documented in an api.php file.

2. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -274,9 +276,26 @@ public function revisionOverview(NodeInterface $node) {
   +    $this->getModuleHandler()->invokeAll($callback, $args);
   

   This can use ModuleHandler::alter() here.

   Also, I'm not sure why we need a helper alter() method in this class. It's not doing very much at all.

3. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -274,9 +276,26 @@ public function revisionOverview(NodeInterface $node) {
   +    return $this->moduleHandler ?: \Drupal::moduleHandler();
   

   When would $this->moduleHandler be set?

   This can just be injected in create().

   According to BC policy, controller classes are internal, so it's fine for us to add a DI parameter.

Updated the IS for D9.
Made the changes from my last comment, and a few others too. In particular, one of the build array keys needs to be changed to be generic. This is allowed by our BC policy: https://www.drupal.org/about/core/policies/core-change-policies/drupal-8...

I've worked on this in parallel with #3212602: [PP-?] Show revision state on the revisions overview tab for moderated nodes/entities, so one thing I realised pretty quickly is that having an array of all revisions isn't enough, because the table doesn't necessarily show them all. Hook implementations need to also know which revisions are in the table.

MR coming.

If we need a sane way to alter, I'm curious about what this issue considers insane and needs changing. What are the solutions currently being used?

The patch currently has its own alter just for the controller. It would be impractical to add alters for every controller in Drupal.

Just throwing this out there, that there is a way already to alter any controller, which I don't think is completely insane:

// Class for a service with `tags: { name: 'event_subscriber' }`
class MyEventSubscriber implements \Symfony\Component\EventDispatcher\EventSubscriberInterface {

  /**
   * Alters controller responses.
   *
   * @param \Symfony\Component\HttpKernel\Event\ViewEvent $event
   *   The event to process.
   */
  public function onView(\Symfony\Component\HttpKernel\Event\ViewEvent $event) {
    $routeMatch = $this->routeMatch->getRouteMatchFromRequest($event->getRequest());
    $route = $routeMatch->getRouteObject();
    if (!$route) {
      return;
    }

    // Alter revision history page.
    if ($route->getDefault('_controller') === \Drupal\Core\Entity\Controller\VersionHistoryController::class . '::versionHistory') {
      $entityTypeId = $route->getOption('entity_type_id');
      $entity = $routeMatch->getParameter($entityTypeId);
      assert($entity instanceof EntityInterface);
      $result = $event->getControllerResult();

      // Do something...

      $event->setControllerResult($result);
    }
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::VIEW][] = ['onView', 1];
    return $events;
  }

}

This example is designed to alter the version history page introduced by #2350939: Implement a generic revision UI.

Consider this comment a vote against this issue.

If this patch goes forward, I think it would be better to wait for #2350939: Implement a generic revision UI and create a generic hook, as the patch here explicitly introduces a node hook. Something entity type agnostic should be introduced instead.

The problem with #26 is that it's fundamentally the same as D7 era hook_page_alter(): something is having to be run on *every* page load on the site.

> If this patch goes forward, I think it would be better to wait for #2350939: [PP-1] Implement a generic revision UI and create a generic hook

My MR changes the hook to being generic for entities.

A better way to alter the page is to alter the route controller and then extend the new controller from the core one.

The best way would be to just convert the page to a form and use form alter all day long.

GIven that one can always alter the route controller, I would say it's support request or a won't fix.

FWIW, adding a new hook to alter the HTML of the page is a no go, IMHO.

> A better way to alter the page is to alter the route controller and then extend the new controller from the core one.

I did consider that, but then it's a lot more boilerplate code for modules wanting to participate.

I agree with what @dpi said in #27.

I'd actually be in favour of just postponing this on #2350939: Implement a generic revision UI, that way once that's in we can find the most effective way to implement this generically. Since then it won't just be specific to the Node Entity Type, if Media, Block and other core entities start gaining revision UIs, the same usecase is going to crop up.

And +1 to exploring some kind of _alter hook, whether that be converting the revision overview to a form, or a new hook completely.

#2612222-29: Provide a better way to alter entity revision overview:

The best way would be to just convert the page to a form and use form alter all day long.

This could be a way to go. Contrib could benefit from the page being a form, such as Diff adding its to/from columns, or introducing entity revision operations. Obviously we would inherit all the form infrastructure, including form alter hooks.

Even if the form doesnt end up being cached, there is unlikely a lot a downside as these pages shouldnt receive a lot of traffic.

> And +1 to exploring some kind of _alter hook, whether that be converting the revision overview to a form, or a new hook completely.

That's what the MR does -- adds a new hook.

> Contrib could benefit from the page being a form, such as Diff adding its to/from columns, or introducing entity revision operations

A form would also allow a UI for deleting multiple past revisions.

> Even if the form doesnt end up being cached, there is unlikely a lot a downside as these pages shouldnt receive a lot of traffic.

It's an editor page; we don't worry as much about performance on pages that are only for editors and admins.

Ok, I am reasonably convinced on the form.

+1 to both #33 and #34.

Now that I think about it actually, would it make sense to just implement this as part of #2350939: Implement a generic revision UI rathern than in parallel or afterwords. After all, that issue is having to create the generic version history page, may as well just make that generic page a form to begin with, then in #3153559: Switch Node revision UI to generic UI we do whatever is needed to make it work for Node?

If we do that, that would in theory mean this issue could be closed.

Now that I think about it actually, would it make sense to just implement this as part of #2350939: [PP-1] Implement a generic revision UI rathern than in parallel or afterwords.

#2350939: Implement a generic revision UI is already doing a lot we have still to agree on the complete scope of it (see the IS over there) so adding this feature over there would be a big scope creep and could delay the patch even more which is already postponed an issue.

#2350939: [PP-1] Implement a generic revision UI is already doing a lot we have still to agree on the complete scope of it (see the IS over there) so adding this feature over there would be a big scope creep and could delay the patch even more which is already postponed an issue.

Yeah that’s a good point. Okay, let’s see how far this issue can get, if #2350939: Implement a generic revision UI gets in before this one, then it would make sense to switch the approach of this issue to the generic approach.

#2350939: Implement a generic revision UI is merged. Not sure what other issue we're blocked on, maybe #3153559: Switch Node revision UI to generic UI. But don't think it actually blocks this if its entity agnostic. So -2

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Reading the comments now that #2350939: Implement a generic revision UI landed think the approach agreed upon should be added to the issue summary.

I do see the MR is going by it from a generic standpoint but don't know what (if any) needs to change with the changes introduced by 2350939.

Separate MR for 10.1.x could probably be started.

Also tagging for tests but those could wait until the IS is there.

Since last commenting on this issue, I found other similar issues which need to solve the same problem, of allowing different modules to add or alter a generic page.

I filed #3228770: Allow decorating route controllers as a unified approach, and made a proof-of-concept contrib module.

Do you think this blocked by https://www.drupal.org/project/drupal/issues/3153559 ?

Since last commenting on this issue, I found other similar issues which need to solve the same problem, of allowing different modules to add or alter a generic page.

I filed #3228770: Allow decorating route controllers as a unified approach, and made a proof-of-concept contrib module.

We might still want a more specific solution here, because a lot of the use cases going forward for adapting the generic Version History Controller (e.g. what the Diff module does) would want to do this across all instances. So with something like a route decorator, you would need to decorate every entity.[entity_type].version_history route, and it would definitely require a programmatic approach of finding every route which uses the Version History Controller (as well as any controllers extending it), which would be less performant and practically speaking could be quite challenging to implement and account for all possible edge cases.

IMO this can be closed in favor of #3153559: Switch Node revision UI to generic UI. The generic revision UI (VersionHistoryController) provides many easy extension points for other modules. e.g the diff module will just subclass it and override something like buildRow

Setting to PMNMI to get any a consensus on whether we'd need any further extensibility.

I don't think that subclassing the version history controller is a good solution for this issue.

The Diff module is a great example. From the perspective of another contrib module which also needs to alter the revision overview, and in order to support both core's controller and Diff's override, it has to subclass both of them. A third one with similar requirements would bring chaos :)

Adding a hook as in the current MR is one way to do it, but I think converting that controller to a form would be more helpful in the long run, especially since Diff (which is a very popular module) already does that.