Problem/Motivation

In #2856363: Path alias changes for draft revisions immediately leak into live site we're looking to add path aliases to non-default revisions. For example when creating a draft (forward) revision with Content Moderation. This works well with Node because we can alias the revision link (/node/1/revision/2). The only other supported entity types is Taxonomy Term and Media. Taxonomy Term is not revisionable yet so we won't need to worry about that, but Media is revisionable, and doesn't have a revision link template, so will not get this feature, and will regress to forward revision aliases breaking the "live" default revision alias.

Proposed resolution

Add a revision link template (and controller etc) for media entity.

Remaining tasks

User interface changes

API changes

Data model changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

timmillwood created an issue. See original summary.

seanB’s picture

Status: Active » Needs review
FileSize
37.05 KB

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-bundle creation permissions form media. is a related issue since we want revision permissions per type as well.

timmillwood’s picture

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

I'm wondering if we add version history, reverting and deleting of revisions in a follow up? Keep this issue to just adding the revision link template and revision page. However not too precious about that.

I do agree that we should add many base classes and interfaces for this stuff. Currently only node does this stuff, the other revisionable entity type in core, block content, doesn't have any of this yet either.

A couple of nit picks:

  1. +++ b/core/modules/media/media.routing.yml
    @@ -4,3 +4,56 @@ entity.media.multiple_delete_confirm:
    +    ¶
    

    Too much spaces.

  2. +++ b/core/modules/media/media.routing.yml
    @@ -4,3 +4,56 @@ entity.media.multiple_delete_confirm:
    \ No newline at end of file
    

    No new line

Status: Needs review » Needs work

The last submitted patch, 4: 2885486-4.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
FileSize
13.81 KB
1.35 KB

This should fix the tests.

timmillwood’s picture

Status: Needs review » Needs work

I think it looks really good. Leaving the "Needs tests" tag as I expect there is a little more test coverage to add. Also Not sure we should add a storage handler.

+++ b/core/modules/media/src/MediaStorage.php
@@ -0,0 +1,22 @@
+  public function countDefaultLanguageRevisions(MediaInterface $media) {
+    return $this->database->query('SELECT COUNT(*) FROM {media_field_revision} WHERE mid = :mid AND default_langcode = 1', [':mid' => $media->id()])->fetchField();
+  }

I don't really like that we're overriding the storage handler just to add this in. It'd be nice if we could add it to SqlContentEntityStorage. I know Node does this, but maybe it shouldn't either.

We could do it in a follow up, but as this it would prevent us adding a whole new file, doing it now seems wise.

seanB’s picture

Status: Needs work » Needs review
FileSize
19.12 KB
9.41 KB

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.

seanB’s picture

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

The last submitted patch, 8: 2885486-8.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: 2885486-9.patch, failed testing. View results

seanB’s picture

Moved countDefaultLanguageRevisions to SqlEntityStorageInterface.

timmillwood’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
    @@ -23,4 +23,15 @@
    +  public function countDefaultLanguageRevisions(EntityInterface $entity);
    

    I think it might be best to add this to \Drupal\Core\Entity\Sql\SqlEntityStorageInterface rather than ContentEntityStorageInterface.

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

    +1 changing this to a select!

  3. +++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
    @@ -26,6 +85,7 @@ public function testFileMediaRevision() {
    +    $this->getSession()->wait(10000);
    

    This seems an unrelated change?

  4. +++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
    @@ -66,6 +133,7 @@ public function testImageMediaRevision() {
    +    $this->getSession()->wait(10000);
    

    And again.

  5. +++ b/core/modules/media/tests/src/Functional/MediaRevisionTest.php
    @@ -83,6 +151,29 @@ public function testImageMediaRevision() {
    +  ¶
    

    Too many spaces (as you spotted).

timmillwood’s picture

Issue tags: -Needs tests

#13.1 and #13.5 are fixed in #12. Just #13.3 and #13.4 to clarify.

seanB’s picture

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.

timmillwood’s picture

@seanB - My concern is, what have we added in this issue that adds the need to wait on the ajax request? If the answer is nothing then it should go in another issue, for clarity more than anything.

Status: Needs review » Needs work

The last submitted patch, 12: 2885486-12.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
FileSize
18.48 KB
964 bytes

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

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

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.

seanB’s picture

Status: Needs work » Needs review
FileSize
15.77 KB
7.01 KB

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

Status: Needs review » Needs work

The last submitted patch, 21: interdiff-18-21.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review

Yes, that was supposed to be a txt file.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Looks to cover everything in #20, so back to RTBC

tstoeckler’s picture

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

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

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.

tstoeckler’s picture

seanB’s picture

Fixed #25 and #26.

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

Status: Needs work » Needs review

Needs review :)

timmillwood’s picture

All looks good to me, but I think @tstoeckler should be the one RTBC'ing.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, looks very nice!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

Gábor Hojtsy’s picture

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

tstoeckler’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

That's a good point :)

Wim Leers’s picture

  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

  • Gábor Hojtsy committed 062c255 on 8.4.x
    Issue #2885486 by seanB, timmillwood, tstoeckler, Wim Leers: Media...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.