Mapped media fields are overridden with metadata on translation save

I think the operator is correct, as any mapped fields get overridden if the "source value" changes. So if you have mapped the media name, that logic applies there, too. If you change the title and the image file at the same time, and have a mapping for the media title, the title will get overridden - that is intentional.

Scenario 1: The media name is not translatable and the translation's file was changed.
Saving a translation will also override the media name. Mapped fields should probably all be marked as translatable, if the file field is.

Scenario 2: The media name is translatable, and the translation's file was changed.
Saving the translation will also override the media name, because the file has changed and thus mappings need to be re-evaluated.

Scenario 3: The media name is translatable, and the translation's file was not changed in the translation.
This is a troublesome case, because $translation->hasSourceFieldChanged() always checks against the default language, instead of the translation. So if you have a different image for each language (think: the flag of the language's associated country), and change a translation's title, it will get overridden again - simply because `flag_xx.png` (in the translation) is different from `flag.png` (in the default language).

I've attached a patch to try and avoid this, but it probably isn't the perfect solution yet and we most definetely need tests for this.

Fixed error if translation does not exist (yet).

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

Was not executed when a new medium was created, thus $original = $this->original; led to $original to be NULL. This in returned led to an error while checking if the $original has a translation.
Fixed by only executing the search for the right translation, if $original is an object.

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

Was not executed when a new medium was created, thus $original = $this->original; led to $original to be NULL. This in returned led to an error while checking if the $original has a translation.
Fixed by only executing the search for the right translation, if $original is an object.

I missed that case, didn't test thoroughly it seems, thanks for spotting it! I've added a check for that to my previous patch, so we don't need the extra nesting level in the code.

We are also facing this issue. Our site does not have any translation related modules enabled.

I think there should be a way to allow user to override "Name" field even filename metadata is mapped to it. So, metadata mapping would occur only if user did not provide custom value for that field.

Removing unnecessary "media" tag.

In my opinion, metadata should be used as a fallback. So, if there is any value in the field, metadata should be ignored.

We have a similar description in the "FIELD MAPPING" section settings:
Media sources can provide metadata fields such as title, caption, size information, credits, etc. Media can automatically save this metadata information to entity fields, which can be configured below. Information will only be mapped if the entity field is empty.

Tests will likely fail, but let's see.

Previous patch didn't work for adding a translation where it didn't exist before.

I investigated this issue and I found solution and worked for me. This issue arisen because media entity name attribute returning the file name so media name override by file name while rendering the media entity info. I hope attached patch helpful to you and revert me if need any help.

Hello @maheshpa

The patch need re-roll.
Kindly review the updated patch.

The patch is still failing tests, so this still needs work. Sorry!

I'm running the tests locally and those are green. Let's try again

We break the tests because the expected behavior is to update the name. But TBH this is very confusing for users:

1- Add media

2- Update alt text

3- The name has been updated

I think we shouldn't update the name if the user has changed that value from the UI. Let's try something like this.

Verified and tested by applying the patch #29.It looks good to me.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/modules.
3. Enable Media and Multilingual languages.
4. Go to admin/config/regional/language.
5. Add another language.
6. Go to admin/config/regional/content-language.
7. Enable Media in Custom language settings and Save it.
8. Go to admin/structure/media/manage/image/fields/media.image.field_media_image.
9. Enable the Alternative text and Save settings.
10. Go to admin/content/media.
11. Add media type and enter text in Name and Choose Image.
12. On choosing image add Alternative text.
13. Save it.
14. Edit the saved media type and change the Alternative text.
15. Verify the listing page.

Refer to the attached screenshots and video.

Before Patch -
Step 1 -

Step 2 -

Step 3 -

After Patch -
Step 1 -

Step 2 -

Step 3 -

Adding #3157183: Contents of Name field replaced by default value (i.e filename) as a related issue, possibly a duplicate.

Escalating the priority of this issue, as it looks like it might cause unintentional data loss.

During a migration of media document translations this issue (assigned media name is overwritten with filename) popped up.

Removing the field mapping for Name (set to '- Skip field -') on admin/structure/media/manage/document avoids the issue from occurring.

The patch from #29 also resolves the issue in case the field mapping for Name is set to 'Name'. (drupal core 8.9.3)

The issue appeared for me in the case when the source field is not the name field, but some other.
The patch attached solved the problem for that case.

Worked on testcase failures, Please review.

Fixed tests, Please review.

I tested #29 on Drupal 8.9.9 and PHP 7.3 2 weeks ago and we didn't have this issue anymore.

I'm wondering if #3192059: Use the source field main property to determine if the source field has changed did the trick and resolved the issue we're struggling here 🤔
Would be great if anyone can re-test in the latest 9.1.x or 9.2.x.

I've tested this on Drupal 9.2.8, and #3192059: Use the source field main property to determine if the source field has changed does not fix the issue.

I'm also updating the title of this ticket, problem is not limited to media title. All mapped media fields are affected by this (as mentioned in #37).

I was testing with remote video and

The HTML representation of the resource

field mapping and without this patch I could not modify value of that field when editing a translation.

With this patch it works correctly.

Thanks for testing this, everyone. I think this needs a little more review before it can be RTBC, for two reasons:

1. I don’t understand why the change in the patch fixes the problem.
2. I’m not sure why we are removing assertions from the tests. If it’s necessary to properly test the fix, that’s fine, but without understanding the nature of the fix, I can’t be sure of that. We definitely do not want to inadvertently lose precious test coverage.

Thanks @phenaproxima for comments, I did more testing with it - and you're right. It does not fix the problem.

The patch fixed the problem of alterations being overridden, but it now stopped doing the mapping if source changes.

What I think might be better here is to first add completely new test coverage that proves the problem exists (i.e., an automated test that fails). Then, knowing that we are reproducing the problem consistently with a test, we can get to work finding a proper fix for the bug. Therefore, marking this as "needs tests".

Changing the branch target to 9.4.x, where all new development is taking place. This can be backported to 9.3.x at committer discretion.

Hiding patches in favor of using the merge request workflow. It's a lot easier to review changes that way.

Don't worry, everyone who worked on the patch will still get credit at commit time. :)

oh I was working on it too :)

