Media entity is revisionable but doesn't have a revision link template

Here is a first patch. I basically copied a lot of stuff from node. If this solves the issue we can also add the tests for it.
This makes me think we definitely need some base classes to not have to copy this for every revisionable entity type. I think it's best to do this in a followup.

#2862422: Add per-media type creation permissions for media is a related issue since we want revision permissions per type as well.

Here is a new patch containing just the revision page. Basically started over so skipped the interdiff. Also added followups:
#2887106: Add generic base classes for content entity revisions
#2887108: Use generic base classes for entity revisions in Node, Media and Block content

This should fix the tests.

Removed MediaStorage / MediaStorageInterface and added the methods to SqlContentEntityStorage / ContentEntityStorageInterface. The param of NodeStorage::countDefaultLanguageRevisions() is changed now, since it also extends ContentEntityStorageInterface. I didn't remove the method in NodeStorage yet, we should probably do that in #2887108: Use generic base classes for entity revisions in Node, Media and Block content.

Also added a bunch of tests. As discussed on IRC we should probably add base classes for the revision related tests as well in #2887106: Add generic base classes for content entity revisions. For now I think this is sufficient.

Removed some extra spaces in the patch.
That didn't go as planned. Let me wait for a review until I post another one.

Moved countDefaultLanguageRevisions to SqlEntityStorageInterface.

13.4 and 13.4 were needed to make sure the tests passed. For some reason I get a ajax JS error. I guess my machien takes a little longer than the testbot. In Drupal\Tests\media\FunctionalJavascript\MediaSourceImageTest we have $assert_session->assertWaitOnAjaxRequest() which serves the same purpose.

I could remove it and add another issue for it? But since it is in MediaRevisionTest we might as well fix it now.

The tests actually fail on those lines. Another good reason to remove it. This should be it then :)
I'll figure out what's going wrong on my local machine later...

