Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The media_library.opener.field_widget service is used by the media_library widget, and this service is throwing an \LogicException when the media_library widget is used in a field different from entity_reference.
This behavior blocks developers from extending entity_reference field type.
Steps to reproduce
- Create a custom field type extending \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem
- Create a custom field widget extending \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget and allowing the previously created field_type on its annotation.
- Add a field to an entity with the new type created on the step 1
- Create a content and use the media library on the new field
- When trying to insert the selected media, the ajax request will fail and you will find the following error message on the logs:
LogicException: Expected the media library to be opened by an entity reference field. en Drupal\media_library\MediaLibraryFieldWidgetOpener->checkAccess() (linea 105 de /path/to/docroot/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php).
Proposed resolution
MediaLibraryFieldWidgetOpener is checking the string returned by $field_definition->getType(). Check the $items interface instead.
Remaining tasks
Test and review.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#18 | 3173770-17-FAIL.patch | 4.11 KB | phenaproxima |
#16 | 3173770-16.patch | 5.33 KB | phenaproxima |
#14 | 3173770-14.patch | 6.52 KB | chr.fritsch |
#9 | 3173770-9.patch | 6.88 KB | chr.fritsch |
#9 | interdiff-3173770-2-9.txt | 7.01 KB | chr.fritsch |
Comments
Comment #2
akalam CreditAttribution: akalam at Metadrop commentedComment #3
akalam CreditAttribution: akalam at Metadrop commentedComment #7
phenaproximaTransferring credit from #3179868: Make MediaLibraryWidget more flexible, which I closed as a duplicate of this one.
Comment #8
phenaproximaI think I'm okay with this idea, but two things come to mind:
checkAccess()
permits it).target_type
) which is particular to that base implementation. So it's not enough that the item list just implement the entity reference interface; it actually has to be an entity reference, or a descendant of it.Comment #9
chr.fritschAdded a test and changed to check for
EntityReferenceFieldItemList
Comment #10
phenaproximaThat looks pretty good, but my only major objection is to how the test is written. Adding a data provider to an existing method is not a bad idea by itself, but the method in question is testing a specific set of validation-related circumstances, as opposed to just "the media library will open with an entity reference subclass". IMHO this should be its own method, so that we can make that distinction clear. Even if it's mostly identical to an existing test. I'm open to being convinced otherwise, but if it was implemented this way for convenience, it should be its own method.
One nitpick: could you change
if (!$items instanceof EntityReferenceItemList)
toif (!($items instance of EntityReferenceItemList))
? I ask because it's not immediately clear if the ! is evaluated first, or theinstanceof
. (You and I know that the ! is evaluated first, but the extra parentheses will make it crystal clear to those who don't have god-mode knowledge of PHP.)A request: we should have a comment above the
instanceof
check to explain why we are looking for a concrete class, rather than an interface. Otherwise, we're guaranteed to forget why we did it that way. Something like// Check that the field is an entity reference, or subclass of it, since we need to check the target_type setting.
And, one question: I'm not clear on why we need to subclass the widget, as opposed to just using
hook_field_widget_info_alter()
to make the existingmedia_library_widget
work with the new entity reference subclass (which, IMHO, is a more realistic scenario).Comment #11
phenaproximaComment #12
alexpottRe precedence and knowledge. The problem with adding the brackets is that then you're reinforcing the idea that there is a precedence issue here.
Comment #13
phenaproximaI personally find the precedence non-obvious, so I imagine others might feel similarly as well. But I'm certainly not gonna block on that. :) Totally a nitpick. 🤓 Feel free to disregard.
Comment #14
chr.fritschReverted the changed test and added a new one.
Comment #15
phenaproximaNo serious complaints here, but I'm wondering if we could clean up and/or speed up the test.
This change is unnecessary; MediaLibraryTestBase already has media_library_test in its $modules array.
Nit: should be "Tests".
Nit: extra blank line.
We don't need the entity_type or field_type properties here. We can just pass
'field_storage' => $field_storage_config
, as long as $field_storage_config is the thing we created just before this. Alternately, we could streamline this even more by using EntityReferenceTestTrait::createEntityReferenceField().Why do we need the
region
andsettings
properties?To be honest, I'm wondering if we shouldn't just make this a kernel test. It might more clearly allow us to reproduce the exception (and therefore prove the fix), and it would be faster.
Comment #16
phenaproximaHere's a patch with the same fix, but tests it by modifying an existing test. If this test fails for a field type-related reason, IMHO it will offer a clearer picture of what's going wrong. I'm open to keeping the functional JS test, though, if committers feel it's helpful.
Comment #17
seanBThe patch looks good to me. Can we add a fail patch to double check the test works as expected?
Comment #18
phenaproximaHere's a fail patch just to prove that the test covers what we think it does.
Comment #20
phenaproximaFail patch failed exactly as expected.
Comment #21
seanBLooks good to me. Nice!
Comment #22
alexpottCommitted 7064c43 and pushed to 9.4.x. Thanks!
Comment #24
alexpottI don't really think this is a feature. It's on the line between bug and task. We should always prefer interfaces over string IDs.
Comment #26
anthony.bouch CreditAttribution: anthony.bouch at Infonomic commented@phenaproxima would be grateful for an explanation as to why the credit for this issue was transferred from original issue that was raised by me in 2019? Wouldn't that make this issue the duplicate? And not mine?