anyway here is a fix for it from comment 11 - that looks good. And I've updated test to do one more save. I think it is correct that mapped field gets forcibly overridden when you change the source, but any subsequent save should then retain the changed values.

I've been experiencing this issue with the following workflow:

- Multilanguage site, but no translation involved.
- Create media attached to a node through entity reference. User enters the name of the media in the entity form.
- Media source is not immediately added as it is too large and it is processed (details don't matter, what matters is that media is created without source).
- When the async process finishes, it saves the media entity, and replaces the user entered name field by the media file name.

This can be reproduced by having to media entity form modes, one without file, and one with file. Create the media without file, enter the name and then save it again with file.

The latest code on the MR does not fix the issue but the change in #2 does. I think @phenaproxima failing test is correct.

Rather than changing the MR, I'm adding a patch so I'm not stepping over anyone, please lmk if the patch makes sense and I'll move it to the MR :)

F in spelling

The latest code on the MR does not fix the issue but the change in #2 does. I think @phenaproxima failing test is correct.

The response (comment #4) for comment #2 states that "If you change the title and the image file at the same time, and have a mapping for the media title, the title will get overridden - that is intentional." I agree with that especially when taking all of the mapped fields into an account. It could be argued that non-empty title should remain even if source is changed, but this code in question applies to all of the mapped fields. So in case of video source, if you change the source should the title be updated? what about the thumbnail? HTML embed code? other mapped fields?

I've modified the test with that in mind and the customized title is set in a subsequent save after the media is first saved with just the changed source property.

Rather than changing the MR, I'm adding a patch so I'm not stepping over anyone, please lmk if the patch makes sense and I'll move it to the MR :)

Thanks for that! To me #2 and #52 looks like different issue, where the automatic mapping for title that can not be turned off is causing the problem.

(sorry for the RTBC, no idea how that happened)

Makes sense, happy to open a followup if there's consensus, should that be a flag on the title/name replacement?

@mikran you were absolutely right, as it's mentioned on #4, what I've described on #52 only happens when you map the fields in the media entity itself, I've configured it as the screenshot and all works as expected:

Leaving this comment here in case someone else finds it useful

Setting back to Needs review for Merge request 1392

I came to this issue while researching a similar problem. However, in our case we do not have translation enabled. The problem seems to still happen with the x-default language. I tried this patch anyway and it doesn't work when no translation is enabled.

I then found #3157183: Contents of Name field replaced by default value (i.e filename) and I'm wondering if that patch might fix this problem too? Just putting this comment here incase that patch fixes translations as well.

UPDATE: I found a test, MediaSourceImageTest::testOnlyMainPropertiesTriggerSourceFieldChanged, that as an assertion that the entity name SHOULD change when a new file is uploaded. That test is currently causing #3157183: Contents of Name field replaced by default value (i.e filename) to fail tests. For that reason, I'm not sure that is is an appropriate fix and I'm wondering if the intended functionality of the media module is to ALWAYS force a document name change when the file name is change, but only when the field mapping exists.

Patch #53 is no longer working with core 9.45

#53 patch Re-rolled (9.5.x).

I have tested patch #62 with Drupal 9.5.3 and it successfully resolves the issue, that you cannot change the name ov a previously saved media entity translation.

The patch in #62 prevents the specific problem outlined in the description, but it introduces a regression. Specifically, metadata will never be mapped to entity fields, even if those fields are empty, unless the source field changes. This does not appear to match the original intent here.
It might not be a noticeable issue for most people because most metadata won't change unless the media source changes - e.g. if you edit an Image-sourced media entity and replace the image file. However, it's perfectly valid for there to be all sorts of other metadata that might be updated somewhere without the media source changing - e.g. a transcript on an externally-hosted video, a caption for an image stored in an external DAM, etc.
I wonder if we could restate the problem by defining the expected behavior as:

Available metadata should always be used to populate mapped fields when media entities are saved/resaved and the target fields are empty. When changing a media source field, we should expect any mapped fields to be updated with metadata-derived values, unless we are simultaneously providing alternative values for some of those fields.

I know it doesn't avoid the "I previously customized the media name, and I was hoping my custom name would persist after replacing the source file" scenario...but it seems to balance the concerns in this thread with the original intent.
Here's a patch that tries to put the aforementioned behavior into practice. If someone is able to look at tests for it or otherwise weigh in, that would be cool.

Edit: see #70 for better patch.

Updating patch to work with new media entity creation, improve empty value handling, and revise comments.

Please review.

+++ b/core/modules/media/src/Entity/Media.php
@@ -433,7 +432,7 @@ public function prepareSave() {
-          if ($translation->hasField($entity_field_name) && ($translation->get($entity_field_name)->isEmpty() || $translation->hasSourceFieldChanged())) {
+          if ($translation->hasField($entity_field_name) && ($translation->get($entity_field_name)->isEmpty() && $translation->hasSourceFieldChanged())) {

@Ajeet Tiwari - this proposal is the same as in #2. I don't believe this is good, because mapped fields that are empty should be eligible for population regardless of whether the source field is changing.

Hi, 62's patch has been re-rolled to work with 10.2.

I can sadly confirm this issue in all multilanguage projects.

@ndewhurst you say the patch in #70 is superior. Should this be turned into a MR against 11.x then and also implement tests (eventually the test copied from the existing MR?)

I agree this is major for all multilingual projects!

Ok, I manually applied patch #70 by @ndewhurst on a new issue branch and created an MR against current 11.x. I'll take a closer look at the patch tomorrow and see if I can get the existing tests to work for the new branch.

Ok something went wrong. I'll get back to it tomorrow.

@Grevil: This was indeed pain! I was finally able to apply #70 with MR !8973 (11.x - sorry for the bad naming).
I think we can now proceed there and hide / close the other MR's?

MR! 8983, containing the changes of patch #70 and the test provided with patch #62 seems to work great! The new code is a bit harder to read, but I see no caveats with it on my end.

Unsure if the provided test is enough, or if we should somehow recreate the case mentioned by @ndewhurst in #69:

However, it's perfectly valid for there to be all sorts of other metadata that might be updated somewhere without the media source changing - e.g. a transcript on an externally-hosted video, a caption for an image stored in an external DAM, etc.

Setting this to "Needs Review" for now, so a core maintainer can decide if the provided test is enough.

@Grevil: I tried if the "Test only" functionality in the Pipeline can help here to show the test-only test fails (https://git.drupalcode.org/issue/drupal-2999370/-/pipelines/239344/) but I'm not familiar enough with that. So could you eventually post a test-only branch to show the test fails without the fixes? Or do you know how to do that correctly?

See output of 2999370-mapped-media-fields-fix-11.x-test-only, the test fails without the changes as expected! 🎉

Yeah GREAT @Grevil! RTBC for MR !8983!
Would still be good if others here could also test the MR for edge-cases, so we can get this fixed finally. Thank you!

I do like the older issue getting fixed. Thanks for getting this to RTBC.

I've added the standard template to the issue summary, can someone fill out the proposed resolution here.

I reviewed the MR and left some suggestions and questions there. Setting to needs work for that.

Alright, adjusted the code based on the suggestions by @quietone. Please review once again!

Believe feedback has been addressed on this one.

I've added some comments on the MR to address.

Added another suggestion to the MR.

There are 2 MRs could one be hidden please.

Assuming the one to review is 8983 but that one appears to have test failures, moving to NW for investigation.

Simple rebase fixed it again. Back to needs review!

My mistake should of noticed before but issue summary is incomplete.

For me the MR did not apply on 11.1.5.

I had a similiar issue that if the source file is different on the translation than the name would not be updated and always gets the new source file title.

My WA would be to check on the $_POST variable but that feels like a real ugly hack.

I did insert this in Media::prepareSave():463

if ($this->activeLangcode === $langcode && $_POST['name'][0]['value'] ?? '' !== $translation->getName()) {
        $translation->setName($_POST['name'][0]['value']);
      }

Here is the patch with the hacky approach.

Updated the MR to latest 11.x forget about what I wrote earlier :D

Ok the MR solves the issue for 2 languages but not for > 2 e.g.
If you have configured de, it and fr and you will change the title for de (which is the default language) the last edited translation title will change back to the filename.

For me it is working if I remove the strict comparison check and only act if the old value is empty.
Maybe I'm too dumb but what is the benefit if we update the field with the default metadata if it does not change?

@didebru

Ok the MR solves the issue for 2 languages but not for > 2 e.g.
If you have configured de, it and fr and you will change the title for de (which is the default language) the last edited translation title will change back to the filename.

So with 3 languages you mean one original entity + 2 translations? I adjusted the tests and they seem to succeed. Any suggestions on how to improve the tests?

Ok, tests are green again. Unsure about the comment from @martins.bertins (https://git.drupalcode.org/project/drupal/-/merge_requests/8983#note_404381). Couldn't replicate this behavior. A test or clear steps to reproduce would be nice.

Setting back to needs review for now though.

Static patch for the time being. We will test it internally as well and come back here if any issues occur.

I have not written a test to replicate the behavior, but you can see in the video that it occurs when translating to the third language (base + 2nd translation), but without uploading a new image that the name of the 1st translation (not the base) is overwritten.

Video of showing issue

Please disregard the noise. I thought I had #116 applied, but it was not properly applied. I had an older patch. Checking again and will report findings.

The current patch, as well as the one from comment 116, doesn't apply properly in D10 any more - the conflicts are in the Kernel test. I've re-rolled the patch against Drupal 10.x for those who need it.

@teknocat thanks, if possible let's please instead (or additionally) keep the MR up to date and finish it, so this can be finalized. Thanks!

I opened #3567230: Only overwrite mapped field values on media items when source field original value was non-empty today which overlaps with this one, but it's a different issue. Whichever gets in first, the other will need a re-roll.

When I re-rolled the patch in my comment #118, I failed to realise that was not actually compatible with D10, which doesn't have the normalized getOriginal() API method. Not quite sure how I missed that until our client flagged errors when uploading media entities in their site (doh!).

@anybody how would you prefer a D10-compatible patch be handled in the short-term? I could just upload a re-rolled one. I understand that issues should always be opened against the current core development branch, but if D10 and D11 are both currently supported and sometimes need different patches to solves the same issues, like in this case, I'm not sure what makes the most sense to make it easy to manage and avoid any confusion around it.

Also I think I actually uploaded the wrong copy of the patch file I rolled for D10 compatibility. I just double checked my local codebase where I was working on the patch, where it needs to use $this->original for D10 and not $this->getOriginal(). (facepalm).

But before I upload a correct D10 version, I'd still like to know the best practice per my previous comment about a case like this.

This really is one of the most frustrating Drupal issues ever. It's particularly frustrating for site editors. It would be much appreciated if this could be fixed soon (created 13 Sep 2018).

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

#3567230: Only overwrite mapped field values on media items when source field original value was non-empty is fixed now, let's get this one finished also!

This one still appears to need issue summary update.