Problem/Motivation

The media library widget has a "submit" that looks like:

    $element['media_library_open_button'] = [
      '#type' => 'submit',
      '#value' => $this->t('Add media'),
...
      '#submit' => [],
      // Allow the media library to be opened even if there are form errors.
      '#limit_validation_errors' => [],
    ];

The other buttons in the widget use '#type' => 'submit' along with custom #submit handlers to execute code after the the button has been pressed and validation has completed.

It looks like the intention here was to have a button which when submitted would do nothing (the primary action of opening the media library is carried out in the #ajax handler). However, the form submitted uses the following logic for figuring out if submit handlers should come from the form itself or the triggering element:

  /**
   * {@inheritdoc}
   */
  public function executeSubmitHandlers(&$form, FormStateInterface &$form_state) {
    // If there was a button pressed, use its handlers.
    $handlers = $form_state->getSubmitHandlers();
    // Otherwise, check for a form-level handler.
    if (!$handlers && !empty($form['#submit'])) {
      $handlers = $form['#submit'];
    }

Since $handlers is [] it's falsey and the parent form submission is triggered, causing entity forms to build the entity and other components like layout builder to insert inline blocks multiple times.

Proposed resolution

I think a good resolution would be to convert this 'submit' into a 'button' instead. We aren't intending to use a submit callback or validation for opening the modal, so I think it's more appropriate.

It's possible FormSubmitter should use a NULL check instead of !$handlers but I don't think it'd be wise to make such low level changes to the FAPI system.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#21 3094059-19.patch4.44 KBseanb
#21 interdiff-11-19.txt1.83 KBseanb
#11 3094059-11.patch4.32 KBsam152
#11 3094059-11-TEST-ONLY-BUG-FROM-10.patch4.4 KBsam152
#11 interdiff.txt1.64 KBsam152
#8 3094059-8.patch3.51 KBsam152
#8 3094059-8-TEST-ONLY.patch2.33 KBsam152
#5 3094059-5.patch3.51 KBsam152
#5 3094059-5-TEST-ONLY.patch2.33 KBsam152

Comments

Sam152 created an issue. See original summary.

sam152’s picture

sam152 commented
Issue summary: View changes
sam152’s picture

sam152 commented
Issue summary: View changes
sam152’s picture

sam152 commented
Issue summary: View changes
sam152’s picture

sam152 commented
Status: Active » Needs review
StatusFileSize
new 3094059-5-TEST-ONLY.patch2.33 KB
new 3094059-5.patch3.51 KB

This seemed like the best way to test this. There are forms in contrib and layout builder which do have side effects in submit handlers that would be measurable, but with a lot of setup and dependencies.

The last submitted patch, 5: 3094059-5-TEST-ONLY.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 5: 3094059-5.patch, failed testing. View results

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new 3094059-8-TEST-ONLY.patch2.33 KB
new 3094059-8.patch3.51 KB

Fixing fails.

The last submitted patch, 8: 3094059-8-TEST-ONLY.patch, failed testing. View results

oknate’s picture

oknate
Location Greater New York City Area
commented
Status: Needs review » Needs work

I'm getting "Title field is required." when opening the Media Library with the patch applied.

Steps to reproduce:
1) Add Media Library Field to Article content type.
2) Open Media Library

Expected: No validation message
Actual: "Title field is required."

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.64 KB
new 3094059-11-TEST-ONLY-BUG-FROM-10.patch4.4 KB
new 3094059-11.patch4.32 KB

Test to reproduce #10 and a fix.

The last submitted patch, 11: 3094059-11-TEST-ONLY-BUG-FROM-10.patch, failed testing. View results

oknate’s picture

oknate
Location Greater New York City Area
commented

I have tested #11, and it works fine for me. This seems like a good change. +1 for RTBC from me. But I'd like to hear from some others before marking it as such.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Issue tags: +Media Initiative, +Needs accessibility review

This all seems to make sense. Not sure why we didn't run into this before? I think the change from a submit button to a regular button shouldn't be a problem, but it would be good to get a +1 from the A11Y experts as well.

I also thought this might had something to do with #3003150: Media library causes validation errors when it is used in a required field of a nested form but that was about the hidden button to update the widget, not the open button. So if the A11Y maintainers are fine with the change, RTBC +1 from me.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

Closed #3047046: Media Library widget fails when used on taxonomy term form. as a duplicate of this issue.

andrewmacpherson’s picture

andrewmacpherson commented
Issue tags: -Needs accessibility review

I think the change from a submit button to a regular button shouldn't be a problem, but it would be good to get a +1 from the A11Y experts as well.

Changing from FAPI #type => submit to #type => button makes no changes whatsoever to the HTML. Both of those FAPI snippets emit the same HTML: <input type="submit"

There isn't any change here at all as far as the HTML source, DOM, or accessibility tree is concerned.

+1 from me.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Before I can RTBC #11, I have a question -- the test-only patch removes #limit_validation_errors from the open button, but the passing patch doesn't. What's up with that?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/media_library/tests/modules/media_library_test/src/Form/TestNodeFormOverride.php
@@ -0,0 +1,24 @@
+    if (in_array('open_button', $triggering_element['#parents'])) {

    Nit: This should have TRUE as the third argument. 

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -480,6 +480,10 @@ public function testWidget() {
+    // Validation messages from the parent form should not appear.
+    $assert_session->pageTextNotContains('Title field is required.');

    We probably need to explain this a bit better and go into more detail about what this assertion covers, lest we accidentally remove it. A few @see comments couldn't hurt.

matthieuscarset’s picture

matthieuscarset commented

Tested on 8.7.8 and it unfortunately does not apply.

FYI this one-line patch from https://www.drupal.org/project/drupal/issues/3047046 works on 8.7.8

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Okay, that means we will need to reroll this for backport. Thanks for testing it!

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-11-19.txt1.83 KB
new 3094059-19.patch4.44 KB

Fixed #18.

phenaproxima’s picture

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

Thanks, @seanB! This looks good, RTBC on green.

phenaproxima’s picture

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

On second thought, sending back to NR for #17. Please feel free to restore RTBC once testbot is green and the question I posed is answered.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented

From https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

When the button is pressed, the form will be submitted to Drupal, where it is validated and rebuilt. The submit handler is not invoked.

So we need the limit validation errors to make sure the validation is not triggered for the button either.

phenaproxima’s picture

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

Makes sense.

sam152’s picture

sam152 commented

The test only patch removed #limit_validation_errors to prove that the new test coverage covered the bug reported in comment #10.

Thanks for reviews and fixes 👍

webchick credited 5n00py.

webchick credited dion-jensen.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Crediting folks from the other issue @ #3047046: Media Library widget fails when used on taxonomy term form..

  • webchick committed ff4351a on 9.0.x 
    Issue #3094059 by Sam152, seanB, phenaproxima, oknate, 5n00py,...

  • webchick committed 157ab25 on 8.9.x 
    Issue #3094059 by Sam152, seanB, phenaproxima, oknate, 5n00py,...

  • webchick committed 95a08e3 on 8.8.x 
    Issue #3094059 by Sam152, seanB, phenaproxima, oknate, 5n00py,...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great find, great fix, and thanks for the test coverage!

Committed and pushed to 9.0.x; 8.9.x; 8.8.x. Thanks a lot!

Status: Fixed » Closed (fixed)

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