Media fields with Media Library form widget trigger PHP 'Notice: Undefined index'

The notice is from core, but in your backtrace, I see that autosave is doing the submit. Moving this to autosave.

Autosave 2.x is doing a submit, but it is a regular submit and therefore the error is not related to autosave.

It just overrides the form builder but does not manipulate the submitted values or anything that might be causing such errors.

Moving back the issue.

Moving to core and media module ;). Probably best to give more info about all fields on the entity

This needs steps to reproduce, from a clean installation of Drupal.

I can sadly confirm this problem. Media library form dispay widgets can not be edited currently due to this bug.

Here's my backtrace (no Autosave used, I think Autosave is unrelated here):

31: Drupal\field_ui\Form\EntityDisplayFormBase->copyFormValuesToEntity() => array (2)
30: Drupal\Core\Entity\EntityForm->buildEntity() => array (2)
29: Drupal\Core\Entity\EntityForm->afterBuild() => array (2)
28: call_user_func_array() => array (1)
27: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
26: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
25: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
24: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
23: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
22: Drupal\Core\Form\FormBuilder->doBuildForm() => array (2)
21: Drupal\Core\Form\FormBuilder->rebuildForm() => array (2)
20: Drupal\Core\Form\FormBuilder->processForm() => array (2)
19: Drupal\Core\Form\FormBuilder->buildForm() => array (2)
18: Drupal\Core\Controller\FormController->getContentResult() => array (2)
17: Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult() => array (2)
16: call_user_func_array() => array (1)
15: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() => array (1)
14: Drupal\Core\Render\Renderer->executeInRenderContext() => array (2)
13: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() => array (2)
12: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() => array (1)
11: Symfony\Component\HttpKernel\HttpKernel->handleRaw() => array (2)
10: Symfony\Component\HttpKernel\HttpKernel->handle() => array (2)
9: Drupal\Core\StackMiddleware\Session->handle() => array (2)
8: Drupal\Core\StackMiddleware\KernelPreHandle->handle() => array (2)
7: Drupal\page_cache\StackMiddleware\PageCache->pass() => array (2)
6: Drupal\page_cache\StackMiddleware\PageCache->handle() => array (2)
5: Drupal\ban\BanMiddleware->handle() => array (2)
4: Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() => array (2)
3: Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() => array (2)
2: Stack\StackedHttpKernel->handle() => array (2)
1: Drupal\Core\DrupalKernel->handle() => array (2)
0: main() => array (2)

It happens for ALL fields on the entity so there are lots of errors messing up the form display page.

I think one very important information is, that it only happens for Media Entity References with Media Library widget. All other field form display settings (gears) work without this error!
If I switch the same field to Inline Entity Reference, the settings work. So I think it's very realistic that this error is caused by the Media Library widget in combination with ??something??

I'm courious if it has to do with layout_builder or perhaps with a messed up configuration... no idea yet how this could be caused.

**Workaround info:** With devel kint error (notice) display disabled I was at least able to set the fields as required.

This is a core problem in the Media module, and is not related to the enabling of Layout Builder. Here are steps to reproduce (done on a vanilla Drupal 9.1.8 site).

1. Enable the "Media Library" module.
1. Add a field of type "Media" to a content type. In the entity form display, set the display to "Media Library."
1. Add a second field of any type to the content type & save.
1. Click the form widget display gears for the field of type "Media".
1. The logs will show a PHP notice relating to the second field (and presumably any fields added subsequent to the Media field), reading Notice: Undefined index: field_test2 in Drupal\field_ui\Form\EntityDisplayFormBase->copyFormValuesToEntity()

Based on the above, I suggest we change the issue title to "Media fields with Media Library form widget trigger PHP 'Notice: Undefined index'"

Ran the steps to reproduce and I can confirm this happens. It didn't produce a visible error for me, but one did show up in the logs.

I'm editing the issue summary and will open a merge request with the fix soon.
Stay tuned :)

MR looks good. We just need test coverage.
MR 716 removes the gears of the media widget if one media type is allowed.
See images attached.

I think it's fine given there is nothing to configure in this case.

What kind of test coverage do we expect here?
I noticed there is MediaLibraryWidgetTest, which is a Kernel test.
Creating a functional test for this type of thing seems like overhead to me.

