Problem/Motivation

@amateescu reported a bug with the media library field widget in #2988617: Creating media with the media library upload is broken for unlimited cardinality:

I think it's worth noting that the target_bundles setting of the ER field has three states:

- NULL: all bundles are referenceable
- []: no bundle is referenceable
- ['some_bundle']: only some_bundle is referenceable

This is documented in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::defaultConfiguration, and maybe some logic of this patch needs to be updated based on this information.

In practice, target_bundles can only be NULL if a user has not configured their field, since it's a required field (see \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildConfigurationForm). Either way, the widget and upload form don't handle this case but probably should.

This bug has been unintentionally fixed by the hullaballoo of #3020716: Add vertical tabs style menu to media library, but we should really have a test in core to prevent it from regressing in the future.

Proposed resolution

Add a test of the NULL case for target_bundles and allow all types.

Remaining tasks

Commit the patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 2989503-11.patch6.16 KBseanb
#11 interdiff-6-11.txt3.08 KBseanb
#6 2989503-6.patch5.75 KBseanb

Comments

samuel.mortenson created an issue. See original summary.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

This has been fixed in #3020716: Add vertical tabs style menu to media library. We can close this issue when that lands.

balsama’s picture

balsama
he/him
Primary language English
Location boston
commented
Title: The media library widget cannot does not handle cases where target_bundles is NULL » Add tests to prove that the media library widget cannot does not handle cases where target_bundles is NULL
Category: Bug report » Task
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: Add tests to prove that the media library widget cannot does not handle cases where target_bundles is NULL » Add tests to prove that the media library widget works when target_bundles is NULL
Status: Active » Needs work
Issue tags: +Needs tests
amateescu’s picture

amateescu commented
Status: Needs work » Active

I think the NW status is when there's a patch in the issue, we don't have one here so it should be active :)

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Active » Needs review
StatusFileSize
new 2989503-6.patch5.75 KB

Added a field without target bundles with a test to show #3020716: Add vertical tabs style menu to media library fixed it.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: -Needs tests 
+++ b/core/modules/media_library/tests/modules/media_library_test/config/install/field.field.node.basic_page.field_empty_types_media.yml
@@ -0,0 +1,27 @@
+  handler_settings:
+    target_bundles: {  }

Maybe I'm missing something, but it looks like target_bundles is an array...but it should be explicitly null in order for this test to be effective.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.node.basic_page.default.yml
@@ -44,6 +44,12 @@ content:
+  field_empty_types_media:
+    type: media_library_widget

+++ b/core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_view_display.node.basic_page.default.yml
@@ -49,6 +49,15 @@ content:
+  field_empty_types_media:
+    type: entity_reference_entity_view

+++ b/core/modules/media_library/tests/modules/media_library_test/config/install/field.storage.node.field_empty_types_media.yml
@@ -0,0 +1,19 @@
+field_name: field_empty_types_media

    ✅ Configurable field for testing this with empty target_bundles. 

  2. +++ b/core/modules/media_library/tests/modules/media_library_test/config/install/field.field.node.basic_page.field_empty_types_media.yml
@@ -0,0 +1,27 @@
+    target_bundles: {  }

    ✅ Empty target_bundles. 

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -201,6 +201,7 @@ public function testWidget() {
+    $assert_session->pageTextContains('Empty types media');

    ✅ Verifies this field exists. 

  4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -222,6 +223,19 @@ public function testWidget() {
+    // Assert that the media type menu is available when no types are configured
+    // for the field. All types should be available in this case.
+    $assert_session->elementExists('css', '.media-library-open-button[href*="field_empty_types_media"]')->click();
+    $assert_session->assertWaitOnAjaxRequest();
+    $menu = $assert_session->elementExists('css', '.media-library-menu');
+    $assert_session->elementExists('named', ['link', 'Type One'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Two'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Three'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Four'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Five'], $menu);
+    $page->find('css', '.ui-dialog-titlebar-close')->click();
+    $assert_session->assertWaitOnAjaxRequest();

    ✅ This then tests that all bundles are visible.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Reviewed & tested by the community » Needs review

✅ Empty target_bundles.

Not quite. The issue title says that we need to prove it works if target_bundles is NULL, not an empty array. Ideally that is what the test we're adding here would address.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented 
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -222,6 +223,19 @@ public function testWidget() {
+    $menu = $assert_session->elementExists('css', '.media-library-menu');
+    $assert_session->elementExists('named', ['link', 'Type One'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Two'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Three'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Four'], $menu);
+    $assert_session->elementExists('named', ['link', 'Type Five'], $menu);

Nit: I think these assertions can all become $this->assertTrue($menu->hasLink('Foo')).

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
StatusFileSize
new interdiff-6-11.txt3.08 KB
new 2989503-11.patch6.16 KB

Fixed #9 / #10

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

RTBC once green. Thanks!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 0b6e897 and pushed to 8.7.x. Thanks!

  • alexpott committed 0b6e897 on 8.7.x 
    Issue #2989503 by seanB, phenaproxima, samuel.mortenson, amateescu, Wim...

Status: Fixed » Closed (fixed)

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