Media library causes validation errors when it is used in a required field of a nested form

I've been able to determine that this is happening when using the 'Paragraphs Classic' form field widget (on Manage Form Display tab). When using the 'Paragraphs EXPERIMENTAL' form field widget, the error does not appear for me.

I have the same issue. Using Paragraphs Experimental does not fix the issue. In my case, removing the 'required' on the Media Image field removes the error. So I'm thinking it's related to a deeper Ajax issue in D8 with required fields. See these related posts

https://www.drupal.org/project/drupal/issues/2614250
https://www.drupal.org/project/drupal/issues/2553983
https://www.drupal.org/project/drupal/issues/2027059
https://www.drupal.org/project/drupal/issues/2027059

F

Setting major as this breaks a common use case on a lot of existing sites.
One can even see this critical as making a field non-required is no general workaround

That dragon already was at IEF:
* #2618502: IEF Widgets are completely broken
* #2580857: "This value should not be null" when saving a form

Tagging as part of the Media Initiative, until this is further triaged.

I can confirm #2

• node with paragraphs field using the 'Paragraphs EXPERIMENTAL' form field widget
• with image paragraph with required media reference field to media entity with whatever field

works.

I can confirm #3

• node with paragraphs field using the 'Paragraphs Classic' form field widget
• with image paragraph without required media reference field to media entity with whatever field

works.

Same as hanness, I confirm that with # 2 and # 3 it works correctly.
It seems validation error with ajax forms.

I think this would be a good thing to write a test for, if possible, before we set about fixing it.

I don't see a patch so i guess NW status was accidental.

I would like to add that 'Paragraphs EXPERIMENTAL' doesn't solve all the issues, for example, if you have a paragraph being referenced into another paragraph. See below the scenario that I have:

- People: image and title
- People list: people, title and subtitle
- Article: people list and other paragraphs

I've changed People in the People list to use EXPERIMENTAL widget and I'm also using EXPERIMENTAL in the Article content type. When I'm adding People list paragraph to the Article I face the 'This value should not be null' message.

I can confirm issue described in #12. I'm using the experimental widget and have had no issues when the media field is on a paragraph referenced by a node. However, when it's a paragraph within a paragraph, then the validation error happens.

It seems that the validation call "$display->validateFormValues" inside ParagraphsWidget (Experimental) and InlineParagraphsWidget (Classic) causes the fields to validate. This validation takes the items from subform (the fields concerned) and validates them. Since the field is required and no value is set at this moment, the error message is displayed.
This might be a bug in Paragraphs, or this might be the expected behavior referencing to a shortage inside the Media library module.
Perhaps all of this can be replaced by a solution that ensures the target_id exists inside the element selection upon validation.

Anyhow, I've added a patch that should solve the issue for now, although it would be nice to get some validation on the security implications.
Inside MediaLibraryWidget I've added a function to remove the error message of the validation and I've added an element validation to capture the required field logic.

Additionally I've had to create a new feature inside the FormState of Drupal Core because it currently does not allow to remove a single element from the form_state errors. A workaround by removing all errors and adding the remaining errors back is also not an option because of the check on $limit_validation_errors inside the FormState class on setErrorByName.

I tested #14 patch after fixing CodeSniffer errors, and it didn't work for me, although the patch applies well.
I opened an issue on paragraphs module 20 days ago: https://www.drupal.org/project/paragraphs/issues/3013035

There is still an issue with the implementation of some abstract classes implementing the FormBaseInterface.
You can fix this by using clearErrors without a parameter and removing the code for clearError from the interface and instance.

I'm doubtfult about originating this issue to Paragraph since this follows a basic validation pattern that is required to be able to valudate the subform fields.
I think the solution resides in providing the value to the form as a validation friendy input field after passing it from the views ( to ensure the target id and optional weight exist and are not empty before the automated submit button trigger, allowing the validation pattern to not throw errors anymore ).

If I can find some time later this week I'll submit an updated patch.

I've updated the patch to remove the clearError mechanism and integrated an exception to avoid the validation errors that were previously deleted in a later validation stage.
There is still an exception inside validateElement to capture the empty target_id. Maybe there is another way around this, or maybe this is an indication of a bad validation logic on the Paragraphs side. Anyhow, in between time this patch should do the trick.

Note that I also apply the patch from https://www.drupal.org/node/2663316 , since without this patch, you won't be able to test this.

The patch works for me. Thanks!

This patch is also working for me... Sweeeeeet.

Patch at #19 is a complete fix for me. Furthermore, by applying the patch to upload a new image then reverting back to core without the patch before saving the node I am trigger these warnings:

