Allow field types extending entity_reference to use field widgets extending media_library

Transferring credit from #3179868: Make MediaLibraryWidget more flexible, which I closed as a duplicate of this one.

I think I'm okay with this idea, but two things come to mind:

1. We'll need automated test coverage of this. A module that provides a fake field type that extends EntityReferenceFieldItemList, I'd say, and a test that proves the media library can open it (or at least that checkAccess() permits it).
2. I think we will want to specifically ensure that the item list extends EntityReferenceFieldItemList, rather than its interface. The reason I say that is because we are also checking a specific setting (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.

Added a test and changed to check for EntityReferenceFieldItemList

That 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) to if (!($items instance of EntityReferenceItemList))? I ask because it's not immediately clear if the ! is evaluated first, or the instanceof. (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 existing media_library_widget work with the new entity reference subclass (which, IMHO, is a more realistic scenario).

Re precedence and knowledge. The problem with adding the brackets is that then you're reinforcing the idea that there is a precedence issue here.

I 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.

Reverted the changed test and added a new one.

No serious complaints here, but I'm wondering if we could clean up and/or speed up the test.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -14,7 +15,7 @@ class EntityReferenceWidgetTest extends MediaLibraryTestBase {
   -  protected static $modules = ['field_ui'];
   +  protected static $modules = ['field_ui', 'media_library_test'];
   

   This change is unnecessary; MediaLibraryTestBase already has media_library_test in its $modules array.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -480,4 +481,61 @@ public function testRequiredMediaField() {
   +   * Test inserting media items from the library into subclasses of ER fields.
   

   Nit: should be "Tests".

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -480,4 +481,61 @@ public function testRequiredMediaField() {
   +  public function testMediaLibraryWithEntityReferenceSubclassField() {
   +
   +    $display_repository = $this->container->get('entity_display.repository');
   

   Nit: extra blank line.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -480,4 +481,61 @@ public function testRequiredMediaField() {
   +    FieldConfig::create([
   +      'label' => 'A Media Image Field',
   +      'field_name' => 'media_image_field',
   +      'entity_type' => 'node',
   +      'bundle' => 'basic_page',
   +      'field_type' => 'entity_reference_subclass',
   +      'required' => TRUE,
   +      'settings' => [
   +        'handler_settings' => [
   +          'target_bundles' => [
   +            'type_one' => 'type_one',
   +          ],
   +        ],
   +      ],
   +    ])->save();
   

   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().

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -480,4 +481,61 @@ public function testRequiredMediaField() {
   +        'region' => 'content',
   +        'settings' => [
   +          'media_types' => ['type_one'],
   +        ],
   

   Why do we need the region and settings properties?

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -480,4 +481,61 @@ public function testRequiredMediaField() {
   +    $user = $this->drupalCreateUser([
   +      'access media overview',
   +      'edit own basic_page content',
   +      'create basic_page content',
   +      'view media',
   +    ]);
   +    $this->drupalLogin($user);
   +
   +    $this->drupalGet('node/add/basic_page');
   +    $this->openMediaLibraryForField('media_image_field', '#media-library-wrapper');
   +    $this->selectMediaItem(0);
   +    $this->pressInsertSelected('Added one media item.');
   

   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.

Here'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.

The patch looks good to me. Can we add a fail patch to double check the test works as expected?

Here's a fail patch just to prove that the test covers what we think it does.

Fail patch failed exactly as expected.

Looks good to me. Nice!

Committed 7064c43 and pushed to 9.4.x. Thanks!

I 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.

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