What kind of test coverage do we expect here?
I noticed there is MediaLibraryWidgetTest, which is a Kernel test.
Creating a functional test for this type of thing seems like overhead to me.

We need a test that causes PHP to produce the notice in question, so we can prove that your bug fix prevents the error from happening. It's not super important whether it's kernel or functional -- I'd actually prefer kernel in this case, if we can -- as long as its execution path is realistic and causes the notice to happen.

I think it's fine given there is nothing to configure in this case.

Not entirely sure about this. Is there a precedent in core already where a widget with nothing to configure doesn't show a gear icon? Or do all widgets show their gears, even if there are no settings? I ask because it it can be hard to get something committed if it deviates from existing UI patterns in core, even if those patterns are sub-optimal.

If there's not existing precedent, we might need UX team sign-off here.

EDIT: Looking at the screenshots @paulocs posted in #13, it appears that there are indeed other widgets which hide their gear in certain circumstances (see the "Authored on", "Comments", and "URL alias" fields in the before image). So, given that we have a precedent we can point to, I agree with you, @Matroskeen. Let's just get a test written, and land this thing!

Is there a precedent in core already where a widget with nothing to configure doesn't show a gear icon?

Yep, this is default Drupal core behavior:

Transferring credit from #3188588: EntityDisplayFormBase (Form Display) throws Notices when opening field settings for Media Library Widget.

Thanks for moving credits here!

I also checked other core widgets to make sure they're using settings form correctly - everything's fine (no follow-ups to be created).

Regarding the test coverage, we can add something for the media widget, but it feels that we should have general protection for every widget provided by Drupal core. We can simply check whether WidgetBase::settingsForm() is not modifying the input array, but creates a new one.

Do you think this test would be relevant?

Regarding the test coverage, we can add something for the media widget, but it feels that we should have general protection for every widget provided by Drupal core.

I'm not sure how we could protect against this generically for every possible widget, including widgets in contrib and custom code. But more to the point, that would significantly expand the scope of this issue and make it harder to commit...all to avoid what is, at the end of the day, a simple bug with a simple fix :)

The best thing to do here, I think, is to test and fix the bug for Media Library specifically. But by all means, please open a follow-up issue to discuss a generic guardrail if you feel strongly about it!

I'll write tests for it.

I added two lines in the functional js test.
I know #15 is asking for a functional or kernel test but maybe it is okay to add those lines in the functional JS test.
Please let me know.
Thanks!

I think adding those lines to the existing functional test is perfectly fine.

Is this a RTBC?

I need to do a bit of research into existing core code, and review the merge request, before I'd feel comfortable RTBCing this. Once I do have a moment, I'll update this issue's status so you'll know. :)

I think this is ready to go.

I did a little research into other core field widgets and discovered that, indeed, this is something Media Library has been doing incorrectly. It's kinda amazing that it didn't break anything else! I was originally thinking that renaming $form to $elements was out of scope, but given what other core widgets are doing, I think it makes sense. Other widgets are doing similar things.

The tests don't specifically check for the error message, but that's okay -- the proper fix here ensures that error should never show up anyway, and we are testing that the fix works as intended. So testing for the error message, or lack of it, would be beside the point. We could add a kernel test to prove that MediaLibraryWidget::settingsForm() doesn't return unexpected elements, but that seems a tad overzealous to me.

So full steam ahead, I say!

Thanks for working on this folks.
I've been caught by similar issues in custom code, including one that ended up in an endless loop.

Can we get a fail/pass patch here (yes I realise this is an MR) to demonstrate that the test covers the bug we're trying to resolve.

Thanks

Saving issue credit for @phenaproxima who has provided reviews that shaped the final patch

@larowlan: I was initially going to ask for a fail patch too, but I realized that the actual proper fix here is to remove the very UI element which would allow the error to be triggered in the first place. Given that, I'm not sure how we could do a fail patch, since the condition that could cause the notice is completely eliminated.

So, tentatively restoring RTBC here, but please do send this back if I've missed something.

I tested this locally, and was able to reproduce it.
My log was full of such errors.
I then applied the patch, and it worked as promised.

Looking at the test, it should fail without the patch, because the button will be present, even though there's no form to edit.

So here's fail/pass patches to confirm

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!