Warning: call_user_func_array() expects parameter 1 to be a valid callback, class 'Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget' does not have a method 'validateElement' in Drupal\Core\Form\FormValidator->doValidateForm() (line 282 of core/lib/Drupal/Core/Form/FormValidator.php).
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 238)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'node_regional_landing_page_edit_form') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('node_regional_landing_page_edit_form', Array, Object) (Line: 575)
Drupal\Core\Form\FormBuilder->processForm('node_regional_landing_page_edit_form', Array, Object) (Line: 318)
Drupal\Core\Form\FormBuilder->buildForm('node_regional_landing_page_edit_form', Object) (Line: 93)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 51)
Drupal\jsonapi\StackMiddleware\FormatSetter->handle(Object, 1, 1) (Line: 84)
Drupal\shield\ShieldMiddleware->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 669)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Given that this patch establishes the missing validateElement method this seems like a full fix.

Looks like we still don't have tests yet...

+1. We can't fix this without tests of some sort. Otherwise, any time the media library undergoes refactoring (and some heavy refactoring is coming), we could break integration with Paragraphs again. Sorry!

I would like to mention that the patch I've created is a workaround to handle the missing target_id upon the submit validation of the media target id's returned from the view modal.

I think the best/cleanest solution is to rewrite the media library widget to not use the field "media_library_update_widget" anymore.
Instead we would have to alter the form_state of the widget and insert the target_id's that are returned from the Views modal (or add the instances), and then update the form using Ajax to contain the values. (or a simular flow).
The problem in the current flow is the absence of access to the form_state of the widget from the views handling, that's why the value is passed to the placeholder field called "media_library_update_widget".

In short:
We need the logic that the entity reference field uses right now to add multiple references to a field, and link the views output in between.

In the above described scenario, I believe we don't need a test because the implementation will follow the default entity reference logic, which will result in applying the tests from this module to ensure the value of the target id. The missing target Id also references to the fact that this field is required and the value is empty, for which a test also already exists.

As soon as I can find the time, I'll have a look into refactoring the widget to take this into account.

I appreciate the work you folks are putting into this bug, but please don't refactor the media library.

The core media team is already planning to do some heavy refactoring and redesign work for the media library in 8.7, and that work may well render this bug obsolete, or otherwise change the nature of it. Refactoring the media library now would complicate this process and possibly delay this bug for a long time.

We already consider this issue to be a stable blocker for Media Library, so we are going to get it fixed, but please do not completely refactor it now. It would be a lot more helpful to simply write a test for the proposed fix in #25. As a rule, we don't remove test coverage from core, so if we just add tests, we could be totally certain that whatever we release in 8.7 will not have this bug, even as we refactor the hell out of it.

The patch still does not work for me, I always have the following error message : "This value should not be null".

Implementing #element_validate as done in #19 does fix the issue. It seems hard to write a test reproducing the issue since it depends on contrib functionality (ief or paragraphs). Any thoughts on how a test for issues such as this one looks like?

Any thoughts on how a test for issues such as this one looks like?

Sure! :) Whatever is causing this bug is probably being triggered by a very specific code path within Paragraphs. If we can figure out exactly what that code path is, we could easily create a test module containing a new form which causes a similar code path to run, thus triggering the error, which would in turn allow us to write an automated test of it.

It seems like #14 came closest so far to explaining the exact reason why the bug happens. Can we build a new form, without using Paragraphs, which would reproduce those conditions? The code doesn't have to be "good"; it just has to do what it takes to cause the bug to manifest itself.

If that's too complicated, a possible alternative approach might be to add the test to Paragraphs itself, which would then cause Paragraphs to fail tests until this is committed to core. We could probably then mark this issue for manual testing. However, that means Paragraphs would be "broken" for an indeterminate amount of time, which kind of sucks. Additionally, that would not prevent core from accidentally regressing. So overall, I think it's better for us to test this in core if it's feasible to do so.

With Drupal 8.7 Beta 1, I am seeing similar problems top those mentioned in the opening post;

the image is never successfully added in the form and as a result, can't be saved.

However I am not seeing any validation errors.

I'll leave updating the core version to someone with more understanding, in case my experience is unrelated to this issue.

Work for me #19

Also getting this error when using the paragraphs browser module

Also getting this error when using the paragraphs browser module

The patch in #19 works for me with core 8.7.0, but I needed to also apply the patch from #3013171: LogicException Form errors cannot be set after form validation has finished.

Are you sure this patch is needed at all when using the paragraphs patch? We still need steps to reproduce there, because it worked fine for me with a required entity browser media field/widget, so maybe it is specific to core media library (which is afaik still experimental).

Yes, it is specific to core media library.

Re-rolled the patch from #19 for 8.7.1.

Setting back to Needs Review & triggering bots.

