Add Media revision UI

What about using views? That would enable users to modify the revision page/table if they have extra media fields, for example.

https://gist.github.com/rlnorthcutt/dff8e7df2d6336089cd2234b0109e1a5

Does this issue cover everything related to viewing media revisions? The issue I'm seeking clarification on is, currently you can create a revision of a media entity and add a revision log message, but there seems to be no way to view these.

I am not looking to revert or delete revisions, just to view them. Is it accurate that there is no way to do this currently? If not can someone point me in the right direction?

I am also facing the same issue. Please suggest a patch by which we can provide revert functionality for media same like node.

If you just want to view the revision log through a revisions tab on media without the diff module features, I've created a module just for that on my github:
Link to the media_revision_log module.

I created this module for the project I am on and it is going to production on their website. Hope it helps for those that just want to see the logs from the front-end, instead of having to check the media revision tables in the database for their customers.

If you just want to view the revision log messages from the front-end through a revisions tab on a media, then I've created a module just for that:
Link to the media_revision_log module

I created this module for the project I am on and stakeholders are deploying it into production. Hopefully this helps those that just want to view the log messages, and saves them time from looking in the database for their customers to find them.

This will be important when extra fields are added to audio/video media as part of the plan at #3002770: Provide authors with tools to manage transcripts and captions/subtitles for local video and audio.

In particular, transcripts will be a long text field, so the ability to manage revisions (or compare them with Diff module) will be really useful.

I wouldn't say that a revisions UI for media entities is a blocker for that plan though.

I created a module that adds a Revisions tab for media entities.

https://www.drupal.org/project/media_revisions_ui

You can view, revert or delete revisions the same way as nodes.

Is this issue postponed on a different issue? If this is the case, which issue is this?
(I don't see any comment changing the status; yet, the issue passed from Closed (duplicate) to Postponed.)

I think this looks like a duplicate of #2350939: Implement a generic revision UI, which would address this generically, so I'm closing it out in favor of that.

It seems appropriate to re-open this, as #2350939: Implement a generic revision UI introduces a foundation for a revision UI, but intentionally does not add it to any entity types.

#2350939: Implement a generic revision UI is now merged, unblocking this.

renaming in the same style as its peers, e.g. #1984588: Add Block Content revision UI

Trying to implement the reqested feature.

Implemented basic functionality, needs test coverage.

#3323788: Revert and Delete actions should not be available for the current revision will be a blocker for this issue.

Can you explain why #3323788: Revert and Delete actions should not be available for the current revision must block this issue but did not block #1984588: Add Block Content revision UI? 🙏

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

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

The reason it didn't block #1984588: Add Block Content revision UI 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 #3323788: Revert and Delete actions should not be available for the current revision 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 in the generic access handler and not have the same code duplicated for each implementation.

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 https://www.drupal.org/node/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.

Additionally, is there any testing for support of translated revisions? Do we need additional coverage for that? I'd be okay deferring that to a follow-up, since I think most if not all of Media's translation handling is bog-standard for core entities. Thoughts, @larowlan or other framework managers?

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

So, to summarize my thoughts here: I think we SHOULD prevent destructive operations on the default revision, but NOT take into account whether it is also the latest revision. That only comes into play when Content Moderation is enabled.

Now, that being said, Yash pointed out to me that, without Content Moderation, all revisions that are created are considered the default revision. I confirmed that he's correct -- the problem seems to be that \Drupal\Core\Entity\ContentEntityBase::$isDefaultRevision has a default value of TRUE, and except for Content Moderation, there is no way to override that in the UI! In other words, Drupal's default behavior is that every revision is the default revision. That seems utterly bananas to me -- and it effectively means that, if we do the thing I just laid out, you can never revert or delete a revision, because all revisions are the default!! (Unless, of course, you have Content Moderation set up for media.) So I see why Yash had to put in the logic that also checks for the latest revision.

But surely that is a bug in core? Do we have an issue to fix Drupal's behavior such that only the latest revision is the default revision, rather than all of them being the default? If not, we probably want to open that issue.

Then what about the logic that controls access to latest revision, for removing revert and delete revision button. 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.

#3323788: Revert and Delete actions should not be available for the current revision 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.

#2869056: Automatically set revision user/log information/created time on entity revisions 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 #3323788: Revert and Delete actions should not be available for the current revision

Thanks in advance.

Need some clarification to proceed.

Should be quick fix for CC failure.

Need some clarification about https://git.drupalcode.org/project/drupal/-/merge_requests/4057#note_205829.
Which test case will fail if we remove this? I can't think of any.

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.

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