This looks nice to me, but I do have some comments. Most prominently, MediaController should not be needed, as far as I can tell.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -809,6 +809,17 @@ public function save(EntityInterface $entity) {
   +    $query = $this->database->select($this->revisionDataTable);
   

   Should we throw an exception or something like that in case the entity is either not translatable or not revisionable (or both) in which case $this->revisionDataTable will not be set?

2. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
   @@ -0,0 +1,135 @@
   +    $map = [
   +      'view' => 'view all media revisions',
   +    ];
   

   Seems like unnecessary complexity?

3. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
   @@ -0,0 +1,135 @@
   +      // There should be at least two revisions. If the vid of the given media
   +      // item and the vid of the default revision differ, then we already have
   +      // two different revisions so there is no need for a separate database
   +      // check.
   

   Let's use "revision ID" instead of "vid".

   Also this comment could do a better job of explaining what is happening. Why is the default language relevant, and not the current language, for example?

4. +++ b/core/modules/media/src/Controller/MediaController.php
   @@ -0,0 +1,98 @@
   +class MediaController extends ControllerBase implements ContainerInjectionInterface {
   

   Since this already injects all of its dependencies, there is no need to extend ControllerBase, as far as I can tell.

5. +++ b/core/modules/media/src/Controller/MediaController.php
   @@ -0,0 +1,98 @@
   +  public function revisionShow($media_revision) {
   +    $media = $this->entityManager->getStorage('media')->loadRevision($media_revision);
   +    $media = $this->entityManager->getTranslationFromContext($media);
   +    $media_view_controller = new EntityViewController($this->entityManager, $this->renderer);
   +    $page = $media_view_controller->view($media);
   +    unset($page['media'][$media->id()]['#cache']);
   +    return $page;
   +  }
   

   We have EntityViewController::viewRevision() for this.

6. +++ b/core/modules/media/src/Controller/MediaController.php
   @@ -0,0 +1,98 @@
   +  public function revisionPageTitle($media_revision) {
   +    $media = $this->entityTypeManager->getStorage('media')->loadRevision($media_revision);
   +    return $this->t('Revision of %title from %date', ['%title' => $media->label(), '%date' => $this->dateFormatter->format($media->getRevisionCreationTime())]);
   +  }
   

   This we don't have as a generic function yet, but we might as well add it to EntityViewController here, I would say.

#20.1 Added exceptions.
#20.2 This allows for other operations as well, but since we don't implement them yet I removed this.
#20.3 Changed vid to revision ID. This for now is a copy of what the node module does. I'm not 100% sure why the default language is used and not the current language. Don't know who we can ask for that? I guess if we need to refactor/improve this it would be nice to handle this in #2887106: Add generic base classes for content entity revisions and #2887108: Use generic base classes for entity revisions in Node, Media and Block content. Or maybe a separate followup?
#20.4 Switched to EntityViewController
#20.5 Switched to EntityViewController
#20.6 Decided to remove the custom title callback. EntityViewController::buildTitle() overrides it anyway.

Yes, that was supposed to be a txt file.

Decided to remove the custom title callback. EntityViewController::buildTitle() overrides it anyway.

Totally missed that that buildTitle() is not used as a _title_callback but actually is called at runtime. Nice catch! I remember that at some point we were still supposed to add _title and _title_callback to all routes, so that the TitleResolver will have something to resolve, I'm not sure if that's still how we do it. I think that \Drupal\Core\Entity\Controller\EntityController::title(), though, should suffice as a sort of "fallback" _title_callback, so maybe we can just add that.

I'm not 100% sure why the default language is used and not the current language.

I looked into it a bit and I'm still not 100% sure it's correct to query on the default language (although it certainly could be, not sure). But I did realize that NodeRevisionAccessCheck is used for two different routes: /node/1/revisions and /node/1/revisions/2/view. The first one is a bit more complicated as it needs to check whether revisions exist in the first place, etc. and that is why we have that sort of logic. The second one should be pretty straightforward, as the routing system should throw a 404, if it can't load the revision, so we should just be able to check access on the loaded revision using the entity access control handler. And we actually already have a route access checker to do just that. So I think we can simply remove the custom access check and specify:

requirements:
  _entity_access: media_revision.view

Then when we provide a revision overview of some sort, then we can try to figure out what kind of access check we need. In any case I think splitting up the two different use-cases makes the code more understandable.

Edit: Now thinking about this, what my suggestion would allow is that you access /media/1/revisions/1 even if you only have a single revision whereas currently that is not the case. So I guess that suggests we should discuss that in a follow-up. So leaving RTBC, although, if you want to fix the _title_callback mentioned above, feel free ;-)

Actually looking around a bit more, I see that contrib entity.module has a protected function countDefaultLanguageRevisions() in the access checker, which performs the entity query instead of doing anything in the storage. I actually like that better than adding more stuff to our already over-powerful storage class, especially if we're not 100% sure about the use-case, so marking needs work for that.

Opened #2887529: Improve revision routing for the above.

Fixed #25 and #26.

• Added the title callback
• Moved countDefaultLanguageRevisions() to MediaRevisionAccessCheck
• Reverted the changes in countDefaultLanguageRevisions() in NodeStorage and NodeStorageInterface

Needs review :)

Thank you, looks very nice!

+++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
@@ -0,0 +1,151 @@
+    if (!$media || $op !== 'view') {
+      // If there was no media to check against, or the $op was not one of the
+      // supported ones, we return access denied.
+      return FALSE;
+    }

There are a lot of similarities of this code to node of course :)

So that made me wondering are we consciously not supporting update and delete operations for revisions? (We don't necessarily need to have a UI to do that?)

I think it would be nice overall to document why not.

Also the patch/approach in #2856363: Path alias changes for draft revisions immediately leak into live site does not require to have this link template anymore. It is nonetheless a good addition IMHO since media does support revisions, so looking at those revisions would ideally be possible on the UI somehow :) Just noting there is no blocking relationship anymore and no regressions caused. :)

(We don't necessarily need to have a UI to do that?)

Well, this is about routing access, so unless there's a UI - i.e. routes - for that it seems pointless to support that?

That's a good point :)

1. +++ b/core/modules/media/media.routing.yml
   @@ -4,3 +4,18 @@ entity.media.multiple_delete_confirm:
   +    _controller: '\Drupal\Core\Entity\Controller\EntityViewController::viewRevision'
   

   +1 for reusing this existing controller.

2. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
   @@ -0,0 +1,151 @@
   +    return AccessResult::allowedIf($media && $this->checkAccess($media, $account, $operation))->cachePerPermissions()->addCacheableDependency($media);
   

   lgtm

3. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
   @@ -0,0 +1,151 @@
   +    // Statically cache access by revision ID, language code, user account ID,
   +    // and operation.
   

   I doubt static caching is necessary, but \Drupal\node\Access\NodeRevisionAccessCheck::checkAccess() doe sthe same, so this is fine.

RTBC +1

Superb, thanks for the additional review @Wim Leers :) Committed!