Looks good at a glance, but unfortunately we will still need tests of some sort. :(

Patch in #38 fixed this for me, also had to clear cache after applying the patch.

I was not seeing any errors or messages (except when re-opening the add media modal, then I did see the mentioned message)
Makes it kinda hard to find this issue!

#38 patch fixed one issue where I had a required image media field in a paragraph with other required fields as well (title, text)

However in another paragraph type where I only have a required media field, I get the following error:

jquery.min.js?v=3.2.1:4 POST http://mysite/node/add/page?ajax_form=1&_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax 500 (Internal Server Error)
send @ jquery.min.js?v=3.2.1:4
ajax @ jquery.min.js?v=3.2.1:4
e.fn.ajaxSubmit @ jquery.form.js:337

Drupal.Ajax.eventResponse @ ajax.js?v=8.7.2:330
(anonymous) @ ajax.js?v=8.7.2:269
dispatch @ jquery.min.js?v=3.2.1:3
q.handle @ jquery.min.js?v=3.2.1:3
ajax.js?v=8.7.2:500 Uncaught Drupal.AjaxError {message: "↵An AJAX HTTP error occurred.↵HTTP Result Code: 50…\Core\DrupalKernel->handle(Object) (Line: 19)↵", name: "AjaxError"}

When removing the required from the media field it works again.
When making it required it breaks again.

The paragraph where it breaks has the media field, tried both image & video, both broke
It also has a few other non-required text fields and a link field

#38 worked for me even without applying the patch from #3013171: LogicException Form errors cannot be set after form validation has finished

@Pascal- I've tried to reproduce the error described by you in #42 and didn't manage to do it. Could you provide more details on that? And it's not clear from the error what is the actual problem.

#38 worked for me.
I tried to duplicate @pascal problem by creating a paragraph with only a media field that references both image and video media types with no success in duplication.

I can not get #38 to apply clean on Drupal 8.7.3. Will update the patch if I get some time tonight.

Edit: Disregard, human error :)

I have no mandatory fields and the following structure:

Node -> Entity reference revisions (Paragraphs EXPERIMENTAL form widget) -> Entity reference (Media library form widget).

Before applying #13 from #3013171: LogicException Form errors cannot be set after form validation has finished I wasn't able to even open the popup. Now I cannot pick any media item because of error, similar to #42.

