Remove source from media mappings

Applied the patch in [#2] and ran into some issues when saving a revision. Looks like the $this->original = $this->getRevisionId(); line in the prepareSave function is being set to a string and will be set every time through, negating the conditional block above it.

The error is:

TypeError: Argument 1 passed to Drupal\media\MediaSourceBase::getSourceFieldValue() must implement interface Drupal\media\MediaInterface, string given, called in /var/www/html/docroot/core/modules/media/src/Entity/Media.php on line 277 in Drupal\media\MediaSourceBase->getSourceFieldValue() (line 327 of core/modules/media/src/MediaSourceBase.php).

This occurred when creating a draft revision of a published media entity. I changed the file field.

$this->original should implement Drupal\media\MediaInterface, maybe in an else condition to the block above it.

$this->original = $this->entityTypeManager()
        ->getStorage('media')
        ->loadUnchanged($this->getRevisionId());

OK. I'll poke around at it some more soon as well. I did just move to the 3.x dev branch of the Media Revisions UI and am seeing the same issue.

Yes, I used both core patches and the 3.x dev version of media_revisions_ui.

@smustgrave Had to move on to other priorities, but could not get past the problems I was seeing.

Still trying to understand the prepareSave() method in the Media.php file though, as it appears that $this->original is set to an entity within a conditional, but is then set to a string after the conditional. Why bother with the conditional loading an entity if it is just going to be set to a string after it?

    if (!isset($this->original) && ($id = $this
      ->id())) {
      $this->original = $this
        ->entityTypeManager()
        ->getStorage('media')
        ->loadUnchanged($id);
    }

    $this->original = $this->getRevisionId();

I am using media entity file replace and when i want to see a revision it says access denied and when i revert a media the old file does not get reverted . the patch #14 did not help .

patch #2 doesnt help either, I get access denied when i try to see the previous revision and when i revert it the body and title of the media gets reverted but not the file. using drupal 9.4.0-alpha1. tried with drupal 9.5 as well. which patch should work? (2 or 14 )?

yes I added that patch and have permissions to see revisions
Note: I am using media_entity_file_replace module
(these are the patches i used )

  "revisions not showing file": "https://www.drupal.org/files/issues/2022-03-25/3271688-2.patch",
               "revision ui patch ": "https://www.drupal.org/files/issues/2021-11-12/2350939-202.patch"

these are the steps i take
1. edit an existing media object , changing the file and title
2. save as published.
3. go to revisions tab
4. cannot see the old revision but can revert it
(image attached of error when try to view )
5. when reverting it doesnt change the file back to the older file

tried once more with the same patches you mentioned in the last post , got the same results,

1. Created new media of type image with image a.
2. edited and replaced the image with image b
3. after editing , i cant view the revision it says access denied.

I am using media_entity_file_replace module could that be the problem?

when i turn off the media_entity_file_replace and still have the patches the revisions work. But I need that module to prevent filenames from having a _1 attached to it when they are replaced.
I created an issue in that module https://www.drupal.org/project/media_entity_file_replace/issues/3282165#...

thank you will use that.

with patch #2 i get this error as the person above did TypeError: Argument 1 passed to Drupal\media\MediaSourceBase::getSourceFieldValue() must implement interface Drupal\media\MediaInterface, string given
but without this patch the revisions dont work.

I used patch #14 instead and that fixes the error and works for me

with this patch #40 i no longer get the error and the revisions work however i cannot view the revisions before revrting (get permission denied) and i see this error in my logs Notice: Undefined index: ref_char in event_log_track_insert() (line 101 of /var/www/html/web/modules/contrib/events_log_track/event_log_track.module) when i try to view the revision

the complete access denied error is Path: /media/140/revisions/461/view. Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException: in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 118 of /var/www/html/web/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).

adding screen shot of the access denied error in log

i disabled media_alias_display , makes no difference. what other module could it be ? i will test disabling different ones

found the problem. if i create media belonging to no group i can see the revision. if i add it to a group i get access denied. found the issue in the Groups Issues here and the patch #35 there fixed it so i can see the revisions now https://www.drupal.org/project/group/issues/3256998

The issue summary makes it sound as though this is only reproducible with an uncommitted patch from #2350939: Implement a generic revision UI applied. If that's the case, the issue other issue should be marked NW on addressing the issue.

If this is reproducible without that page, please describe how; the issue sounds quite bad. Thanks!

Oh, the IS says it's using an uncommitted core patch and the Workflow module.

@smustgrave, thanks for making the issue and patch, with tests!

The information in #59 should be added to the IS. I too was confused why the patch in the other issue is being used.

I have not done a full triage here or reviewed the patch.

Fails since 9.4.4 - needs a re-roll. Will see if I can do that today.

Added reroll of patch #55 on Drupal 9.5.x. needs work for comment #60.

I've read this issue several times and its not clear what combination of modules (and patches) is needed to trigger it.

The changes to existing tests in the patch makes me feel there might be some behaviour changes here.

Can we get the issue summary updated here with the latest state of play - thanks!

Updated steps to test after manual testing

Left a review with the path forward and root cause

On second thoughts, I think a simpler way forward would be to:

• Prevent the 'source' field from being shown in the list of fields available in the mappings
• An update path to remove such entries from any existing media type

Holy smoke, I can't believe we allow the source field -- i.e., the single most critical field on any media entity, without which everything about that entity breaks badly -- to be mapped!! That's a major oversight.

I agree with @larowlan's proposed solution in #73. We should also make sure it is impossible to save a media type with a broken mapping like that -- trying to do it should throw a \LogicException or similar.

This is a great start! I've left a review with some proposed changes, hopefully to make things a little simpler and more robust.

Also we'll need to re-title and re-summarize this issue.

It's not a big deal, but the if ($source_field) check shouldn't be needed. All media types must have a source field, or they break.

I understand it's because we have a broken fixture somewhere, but could we not adjust the fixture -- or the test which uses it?

But as I said, the extra if shouldn't matter much since all functioning media types have a source field. I think this is ready for review.

Kicked things along a bit

Merged 11.x into this branch, resolved the remarks by @Wim Leers.

There's a failure in MediaTypeValidationTest::testRequiredPropertyValuesMissing, this was recently introduced in #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support

Left some suggestions on the MR

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot be wrong.

Couple of minor things now - thanks!

I've been over this several times and I can't fault it now. Thanks @smustgrave for putting up with my change requests.

As this bug requires an update function only backporting to 10.3.x. I'll put it to the release managers to backport this to 10.2.x

Committed and pushed 1857ebff31 to 11.x and 7c2638e272 to 10.3.x. Thanks!