Notice: Undefined index: target_bundles when new reference media field created

Solved the problem for me, thanks!

All looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

But … target_bundles is a required setting:

So this shouldn't be necessary. How did you manage to set this up without configuring allowed target bundles?

Added steps to reproduce. Although the previous description was pretty brief, it was actually correct, this happens when creating the field. Expanded the IS a bit though, to make this more clear. Re #11, that's why this can occur, even though the field is required, because the very first you hit this form the field settings have not yet been saved.

We should add a test, though, before this can go in.

because the very first you hit this form the field settings have not yet been saved.

🤯 That sounds like a serious bug in Field UI, not Media? That would also explain many bug reports in JSON:API, to be honest, where JSON:API chokes on this required information not being set.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -152,7 +152,8 @@ protected function getAllowedMediaTypeIdsSorted() {
+    $allowed_media_type_ids = (isset($handler_settings['target_bundles']))
+      ? $handler_settings['target_bundles'] : NULL;

Can we remove the extra () around the isset() and put this on 1 line? Or better yet, in PHP7 we can also do this:
$allowed_media_type_ids = $handler_settings['target_bundles'] ?? NULL;

From a backend perspective, this seems quite minor. Media Library itself handles the various values of target_bundles quite gracefully as of #2989503: Add tests to prove that the media library widget works when target_bundles is NULL and #3033653: InvalidArgumentException when adding reference field without Media type, but I guess we missed an isset() check somewhere. If it's easy, go ahead and add a small test (if we're lucky, it could be a one-line change, asserting that the notice is not there when creating a new field).

Interestingly, when adding a new entity reference field...
If you select Reference > Media then this notice occurs
However, if you select Reference > Other... > Type of item to reference > Media, then this notice does not occur

Patch works for me and is badly needed IMHO. Otherwise everything is inundated with these stupid error messages.
#14 is right, of course. Modern PHP exists and should be used. I've adapted the patch accordingly.

The patch looks good and I can confirm it fixes the issue. We now only need a test to prove that no error messages are shown when adding media fields.

I'll see if I can write a test this morning.

Here's test coverage.

Removing an extraneous permission.

Thanks, looking good to me. One minor thing though.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -1582,4 +1582,47 @@ public function testWidgetOEmbed() {
+    $page->clickLink('Manage form display');
+    $this->assertSame('media_library_widget', $assert_session->selectExists('fields[field_shatner][type]')->getValue());
+    $assert_session->elementExists('css', '#field-shatner')->pressButton('Edit');
+    $this->assertTrue($settings = $assert_session->waitForElement('css', 'div[data-drupal-selector="edit-fields-field-shatner-settings-edit-form"]'));
+    $type_three_drag_handle = $assert_session->elementExists('css', 'tr[data-drupal-selector$="type-three"] .tabledrag-handle');
+    $type_two = $assert_session->elementExists('css', 'tr[data-drupal-selector$="type-two"]');
+    $type_three_drag_handle->dragTo($type_two);
+    $settings->pressButton('Update');
+    $this->assertTrue($assert_session->waitForText('Tab order: Type One, Type Two, Type Three'));
+    $page->pressButton('Save');
+    $assert_session->pageTextContains('Your settings have been saved.');
+    $assert_session->elementNotExists('css', '.messages--error');

We already have test coverage for this in MediaLibraryTest::testWidget().

I'm not against moving that coverage to the new testFieldUiIntegration method, but that other test also asserts the configured order is actually used in the widget, so that might be more complete.

I personally feel it's best to remove the duplicate test coverage for now, since it is not actually related to the bug we are trying to solve. We could have a follow-up to move some of the test coverage in testWidget, but I have no strong preference regarding that.

Oh bummer. I didn't see that test. I'll see if I can combine them and add the test coverage needed here to that one.

Hmm, looking. It looks like i can just drop the second part of my test.

Let's make the changes discussed above and add a new FAIL patch to move this along.

Attached are the removals mentioned by seanB.

I generated these from a fresh 8.8.x branch.

Edit: I must have had some issue with creating these. I'll generate new patches.

I goofed hard on the patch in #29. I removed those from the file list.

Here are new patches to
* update FAIL from 21 with the 23 interdiff and comments from seanB
* update Success from 23 patch with comments from seanB

I found nitpicks, but nothing that should block RTBC. Feel free to fix these if they're not too annoying!

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1582,4 +1582,35 @@ public function testWidgetOEmbed() {
   +    $this->drupalLogin($user);
   +    $this->drupalGet('/admin/structure/types/manage/article/fields/add-field');
   

   It'd be nice to split these bits up with a blank line, since the setting up and logging in is like the "prologue" of the test, and the real meat of it starts with the drupalGet().

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1582,4 +1582,35 @@ public function testWidgetOEmbed() {
   +    $page->fillField('label', 'Shatner');
   +    $this->assertTrue($assert_session->waitForText('field_shatner'));
   

   This assertion really...engages me. (See what I did there? Wrong captain, I know. :) 👍🤓

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1582,4 +1582,35 @@ public function testWidgetOEmbed() {
   +    $assert_session->elementNotExists('css', '.messages--error');
   

   I would change this, I think, to assert that the partial text of the error is not showing up. So, something like:

   $assert_session->pageTextNotContains('Undefined index: target_bundles');
   

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1582,4 +1582,35 @@ public function testWidgetOEmbed() {
   +    $page->checkField('settings[handler_settings][target_bundles][type_one]');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $page->checkField('settings[handler_settings][target_bundles][type_two]');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $page->checkField('settings[handler_settings][target_bundles][type_three]');
   +    $assert_session->assertWaitOnAjaxRequest();
   

   It's a shame we need to use assertWaitOnAjaxRequest(), which is not a particularly reliable method. However, there's no particular condition or element for us to wait for, so this is fine for now.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -146,7 +146,9 @@ protected function getAllowedMediaTypeIdsSorted() {
+    // The target bundles will be blank when saving field storage settings
+    // when first adding a media reference field.
+    $allowed_media_type_ids = $handler_settings['target_bundles'] ?? NULL;

I know @seanB asked for this, but I don't think we should use the ?? operator, since that means this patch will not be backportable to Drupal 8.7 (which, IMHO, it absolutely should be). So, ugly as it is, let's revert this to the isset() ? $foo : $bar syntax instead.

Fail patch failed. :)

Adding new fail patch to demonstrate this change will fail without bugfix:

-    $assert_session->elementNotExists('css', '.messages--error');
+    $assert_session->pageTextNotContains('Undefined index: target_bundles');

1. ✅ Capitalized AJAX.
2. 👍 I thought you’d appreciate that. I don’t see “Picard” in the code base yet. That’ll be in the next generation of patches.
3. updated to $assert_session->pageTextNotContains('Undefined index: target_bundles’);
4. I know you don't care for these vague assertions, we could test if the form_build_id changes, but that seems silly.

I know @seanB asked for this, but I don't think we should use the ?? operator, since that means this patch will not be backportable to Drupal 8.7 (which, IMHO, it absolutely should be). So, ugly as it is, let's revert this to the isset() ? $foo : $bar syntax instead.

Meh, but excellent point.

Rgarding #33.3
I think since $assert_session->elementNotExists('css', '.messages--error'); will also catch other error in the future, that might be a bit more robust?

Re:

I think since $assert_session->elementNotExists('css', '.messages--error'); will also catch other error in the future, that might be a bit more robust?

I think phenaproxima is right. What if we need to test error messages on the form, and it makes this test fail?

I guess we could change the test at that point. For now this would be more robust coverage that the form saves without error.

I think phenaproxima is right. What if we need to test error messages on the form, and it makes this test fail?

I think the normal path in this test should never produce an error, but I have no strong opinion on it. If we feel more comfortable with the more specific assert I could definitely live with it.

I think the normal path in this test should never produce an error

I agree with @seanB's rationale. However, the reason we should still change the assertion is because, if the error message area's CSS selector is ever changed to something besides .messages--error, this test will silently cease to test the regression. Using .messages--error gives us an accidental, unintentional dependency on front end-specific code. So in the name of specificity and future-proofing, we should change the assertion.

Oops. Should have been isset(), not !empty(), as phenaproxima said in #33.

I'm happy with this. RTBC when it's green.

+1 from me!

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -146,7 +146,9 @@ protected function getAllowedMediaTypeIdsSorted() {
     $handler_settings = $this->getFieldSetting('handler_settings');
-    $allowed_media_type_ids = $handler_settings['target_bundles'];
+    // The target bundles will be blank when saving field storage settings
+    // when first adding a media reference field.

The double 'when' is a bit hard to read, will add a comma after 'setting' on commit.

Committed c6a7ce5 and pushed to 8.8.x. Thanks!

I'm nominating the patch to be backported. I believe it also exists on 8.7.x, and although it's not "serious", per se, it's a big scary WTF for users.

Applied this patch to 8.7.7 after getting the nice red error :p

Works fine for 8.7.7 as wel. Tests also seems to be green. I don't know what needs to be done for a backport?

We don't need to do anything to backport this. Tests passed/failed on 8.7.x exactly as expected, so I don't think we're blocked.

8.7.x has been in a commit freeze for a couple of days, I think, so a committer will probably backport this to that branch when the freeze is done.

This seems fine for backport. (Note that "backport" isn't a tag we use or track.) Patch-eligible issues should just always be set to the production branch (currently 8.7.x). Separate tests can still be run against 8.8.x and 8.7.x if needed.

Cherry-picked to 8.7.x. Thanks!