Add Media revision UI

Trying to implement the reqested feature.

Implemented basic functionality, needs test coverage.

📌 Revert and Delete actions should not be available for the current revision Active will be a blocker for this issue.

Can you explain why 📌 Revert and Delete actions should not be available for the current revision Active must block this issue but did not block ✨ Add Block Content revision UI Fixed ? 🙏

It is working for Block content so I am missing something, trying to find that.

📌 Revert and Delete actions should not be available for the current revision Active doesn't actually have to block this issue.

The reason it didn't block ✨ Add Block Content revision UI Fixed is because in that issue the logic to remove the revert and delete button was added to the block content access handler, but I created 📌 Revert and Delete actions should not be available for the current revision Active because there's no reason that it can't be added to a generic access handler, since pretty much every implementation of the generic revision UI will need that same logic.

So not strictly a blocker, but it would be really nice if we could do it generically and not have the same code duplicated for each implementation.

Yep, after looking at it more closely I came to the same conclusion that It is nice to have but not a blocker for this issue.

Still the access needs to be changed and test coverage for it.

This seems in line with what we did for Block Content

Applying MR 4057 to 11.x and clearing cache I can confirm I'm seeing the revisions tab on media.
Created a few revisions and reverted back n forth with no issue.

Test coverage also seems to fully cover the change.

Only thing missing we did for Block Content is add a change record.

added change record at [#3366630]

@yash the change notice shouldn't be published until the issue is merged, I've unpublished it. Thanks for creating it!

Change record and everything else looks good to me!

There appears to be one BC break for JSON:API? If we can revert the JSON:API test change and the test still passes, then my only major concern would be addressed! 😊

Think the one open thread is legit. Needing those permissions for GET doesn't seem correct. The node module didn't need to grant access to the content page as a reference.

How come we are editing the current access checking? I have a feeling this could impact existing sites if not configured in a specific way.

It won't cause any BC problem as there was already an early return for admin in upstream.

I am not sure if we want to Allow admin permission to override all operations for revisions also?

Lets see what the committers think. Don't think I can make that call.

I have a doubt about the permissions, do we need two if checks in the \Drupal\media\MediaAccessControlHandler::checkAccess()'s view all revisions case? asking this because we don't have anything like that for block content but it was there in upstream for media.

Will admit block content may need some tweaking itself. We just added granular permissions a few weeks ago.

Moving back to committer eyes.

Left some comments on the MR, there's something awry with the permissions in my opinion

Still appears to be working.

This seems reasonably straightforward but could use some cleanup.

addressed feedbak.

Back to the committer queue.

I'm sorry, this isn't quite ready to go yet. There is still some stuff that could use clean-up and consistency; in particular, I'm not sure the functional test coverage is testing all the ins and outs that it should.

Shaping up! But I have more feedback. I'm not sure the functional test coverage is as robust as it should be, and it could still use some streamlining.

Got some idea about the default and latest, so giving it a shot.

I discussed this today with @yash.rode and I think the thing I'm stuck on, at least for now, is: why are we borrowing access logic from Block Content?

Custom blocks are not meant to be viewed on their own. Media sort of is -- it's disabled by default, but they're not quite embedded things like blocks are. I feel like Node's access control logic is closer to what we'd want for media items, albeit without the complicated system of access grants.

To me, the logic we want is this, based on permissions:

If the operation is view, you can see the media if ANY of the following are true:

• You have view all media revisions
• You have view any TYPE_ID media revisions
• You have administer media (this is already the case in HEAD)

If the operation is revert or delete revision, you can do the operation if either:

• You have revert any TYPE_ID media revisions/delete any TYPE_ID media revisions
• You have administer media

AND the media item in question is NOT the default revision. It's okay if it's the latest revision -- that ensures it plays nicely with Content Moderation, which has the concept of a newer, not-yet-published revision -- but you can't revert or delete the default revision, because that makes no sense. So, for example:

      case 'revert':
        return AccessResult::allowedIfHasPermissions($account, [
          'revert any ' . $type . ' media revisions',
          'administer media',
        ], 'OR')
          ->andIf($is_not_default_revision);

Then what about the logic that controls access to latest revision, for removing the revert and delete revision button 📌 Revert and Delete actions should not be available for the current revision Active . We need that for generic revision UI.

Ooof! 🤯 🤬 What a maddening bug!

IMHO the correct thing to do is postpone this issue on getting that bug fixed. Chances are, the fix isn't too complicated to implement. I'd rather we did that than add confusing workarounds to this issue.

Per #68.

📌 Revert and Delete actions should not be available for the current revision Active has been committed.

Rebasing and changing the MediaAccessControlHandler.

Posted a new review 🏓

> Then I think it'd be better to not have both Media::preSaveRevision() and BlockContent::preSaveRevision() achieve the same behavior using two very different method implementations and instead move that to EditorialContentEntityBase::preSaveRevision() RevisionLogEntityTrait::preSaveRevision().

We have an existing issue to standardize that, but it's been stuck on semantics/naming for years. This is not the issue for this and I don't think it should block it.

@Berdir: TIL!

Then let's:

1. link that issue and have that issue point to this as another example.
2. at least ensure that BlockContent::preSaveRevision() and Media::preSaveRevision() are identical, to allow that issue to land more easily in the future. That way, Media::preSaveRevision() needs to change only once.

📌 Automatically set revision user/log information/created time on entity revisions Needs work is the issue I talked about.

Addressing Omkar's feedback.

AFAICT there's a security vulnerability/regression in here 😅

I have few questions/doubts:

1. Do we need to add revert all revisions
and delete all revisions 
permissions like node.permissions.yml?
2. Do we need to care about the default and latest revision in this issue? Because, we were doing that before to remove the revert and delete buttons from default revision. We don't care about revision when it's a view revision operation.
   Why to get rid of default and latest handling in this issue -> We are already doing that in 📌 Revert and Delete actions should not be available for the current revision Active

Thanks in advance.

Need some clarification to proceed.

Should be quick fix for CC failure.

Need help with https://git.drupalcode.org/project/drupal/-/merge_requests/4057#note_205829.

This is looking pretty good to me. The scope of changes is clear, and the test coverage is solid, although could use some clean-up in certain spots. But I took a long time to read all the way through this and I feel like I understand what we're trying to do.

So this one doesn't stall and maybe can land in 10.2!

Tested again by creating a Media object (Image bundle)
Editing the source file
Verified the revisions tab
Reverted back to the previous revision
Old image is used.
Deleted revision 2 jsut fine.

Addressed Ben's suggestions except one thread, which is not clear to me. Thanks for the review.

Two remaining threads to address. The changes are straightforward enough that whoever makes them is welcome to self-RTBC this and get it back into committer consideration.

This also needs an update hook for the entity defintion. I noticed this was added in some of the other Revision UI issues and wasn't sure why it was needed since updating the definition in the PHP class provided the expected functionality. The EntityDefinitionUpdateManagerInterface docs explain why, and there's are existing examples to work from and just swap in the media specific stuff.

Threads have been resolved, and more work has been done to fix other bugs. This needs a re-review.

Not sure I'm one that can mark this so apologize if I'm wrong

Followed the same steps as above

Tested again by creating a Media object (Image bundle)
Editing the source file
Verified the revisions tab
Reverted back to the previous revision
Old image is used.
Deleted revision 2 just fine.

Seems all the threads have been resolved except maybe the deprecation of core/modules/media/media.routing.yml. But @acbramley makes a good point about it being needed.

Think this landing in 10.2 would be great.

lauriii → closed merge request !4057

Committed d4c6b32 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

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