Problem/Motivation

In the media image type we set up a fieldmapping from filename to media name.

Imagine a media item where a custom media name was set. When the alt of the image field will be changed, also the media name gets reverted to the filename. This is because Media::hasSourceFieldChanged() compares all values of the source field.

Steps to reproduce

1. Install standard and enable media
2. Put the media name in the form display of the image type
3. Go to media/add/image and upload an image, and set your media name
4. Open the edit form of that media and change the alt
5. Save
6. The name of the media image will be the filename

Proposed resolution

IMHO Media::hasSourceFieldChanged() should not compare all values of the source field and instead only the mainProperty of the source field should be used.

Remaining tasks

Write patch
Review
Commit

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

Status: Active » Needs review
FileSize
3.47 KB

Here is a patch

alexpott’s picture

Is it possible that someone is expecting it to work this way?

What's the impact on http://grep.xnddx.ru/search?text=hasSourceFieldChanged&filename= ?

+++ b/core/modules/media/src/Entity/Media.php
@@ -269,7 +269,8 @@ protected function getThumbnailUri($from_queue) {
   protected function hasSourceFieldChanged() {
     $source_field_name = $this->getSource()->getConfiguration()['source_field'];
     $current_items = $this->get($source_field_name);
-    return isset($this->original) && !$current_items->equals($this->original->get($source_field_name));
+    $main_property_name = $this->getFieldDefinition($source_field_name)->getFieldStorageDefinition()->getMainPropertyName();
+    return isset($this->original) && $current_items->{$main_property_name} !== $this->original->get($source_field_name)->{$main_property_name};
   }

Should we be improving the documentation on this method as well?

Abhijith S’s picture

FileSize
21.14 MB
20.28 MB

Applied patch #2 and the issue is solved.Adding screenshots below.

Before patch:
before

After patch:
after

chr.fritsch’s picture

Is it possible that someone is expecting it to work this way?

TBH, I don't think so. Also, you can not map the alt or title, so it would make no sense to trigger the mapping again.

What's the impact on http://grep.xnddx.ru/search?text=hasSourceFieldChanged&filename= ?

This will not be affected. There the entire source value is changed. So the mapping will still be triggered.

Updated the method docs.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/media/src/Entity/Media.php
@@ -261,6 +261,11 @@ protected function getThumbnailUri($from_queue) {
+   * The determination happens by comparing the main property of the source
+   * field. This is important because e.g. ImageItem's not only store the file,
+   * but also alt and title. This method shouldn't return TRUE if only the alt
+   * value has changed.

How about?

   * The determination happens by comparing the main property of the source
   * field. This is important because some source fields have multiple
   * properties. For example, ImageItem not only stores the file, but also alt
   * and title. This method should not return TRUE if only the alt value has
   * changed.

Also the docs for getMainPropertyName say...

   * @return string|null
   *   The name of the value property, or NULL if there is none.
   */
  public function getMainPropertyName();

... do we have to cope with the NULL case? In case someone has used a source field that extends \Drupal\Core\Field\Plugin\Field\FieldType\MapItem for example.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
1.84 KB

Ok, let's fall back to the current behavior in case we don't have the main property.

phenaproxima’s picture

Although I agree with the rationale for this change, I'm not sure if we should be changing it for every media type. I like that we use FieldItemListInterface::equals(), because it delegates the actual comparison to the data layer, which makes sense and keeps our implementation clean.

Ideally this is something we could change for image media only, but the only obvious way to do that would be to change the Image source plugin, adding a public method for the comparison. We could do that in MediaSourceBase, but not in MediaSourceInterface (since that would be a BC break), which would add confusion to the API.

On the other hand, I might also be overthinking it. The fact that @alexpott seems okay with the current implementation appears to back that up. So I guess I'll close by saying that I'm okay with this patch as-is, but felt that it was worth sharing these thoughts anyway.

chr.fritsch’s picture

After some back and forth, @alexpott and I came to the conclusion that using MediaSourceInterface::getSourceFieldValue() here is probably the best way.
In that way, we are delegating the decision which properties should be compared to the source.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think that decision makes a lot of sense. RTBC, but could you post a fail patch as well, @chr.fristch? I want to be certain that the test is actually testing the bug :)

chr.fritsch’s picture

Here is the fail patch

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3192059-9-FAIL.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Reviewed & tested by the community

Failed as expected

quietone’s picture

Status: Reviewed & tested by the community » Needs work

The last patch should be the success patch so it gets retested every two days. Setting to NW to re-upload the patch from #9.

chr.fritsch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.82 KB

Reupload #9

alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6d5e083a77 to 9.2.x and 0c968a45cd to 9.1.x. Thanks!

diff --git a/core/modules/media/src/Entity/Media.php b/core/modules/media/src/Entity/Media.php
index 3f45530d94..6649cccfd0 100644
--- a/core/modules/media/src/Entity/Media.php
+++ b/core/modules/media/src/Entity/Media.php
@@ -261,9 +261,8 @@ protected function getThumbnailUri($from_queue) {
   /**
    * Determines if the source field value has changed.
    *
-   * The determination happens by comparing the return values of
-   * MediaSourceInterface::getSourceFieldValue(). Using that function delegates
-   * the decision, which property is important, to the source.
+   * The comparison uses MediaSourceInterface::getSourceFieldValue() to ensure
+   * that the correct property from the source field is used.
    *
    * @return bool
    *   TRUE if the source field value changed, FALSE otherwise.

Improved the comment on commit.

  • alexpott committed 6d5e083 on 9.2.x
    Issue #3192059 by chr.fritsch, Abhijith S, phenaproxima, alexpott: Use...

  • alexpott committed 0c968a4 on 9.1.x
    Issue #3192059 by chr.fritsch, Abhijith S, phenaproxima, alexpott: Use...
chr.fritsch’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Fixed » Patch (to be ported)

I would like to see this in 8.9.x

Also #3157183: Contents of Name field replaced by default value (i.e filename), which was closed as a duplicate, was major and had a lot of followers. So I think it's a good idea to backport.

chr.fritsch’s picture

Title: Use the source field main property to determine if the source field has changed » [backport] Use the source field main property to determine if the source field has changed
Status: Patch (to be ported) » Reviewed & tested by the community

Fixing the staus after some advice form @alexpott

Wim Leers’s picture

Very nice improvement! 👏

chr.fritsch’s picture

Priority: Normal » Major

Raising to major, since the duplicate was already raised by @phenaproxima

chrisfromredfin’s picture

Patch rolled against 8.9.x.

Berdir’s picture

+++ b/core/modules/media/src/Entity/Media.php
@@ -261,15 +261,20 @@ protected function getThumbnailUri($from_queue) {
   protected function hasSourceFieldChanged() {
-    $source_field_name = $this->getSource()->getConfiguration()['source_field'];
-    $current_items = $this->get($source_field_name);
-    return isset($this->original) && !$current_items->equals($this->original->get($source_field_name));
+    $source = $this->getSource();
+    return isset($this->original) && $source->getSourceFieldValue($this) !== $source->getSourceFieldValue($this->original);
   }

Drive-by-comment: this isn't introduced here, but for the record, having a main property is optional, there are field types with multiple main properties or dynamic properties where this doesn't work. Certainly uncommon for a media source field, but not impossible.

damondt’s picture

I don't know if this intends to fix https://www.drupal.org/project/drupal/issues/3157183 where changing the image file in the media item results the in the same issue, but it does not address that issue, and that issue was closed as a duplicate of this one.

chrisfromredfin’s picture

@damondt - commented in the other issue as well, but this fix does fix that issue as well; I believe it is properly marked as a duplicate.

stefanos.petrakis@gmail.com’s picture

Status: Reviewed & tested by the community » Needs review

The docblock text added by the 9.x patch (#15) is different from the one in the 8.9.x backport (#23), any reason for that?
Also missing the @see reference.

+++ b/core/modules/media/src/Entity/Media.php
@@ -261,15 +261,20 @@ protected function getThumbnailUri($from_queue) {
   /**
    * Determines if the source field value has changed.
    *
+   * The determination happens by comparing the return values of
+   * MediaSourceInterface::getSourceFieldValue(). Using that function delegates
+   * the decision, which property is important, to the source.
+   *
    * @return bool
    *   TRUE if the source field value changed, FALSE otherwise.
    *
+   * @see \Drupal\media\MediaSourceInterface::getSourceFieldValue()
+   *
    * @internal
    */

And a nit, an empty line introduced inside a multi-line comment.:

   /**
@@ -354,7 +356,6 @@ public function prepareSave() {
     // brittle and should probably be handled by a queue, to avoid doing HTTP
     // operations during entity save. See
     // https://www.drupal.org/project/drupal/issues/2976875 for more.
-
     // In order for metadata to be mapped correctly, $this->original must be
     // set. However, that is only set once parent::save() is called, so work
chrisfromredfin’s picture

I'm not sure what happened with the comments there, but I've rectified. The removal of that blank line was something Prettier did (per Drupal coding standards?) but I got it back in with vim so it's not an issue. :)

jonathan_hunt’s picture

@chrisfromredfin Patch #28 is working for me on Drupal 8.9.13, thanks.

Matroskeen’s picture

Title: [backport] Use the source field main property to determine if the source field has changed » Use the source field main property to determine if the source field has changed
Version: 8.9.x-dev » 9.1.x-dev
Status: Needs review » Fixed

Unfortunately, it didn't go into 8.9.x and according to @larowlan message:

Anything that is open for backport for 9.1 and 8.9 can be marked fixed and have the version updated to the lowest version it was committed to.

I'm moving this back to 9.1.x and marking as Fixed.
Thanks everyone!

Status: Fixed » Closed (fixed)

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