Mapped media fields are overridden with metadata on translation save

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 last submitted patch, 62: 2999370-62.patch, failed testing. View results →

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.

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

Please review.

The last submitted patch, 71: 2999370-71.patch, failed testing. View results →

+++ 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 🐛 Mapped media fields are overridden with metadata on translation save Needs review . 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.

Anybody → changed the visibility of the branch 11.x to hidden.

Anybody → changed the visibility of the branch 9.3.x to hidden.

Anybody → changed the visibility of the branch 11.x to active.

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

Anybody → closed merge request !8973

Grevil → changed the visibility of the branch 2999370-mapped-media-fields-11.x to hidden.

Grevil → changed the visibility of the branch 2999370-mapped-media-fields to hidden.

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.

grevil → changed the visibility of the branch 2999370-mapped-media-fields-fix-11.x-test-only to hidden.

Simple rebase fixed it again. Back to needs review!

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