Drupal.AjaxError#message:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /admin/content/media-widget?destination=/admin/content&ajax_form=1&_wrapper_format=drupal_ajax&media_library_opener_id=field%3Afield_admin_dashboard_link_image-field_admin_dashboard_link-0-subform&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&hash=2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU
StatusText: OK
ResponseText: 
Media | SANITIZED_PROJECT_TITLE
Sorry, you need to enable JavaScript to visit this website.
Skip to main content
Toolbar items
Back to site
Manage
Administration menuDashboardContentStructureAppearanceExtendConfigurationPeopleReportsHelp      
Edit
SANITIZED_USER_NAME
User account actionsView profileEdit profileLog out      
Subscription not active
Media
Breadcrumb
Home
Administration
All content
Published
- Any -PublishedUnpublished
Name
Media type
- Any -FileImage
Sort by
Newest firstName (A-Z)Name (Z-A)
Wed, 06/26/2019 - 08:40
SANITIZED_USER_NAME
content.png
Select content.png
Wed, 06/26/2019 - 08:29
SANITIZED_USER_NAME
my-resources.png
Select my-resources.png
Wed, 06/26/2019 - 08:29
SANITIZED_USER_NAME
account-groups.png
Select account-groups.png
Wed, 06/26/2019 - 08:28
SANITIZED_USER_NAME
user-management.png
Select user-management.png
Wed, 06/26/2019 - 08:28
SANITIZED_USER_NAME
user-access.png
Select user-access.png
Wed, 06/26/2019 - 08:27
SANITIZED_USER_NAME
resource-manager.png
Select resource-manager.png
Wed, 06/26/2019 - 08:26
SANITIZED_USER_NAME
daily-stock.png
Select daily-stock.png
Wed, 06/26/2019 - 08:25
SANITIZED_USER_NAME
products.png
Select products.png
Test file
Select Test file
{"ajaxPageState":{"theme":"adminimal_custom","theme_token":"G3lt0L2ZT-kzJx-LotjMVzTVv_bfJviJOyFzd5M1FUI","libraries":"acquia_connector\/acquia_connector.icons,adminimal_custom\/global-styling,adminimal_theme\/global-styling,adobe_analytics\/tag_manager,autologout\/drupal.autologout,basic_cart\/basic_cart,classy\/base,classy\/file,classy\/messages,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/drupal.autocomplete,core\/drupal.collapse,core\/drupal.date,core\/drupal.states,core\/drupal.tabledrag,core\/drupal.tableresponsive,core\/html5shiv,core\/normalize,file\/drupal.file,media_library\/ui,media_library\/widget,node\/drupal.node,paragraphs\/drupal.paragraphs.actions,paragraphs\/drupal.paragraphs.widget,path\/drupal.path,seven\/drupal.nav-tabs,seven\/global-styling,seven\/node-form,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,user\/drupal.user.icons,views\/views.ajax,views\/views.module"},"autologout":{"timeout":7200000,"timeout_padding":20000,"message":"Your session is about to expire. Do you want to reset it?","redirect_url":{},"title":"SANITIZED_PROJECT_TITLE Alert","refresh_only":true,"no_dialog":false,"use_alt_logout_method":false},"views":{"ajax_path":"\/views\/ajax?destination=\/admin\/content\u0026ajax_form=1\u0026_wrapper_format=html\u0026media_library_opener_id=field%3Afield_admin_dashboard_link_image-field_admin_dashboard_link-0-subform\u0026media_library_allowed_types%5Bimage%5D=image\u0026media_library_selected_type=image\u0026media_library_remaining=1\u0026hash=2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU","ajaxViews":{"views_dom_id:798c02a139596adf50df0be799a466425c10eff4406a3fc8d31fe447d0988f6d":{"view_name":"media_library","view_display_id":"widget","view_args":"","view_path":"\/admin\/content\/media-widget","view_base_path":"admin\/content\/media-widget","view_dom_id":"798c02a139596adf50df0be799a466425c10eff4406a3fc8d31fe447d0988f6d","pager_element":0}}},"ajaxTrustedUrl":{"\/admin\/content\/media-widget?destination=\/admin\/content\u0026ajax_form=1\u0026_wrapper_format=html\u0026media_library_opener_id=field%3Afield_admin_dashboard_link_image-field_admin_dashboard_link-0-subform\u0026media_library_allowed_types%5Bimage%5D=image\u0026media_library_selected_type=image\u0026media_library_remaining=1\u0026hash=2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU":true,"\/admin\/content\/media-widget":true},"ajax":{"edit-submit--gphkVMV5Tps":{"url":"\/admin\/content\/media-widget?destination=\/admin\/content\u0026ajax_form=1\u0026_wrapper_format=html\u0026media_library_opener_id=field%3Afield_admin_dashboard_link_image-field_admin_dashboard_link-0-subform\u0026media_library_allowed_types%5Bimage%5D=image\u0026media_library_selected_type=image\u0026media_library_remaining=1\u0026hash=2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU","callback":"::updateWidget","event":"mousedown","keypress":true,"prevent":"click","dialogType":"ajax","submit":{"_triggering_element_name":"op","_triggering_element_value":"Insert selected"}}},"toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"BkLhufRu588ocIxM9XGfl_ACVJ6eSQcnMv6ihbL4SOI"},"path":{"baseUrl":"\/","pathPrefix":"","currentPath":"admin\/content\/media-widget","currentPathIsAdmin":true,"isFront":false,"currentLanguage":"en","currentQuery":{"_wrapper_format":"html","ajax_form":"1","destination":"\/admin\/content","hash":"2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU","media_library_allowed_types":{"image":"image"},"media_library_opener_id":"field:field_admin_dashboard_link_image-field_admin_dashboard_link-0-subform","media_library_remaining":"1","media_library_selected_type":"image"}},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"23ad9f1813933a9d2750914716a8ddf555f80493dea037a0e1aadcd1ea52205a"}}

This happens after hitting the Insert selected button. The request goes to /admin/content/media-widget?destination=/admin/content&ajax_form=1&_wrapper_format=drupal_ajax&media_library_opener_id=field%3Afield_admin_dashboard_link_image-field_admin_dashboard_link-0-subform&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&hash=2c-YqAy5WYYkCs4qq0dH76uqKFS8poTTW74NkE6v0CU&_wrapper_format=drupal_ajax and the response is what you see in the error message.

The problem here is that Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm::viewsForm() defines this button with AJAX handler that never triggers:

    $form['actions']['submit']['#ajax'] = [
      'url' => Url::fromUserInput($url),
      'options' => [
        'query' => $query,
      ],
      'callback' => [static::class, 'updateWidget'],
    ];

    $form['actions']['submit']['#value'] = $this->t('Insert selected');

Drupal 8.7.3 with the following patches applied (apart of #38):

1. https://www.drupal.org/files/issues/2019-05-16/drupal-media-library-para...
2. https://www.drupal.org/files/issues/2019-06-19/2745797-72--8.6.x.patch
3. https://www.drupal.org/files/issues/2019-05-02/2663316-70.patch
4. https://www.drupal.org/files/issues/2019-04-24/2877994-118.patch

Here's a test - the test only patch doubles as the interdiff, as everything else in the main patch is from #38

Forgot the @group annotation.

I've just copied media_library from current 8.8.x branch to the my setup with 8.7.3, cleared the cache and it worked.

After a couple of manual tests I confirm the media_library from 8.8.x (commit da6392f87813a9db14550c635cc61ec78e68d67c) allows to select existing media on 8.7.3.

Here's the patch for 8.7.3 to update media_library to latest (for now) version.

Changing issue to current dev version of core

Same as the patches in #48 but reuploading to test against 8.8.x

#54 unblocked choosing a file from media library for the required field, so This value should not be null. no longer appears.

Now I have the following patches in my composer.json to make everything work on Drupal 8.7.3:

{
    "extra": {
        "patches": {
            "drupal/core": {
                "#3003150-52: Use `media_library` from 8.8.x": "https://www.drupal.org/files/issues/2019-07-03/3003150--8.7.3--8.8.x-da6392f87813a9db14550c635cc61ec78e68d67c.patch",
                "#3003150-54: Media library widget + Paragraphs broken: validation error on required field in paragraphs subform": "https://www.drupal.org/files/issues/2019-07-03/3003150-54.patch"
            },
            "drupal/paragraphs": {
                "#3013171-13: LogicException Form errors cannot be set after form validation has finished": "https://www.drupal.org/files/issues/2019-05-03/3013171-13.patch"
            }
        }
    }
}

This is not quite ready for RTBC :) It needs review and sign-off from a subsystem maintainer (myself or @seanB). But don't worry, we'll get this done!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -320,6 +320,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#element_validate' => [[static::class, 'validateElement']],
   

   Proposition: rename the method to validateFormElement since it validates the element defined in formElement.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +   * @param $element
   

   Add the array type.

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +   *  The form element.
   

   Drupal CS: two spaces required.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +   *  The form state.
   

   Drupal CS: two spaces required.

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +   * @param array $form
   +   *  The form array.
   

   This parameter needs to be removed since not used.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +  public static function validateElement($element, FormStateInterface $form_state, array $form) {
   +    $media = static::getNewMediaItems($element, $form_state);
   

   Can we add the array type for $element? The static::getNewMediaItems() has it.

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      && $element['#required'] === TRUE
   

   Let's use !empty($element['#required']) instead, because the #required key might not be defined.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      && !in_array([static::class, 'removeItem'], $form_state->getSubmitHandlers())) {
   

   The !in_array([static::class, 'removeItem'], $form_state->getSubmitHandlers()) duplicates two times per method. It can be the main condition.

       if (!in_array([static::class, 'removeItem'], $form_state->getSubmitHandlers())) {
         $media = static::getNewMediaItems($element, $form_state);
         // Mark the field as required if there is no existing or new media
         // and this does not concern a remove call.
         if (empty($media) && !isset($element['selection'][0]) && !empty($element['#required'])) {
           $form_state->setError($element, \Drupal::translation()->translate('Please select an item from the media library.'));
         }
         else {
           $field_state = static::getFieldState($element, $form_state);
   
           // Ensure 1 target_id exists to avoid future validation errors.
           if (count($field_state['items']) === 0) {
             foreach ($media as $media_item) {
               // Any ID can be passed to the widget, so we have to check access.
               if ($media_item->access('view')) {
                 $field_state['items'][] = [
                   'target_id' => $media_item->id(),
                 ];
               }
             }
           }
           static::setFieldState($element, $form_state, $field_state);
         }
       }
   

@phenaproxima, the RTBC was set in a fit of joy, because #54 works.

No judgment here! I'm the undisputed king of promiscuous RTBCs, so I totally understand the feeling. Now that we have test coverage (and, thanks you to and others in this issue, manual testing and confirmation that it works), I think this will probably be quite straightforward to land; we just gotta put it through its paces. :)

#54 works.

Drupal 8.7.5
Paragraph: 8.x-1.x-dev

Hi,

I just test #57 patches list on Drupal 8.7.5 + Paragraph 8.x-1.8 and I have an error :

git apply 3003150--8.7.3--8.8.x-da6392f87813a9db14550c635cc61ec78e68d67c.patch --check
fatal: corrupt patch at line 2439

To fix this, a blank line between 2438 and 2439 is needed :

+

}

Patch #54 works on Drupal 8.7.5 and Paragraph 8.x-1.8. Thanks

Nice work so far. Let's get this thing done!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -542,7 +543,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // Exclude other validations in case there is no data yet.
   +      '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],
   

   Let's rephrase this comment. Maybe something like "Only run validation if there is data (i.e., referenced entities) to validate."

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +  /**
   +   * Validation checks on the element level.
   

   This method contains very complex validation logic, so I think we need a longer comment explaining what we're doing here and, more importantly, why. Otherwise this code is probably going to make some future maintainer very unhappy.

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +  public static function validateElement($element, FormStateInterface $form_state, array $form) {
   

   $element should be type hinted as an array, here and in the doc comment.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      && !isset($element['selection'][0])
   

   Wouldn't empty($element['selection']) be equivalent here?

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +746,47 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      && !in_array([static::class, 'removeItem'], $form_state->getSubmitHandlers())) {
   

   This seems a little awkward. Would it work for us to attach this method instead as a #validate handler on every button except the remove button?

#59.1 ✅ validateElement to validateMediaWidget since that is what gets the validation
#59.2 ✅
#59.3 ✅
#59.4 ✅
#59.5 ❌ $form isn't used but it is sent by the callback process so it can't be removed
#59.6 ✅
#59.7 ✅
#59.8 ✅ solution is different than what was suggested, but the redundant check is no longer present.
#64.1 ✅ This and pretty much every comment has been thoroughly rewritten.
#64.2 ✅
#64.3 ✅
#64.4 ❌

Wouldn't empty($element['selection']) be equivalent here?

it's not equivalent but it looks enough like that would be the case that I added a comment explaining why so it doesn't result in the patch getting kicked back.
#64.5 ❌

This seems a little awkward. Would it work for us to attach this method instead as a #validate handler on every button except the remove button?

This won't work (as far as I know) as the validators being triggered are assigned to the element, not the submit element. This has been refactored and commented to make things clearer. Its still possible a better approach exists but perhaps this will help make it more apparent how that would be implemented.

Also Removed some junk from the previous patch that shouldn't have been there - largely stuff related to local testing.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -747,44 +752,56 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
+      $field_state['items'] = array_filter(array_map(function ($media_item) {
+        return $media_item->access('view') ? ['target_id' => $media_item->id()] : NULL;
+      }, $media));

Nit: This is a bit dense. I think this might be a bit more readable:

$media = array_map(...);
$field_state['items'] = array_filter($media);

That's my only complaint :)

Made the change requested in #66 and had to make an adjustment to address the fails in MediaLibraryTest as some parent validators make assumptions about the presence of media even if the field is not required. Comment was also updated to reflect this.

Thanks, @bnjmnm. You are my hero. RTBC once testbot agrees.

I'm nominating this patch for backport to 8.7.x, since it is a bug fix in an experimental module.

Looks good, a few questions

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +751,59 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +    if (in_array([static::class, 'removeItem'], $form_state->getSubmitHandlers())) {
   

   I think we should be using the third argument here, because of php's known type juggling issues with in_array - see https://3v4l.org/tAfWE

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +751,59 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +    if (empty($media) && empty($element['selection'][0]) && !empty($element['#required'])) {
   

   Should we be using \Drupal\Core\Render\Element::isEmpty here instead of the semi-magic 0?

   Or failing that, should we be using \Drupal\Core\Render\Element::children instead of hard-coding 0?

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +751,59 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      $form_state->setError($element, \Drupal::translation()->translate('The field @field_name requires an item from the media library.',
   +        ['@field_name' => $field_name]));
   

   should we use `new TranslatableMarkup()` here instead of the \Drupal singleton?

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +751,59 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +      $media_target_ids = array_map(function ($media_item) {
   +        return $media_item->access('view') ? ['target_id' => $media_item->id()] : NULL;
   +      }, $media);
   

   is there a notion of selection handler plugins here? should we be calling the relevant \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::validateReferenceableEntities instead in case someone wants additional logic on top of the view access check?

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -744,6 +751,59 @@ public static function openMediaLibrary(array $form, FormStateInterface $form_st
   +    static::setFieldState($element, $form_state, $field_state);
   

   we're calling this every time, even if we don't change anything? should we only do this inside the if?

6. +++ b/core/modules/media_library/tests/modules/media_library_test_widget/src/Plugin/Field/FieldWidget/MediaLibraryInceptionWidget.php
   @@ -0,0 +1,56 @@
   + *   description = @Translation("Puts a widget in a widget for testing purposes"),
   

   ⭐️ needs more widget 😉

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EmbeddedFormWidgetTest.php
   @@ -0,0 +1,107 @@
   +      if ($extension === 'jpg') {
   +        $jpg_image = $image;
   

   nit: should we break here once we've found one, no need to keep looping?

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EmbeddedFormWidgetTest.php
   @@ -0,0 +1,107 @@
   +    $this->assertTrue($assert_session->waitForText($jpg_image->filename));
   

   should we also add some negative assertions here that we don't see the additional validation errors that spawned this issue?

Responding to the feedback in #71:

1. ✅Added third argument to in_array()
2. ✅::isEmpty doesn't work here because it doesn't check for the presence of numeric keys. It only checks if the array is empty or it only contains the #cache key. It would give a false negative based on other keys that start with '#'. I added the call to ::children, which is less efficient but is more readable and clearer about what is going on. I think checking for the key [0] was fine though. I debated leaving it as it was.
   

   -    //
   -    // Note that the empty() is checking $element['selection'][0] and not
   -    // $element['selection'] because $element['selection'] is a complete render
   -    // array. Specifically checking for $element['selection'][0] checks if a
   -    // value is present.
   -    if (empty($media) && empty($element['selection'][0]) && !empty($element['#required'])) {
   -      $field_name = $element['#title'];
   -      $form_state->setError($element, \Drupal::translation()->translate('The field @field_name requires an item from the media library.',
   -        ['@field_name' => $field_name]));
   +    $selection_count = !empty($element['selection']) ? count(Element::children($element['selection'])) : 0;
   +    if (empty($media) && $selection_count === 0 && !empty($element['#required'])) {
   

3. ✅switched to using TranslatableMarkup
4. Still @todo. I haven't investigated this yet.
5. ✅moved the call inside the loop. I'm not sure how this would affect the error message, if set. Also there isn't test coverage yet for the new error message added in validateMediaWidget.
6. I'm not sure how to add more widget, but I did add a period at the end.
7. ✅Added break after jpeg found.
8. I haven't found a way to get the error message to display in the fail patch without refreshing the page, and we won't want to do that in the actual test. I think it's OK without the error message in in the fail patch, as the assertion that the filename appears means the error didn't occur.

Marking as needs work. I believe it introduces a bug. I'm not certain, as it's using the MediaLibraryInceptionWidget.

With the patch, and using the inception widget, with these steps:

• Select an item
• Use the remove button in the upper right hand corner to remove it
• Select the item again

Expected: Media inserts again.
Actual: It doesn't insert.

https://www.drupal.org/files/issues/2019-08-07/3003150-regression-720.mov

Nice catch, @oknate! ⚾️I think we should add expanded test coverage for that, since in theory MediaLibraryInceptionWidget should be able to do everything the normal one can do. We should also be sure that, somewhere, we have test coverage for MediaLibraryWidget which proves that

Select an item
Use the remove button in the upper right hand corner to remove it
Select the item again

...works with it too.

Here's an expanded test that demonstrates the bug.

After I posted this I realized this could use some simplification:

+    $this->assertTrue($page->waitFor(10, function () use ($page, $opener) {
+      $selection_count = count($page->findAll('xpath', "(//div[@data-drupal-selector='edit-media-image-field-selection-0'])[1]"));
+      $button_enabled = $opener->hasAttribute('disabled') === FALSE;
+      return ($selection_count === 0 && $button_enabled);
+    }));

I just need to test that the selection is no longer there.

I think empty($page->findAll('xpath', "(//div[@data-drupal-selector='edit-media-image-field-selection-0'])[1]")) would work.

Simplifying the code mentioned in #75.

I should clarify that this patch is expected to fail, since I forgot the put it in the patch name. This demonstrates the bug in #73.

Here's the rest of the test coverage asked for in #74. This confirms it works with the regular media_library_widget. 🎉!

I started to investigate the bug and found that as far as I can tell, the second selection that fails silently is returning a series of ajax commands that look right to me.

Why is this not doing what it says it's doing? It seems the backend thinks the field has been updated, but the ajax commands fail silently. There are no error messages in the console.

Just debugged this issue. We actually don't need to add the values to the form state in a custom validate function, just limiting the form validation errors conditionally when the field is filled is enough. The problem for empty widget occurs when all validation errors are limited. In this case the unvalidated user input is not added to the form state. For this reason we need to fetch the values from $form_state->getUserInput() instead of $form_state->getValues().

New patch is attached.

Fantastic!

Everything looks great. The only thing I saw was this:

diff --git a/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
index 0d8af0979c..1e9c82a179 100644
--- a/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -545,8 +545,13 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
       ],
       '#validate' => [[static::class, 'validateItems']],
       '#submit' => [[static::class, 'addItems']],
-      // Prevent errors in other widgets from preventing updates.
-      '#limit_validation_errors' => $limit_validation_errors,
+      // We need to prevent the widget from being validated when no media items
+      // are selected. When a media field is added in a subform, entity
+      // validation is triggered in EntityFormDisplay::validateFormValues().
+      // Since the media item is not added to the form yet, this triggers errors
+      // for required media fields.
+      '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],
+
     ];

There's an extra empty line at after '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],

Removed the extra empty line.

Came here from #3013035: This value should not be null. This solved the issue using the media library in combination with paragraphs module where a media field inside a paragraph was required. (thank you @seanB for the reference)

Before this patch, the field gave the error: "This value should not be null."

--- edit:
used: #81 with 8.7.x

Patch from #81 works for Drupal 8.7.7.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -545,8 +545,12 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // We need to prevent the widget from being validated when no media items
   +      // are selected. When a media field is added in a subform, entity
   +      // validation is triggered in EntityFormDisplay::validateFormValues().
   +      // Since the media item is not added to the form yet, this triggers errors
   +      // for required media fields.
   +      '#limit_validation_errors' => !empty($referenced_entities) ? $limit_validation_errors : [],
   

   This comment is 👍 and makes clear why we are doing this dance. Beautiful. Thank you.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -837,8 +841,11 @@ public static function addItems(array $form, FormStateInterface $form_state) {
   +    // Get the new media IDs passed to our hidden button. We need to use the
   +    // actual user input, since when #limit_validation_errors is used, the
   +    // unvalidated user input is not added to the form state.
   +    // @see FormValidator::handleErrorsWithLimitedValidation()
   +    $values = $form_state->getUserInput();
   

   This is also great. I 😍 it.

Frankly, I see nothing to complain about here. RTBC against 8.8.x.

I am concerned about the failure of #81 on 8.7.x, but maybe it was a fluke. I have queued it for re-testing.

I believe this should be backported, since it's not disruptive at all and fixes a problem that many people have encountered. However, if it still fails on 8.7.x, we can address that once this issue hits the "Patch (to be ported)" status.

The new tests look to fail on 8.8.x on Postgres, see https://www.drupal.org/pift-ci-job/1416992

NW for that.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -837,8 +841,11 @@ public static function addItems(array $form, FormStateInterface $form_state) {
+    // Get the new media IDs passed to our hidden button. We need to use the
+    // actual user input, since when #limit_validation_errors is used, the
+    // unvalidated user input is not added to the form state.
+    // @see FormValidator::handleErrorsWithLimitedValidation()
+    $values = $form_state->getUserInput();

@@ -870,7 +877,10 @@ protected static function getNewMediaItems(array $element, FormStateInterface $f
+    // We need to use the actual user input, since when #limit_validation_errors
+    // is used, the unvalidated user input is not added to the form state.
+    // @see FormValidator::handleErrorsWithLimitedValidation()
+    $values = NestedArray::getValue($form_state->getUserInput(), $path);

Are there any security implications here?

Attempting to fix the test for Postgres. I don't have it set up locally, so I'm just guessing here:
Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-10043:9515/session/2456d240348eb81afedf3d129f9f5f41/execute with params: {"script":"return arguments[0].getAttribute(\"disabled\")","args":[{"ELEMENT":"0.30367707132219235-9"}]}

I think it's this:
$opener->hasAttribute('disabled') in this
$button_enabled = $opener->hasAttribute('disabled') === FALSE;

I refactored the test a bit so that it won't be susceptible to $opener getting stale (I suspect that's what's happening). Also, If we create a wrapper element variable, we can use the more readable 'Add media' button selector.

Also, "(//div[@data-drupal-selector='edit-media-image-field-selection-0'])[1]" is repeated three times, I think a variable will make this more readable.

The test in question passed on Postgres. The new failure is related to this newly committed issue: #2969678: Integrate Media Library with Content Moderation.

It's failing in HEAD: https://www.drupal.org/node/3060/qa

https://www.drupal.org/pift-ci-job/1417227
https://www.drupal.org/pift-ci-job/1417228

So this issue's test coverage is OK now.

I retriggered the tests for Postgres and SQLite in #87, now that the failure in HEAD has been fixed in #2969678: Integrate Media Library with Content Moderation.

All tests are now green in #87.

Regarding #86

Are there any security implications here?

We have this:

        // Any ID can be passed to the widget, so we have to check access.
        if ($media_item->access('view')) {
          $field_state['items'][] = [
            'target_id' => $media_item->id(),
            'weight' => ++$weight,
          ];
        }

We already expected unvalidated input, so I think we are fine.

Restoring RTBC since the test fixes look good to me.

This is a minor stylistic thing in the test suggested by @phenaproxima:

-    $assert_session->buttonExists('Add media', $wrapper)->press();
+    $wrapper->pressButton('Add media');

It has no functional difference, so leaving at RTBC. I do think we need to explore the security question raised in #86.

We already expected unvalidated input, so I think we are fine.

But now that validateItems isn't always run (correct me if I'm wrong) are we now missing the check on number of values and bundles?

To get an answer to #93

@larowlan just double checked. The #limit_validation_errors code does not set the values on the form state and for that reason #needs_validation is not set on these elements. In FormValidator::doValidateForm() this means the required check is not done on the elements for example.

The validateItems() method runs as expected since the validation handlers of a clicked button are ALWAYS called. So for our custom validation that only means we have to fetch the direct user input and validate it ourselves.

Committed f31214d and pushed to 8.8.x. Thanks!

I have recently faced this issue with the media library widget used in the media field of the paragraph (v1.12), got resolved after changing the widget to entity browser.

Sorry to reopen this but I think I found a new problematic case. When deleting multiple media (like clicking on the cross of two medias before the ajax response is triggered) then weirdly it replace the entire paragraph by another paragraph in the page.
Step to reproduce :
- Create two paragraphs field in a node with PARAGRAPH EXPERIMENTAL
- create a media field in paragraphs used in both fields
- Add paragraphs and media to the node
- Save the content
- Edit et remove multiple medias in one paragraph quickly.

=> The paragraph is replaced by another one.