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
Comment | File | Size | Author |
---|---|---|---|
#28 | 3192059-28.patch | 3.82 KB | chrisfromredfin |
#15 | 3192059-9.patch | 3.82 KB | chr.fritsch |
#11 | 3192059-9-FAIL.patch | 2.62 KB | chr.fritsch |
#9 | interdiff-3192059-7-9.txt | 1.8 KB | chr.fritsch |
#9 | 3192059-9.patch | 3.82 KB | chr.fritsch |
Comments
Comment #2
chr.fritschHere is a patch
Comment #3
alexpottIs it possible that someone is expecting it to work this way?
What's the impact on http://grep.xnddx.ru/search?text=hasSourceFieldChanged&filename= ?
Should we be improving the documentation on this method as well?
Comment #4
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #2 and the issue is solved.Adding screenshots below.
Before patch:
After patch:
Comment #5
chr.fritschTBH, 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.
This will not be affected. There the entire source value is changed. So the mapping will still be triggered.
Updated the method docs.
Comment #6
alexpottHow about?
Also the docs for getMainPropertyName say...
... 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.
Comment #7
chr.fritschOk, let's fall back to the current behavior in case we don't have the main property.
Comment #8
phenaproximaAlthough 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 inMediaSourceBase
, but not inMediaSourceInterface
(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.
Comment #9
chr.fritschAfter 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.
Comment #10
phenaproximaI 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 :)
Comment #11
chr.fritschHere is the fail patch
Comment #13
chr.fritschFailed as expected
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedThe last patch should be the success patch so it gets retested every two days. Setting to NW to re-upload the patch from #9.
Comment #15
chr.fritschReupload #9
Comment #16
alexpottCommitted and pushed 6d5e083a77 to 9.2.x and 0c968a45cd to 9.1.x. Thanks!
Improved the comment on commit.
Comment #19
chr.fritschI 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.
Comment #20
chr.fritschFixing the staus after some advice form @alexpott
Comment #21
Wim LeersVery nice improvement! 👏
Comment #22
chr.fritschRaising to major, since the duplicate was already raised by @phenaproxima
Comment #23
chrisfromredfinPatch rolled against 8.9.x.
Comment #24
BerdirDrive-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.
Comment #25
damondt CreditAttribution: damondt commentedI 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.
Comment #26
chrisfromredfin@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.
Comment #27
stefanos.petrakis@gmail.comThe 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.
And a nit, an empty line introduced inside a multi-line comment.:
Comment #28
chrisfromredfinI'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. :)
Comment #29
jonathan_hunt CreditAttribution: jonathan_hunt commented@chrisfromredfin Patch #28 is working for me on Drupal 8.9.13, thanks.
Comment #30
MatroskeenUnfortunately, it didn't go into 8.9.x and according to @larowlan message:
I'm moving this back to 9.1.x and marking as Fixed.
Thanks everyone!