$form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose

I think this can only be RTBC'd by fellow Form API maintainer Tim Plunkett?

Wow, great find. And the fix looks good.
This should be testable.

Thank you, the patch in #5 fixed my issue.

(I tried using AjaxFormHelperTrait which didn't work correctly because it uses data-drupal-selector to reload the form. Might be something else to look into.)

The patch in #5 fails with the latest version of Drupal.

Testing Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest
.F                                                                  2 / 2 (100%)

Time: 19.68 seconds, Memory: 4.00MB

There was 1 failure:

1) Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest::testDateTimeAjaxCallback
Failed asserting that a NULL is not empty.

/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbacksTest.php:41

Had a look at this, and to me it looks like the failure is catching a bug introduced by the current patch.

I installed the ajax_forms_test module and had a look at the form element attributes. The problem is that with the patch applied, it is getting data-drupal-selector="edit-datetime" on both the date and the time elements, when it should be getting edit-datetime-date and edit-datetime-time respectively.

Thanks to everyone who has worked on this. I dug into #14 and #15. I don't think we want to check if the 'data-drupal-selector' attribute has already been set on an element. It breaks with complex fields, such as datetime. When the datetime date and datetime time subfields are being built by \Drupal\Core\Form\FormBuilder::doBuildForm(), they already have a 'data-drupal-selector' attribute from their parent. We do want to overwrite that. Proposed change:

    $unprocessed_id = 'edit-' . implode('-', $element['#parents']);
    if (!isset($element['#id'])) {
      $element['#id'] = Html::getUniqueId($unprocessed_id);
    }
-   if (!isset($element['#attributes']['data-drupal-selector'])) {
-   // Provide a selector usable by JavaScript. As the ID is unique, its not
-   // possible to rely on it in JavaScript.
-   $element['#attributes']['data-drupal-selector'] = Html::getId($unprocessed_id);
-   }
+   // Provide a selector usable by JavaScript. As the ID is unique, its not
+   // possible to rely on it in JavaScript.
+   $element['#attributes']['data-drupal-selector'] = Html::getId($unprocessed_id);

FYI - My use case is validating section configuration forms in Layout Builder. I can trigger an error, but it doesn't appear because of the bug identified by this issue.

I'm queuing up #5 to re-test on PHP 7.1 & MySQL 5.7 against D8.9.

Re the "Needs tests" tag, take a look at the test results from #5 and #17. There's test coverage in place.

Changing to version 8.8.x-dev as this is a non-disruptive bug fix.

Tests still pass on 8.9.x and 9.1.x. Leaving at 'Needs Review'.

There is indeed existing test coverage of this code, but it is insufficient.
What is needed is a test that enforces the new change. One that passes with the above fix, but fails when run against HEAD without the fix.
This proves that the bug fix works, and will prevent regressions in the future.

I have Monday blocked off to work on this. As I've dug further into this issue, I've found additional code that needs addressed. For example, in \Drupal\layout_builder\Form\ConfigureBlockFormBase::doBuildForm(), the bug described by this issue is acknowledged with a workaround implemented.

I've taken a deeper dive into this issue, and I think it may be as simple as this: If a form's 'data-drupal-selector' attribute is set, then leave it alone. If it's not set, then set it.

In \Drupal\Core\Form\FormBuilder::prepareForm(), check if 'data-drupal-selector' is set. If yes, then leave it alone. If no, then set it.

In \Drupal\Core\Form\FormBuilder::doBuildForm, don't attempt to set the 'data-drupal-selector' attribute. This has already been handled in ::prepareForm().

I've removed the following code from the updated patch because, so far as I can tell, it doesn't do anything:

  if (isset($form_id)) {
    $form['form_id'] = [
      '#type' => 'hidden',
      '#value' => $form_id,
      '#id' => Html::getUniqueId("edit-$form_id"),
-     '#attributes' => [
-       'data-drupal-selector' => Html::getId("edit-$form_id"),
-     ],
      // Form processing and validation requires this value, so ensure the
      // submitted form value appears literally, regardless of custom #tree
      // and #parents being set elsewhere.
      '#parents' => ['form_id'],
    ];
  }

Unit Tests

The patch adds the following unit test: Drupal\Tests\Core\Form\FormBuilderTest::testDataDrupalSelector(), along with a TestFormWithDataDrupalSelector class.

Remove workarounds in Layout Builder and Settings Tray

This bug is acknowledged with a workaround implemented in each Layout Builder and Settings Tray. The workarounds can be removed with this patch.

Layout Builder Workaround

Workaround code in \Drupal\layout_builder\Form\ConfigureBlockFormBase::doBuildForm():

  if ($this->isAjax()) {
    $form['actions']['submit']['#ajax']['callback'] = '::ajaxSubmit';
    // @todo static::ajaxSubmit() requires data-drupal-selector to be the same
    //   between the various Ajax requests. A bug in
    //   \Drupal\Core\Form\FormBuilder prevents that from happening unless
    //   $form['#id'] is also the same. Normally, #id is set to a unique HTML
    //   ID via Html::getUniqueId(), but here we bypass that in order to work
    //   around the data-drupal-selector bug. This is okay so long as we
    //   assume that this form only ever occurs once on a page. Remove this
    //   workaround in https://www.drupal.org/node/2897377.
    $form['#id'] = Html::getId($form_state->getBuildInfo()['form_id']);
  }

BlockFormMessagesTest results with workaround removed and no patch

$ ../vendor/phpunit/phpunit/phpunit -c core/ core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php --verbose
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.17-1+0~20200419.57+debian10~1.gbp0fda17
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest
F                                                                   1 / 1 (100%)

Time: 1.01 minutes, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest::testValidationMessage
Failed asserting that a NULL is not empty.

/var/www/html/web/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php:103
/var/www/html/web/core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php:73

FAILURES!
Tests: 1, Assertions: 14, Failures: 1.

BlockFormMessagesTest results with workaround removed and with patch

b$ ../vendor/phpunit/phpunit/phpunit -c core/ core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFormMessagesTest.php --verbose
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.17-1+0~20200419.57+debian10~1.gbp0fda17
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest
.                                                                   1 / 1 (100%)

Time: 1.02 minutes, Memory: 4.00MB

OK (1 test, 24 assertions)

Settings Tray Workaround

Workaround code in \Drupal\settings_tray\Block\BlockEntitySettingTrayForm::doBuildForm():

  $form['#attached']['library'][] = 'core/drupal.dialog.ajax';

  // static::ajaxSubmit() requires data-drupal-selector to be the same between
  // the various Ajax requests. A bug in \Drupal\Core\Form\FormBuilder
  // prevents that from happening unless $form['#id'] is also the same.
  // Normally, #id is set to a unique HTML ID via Html::getUniqueId(), but
  // here we bypass that in order to work around the data-drupal-selector bug.
  // This is okay so long as we assume that this form only ever occurs once on
  // a page.
  // @todo Remove this workaround once https://www.drupal.org/node/2897377 is
  //   fixed.
  $form['#id'] = Html::getId($form_state->getBuildInfo()['form_id']);

  return $form;

SettingsTrayBlockFormTest results with workaround removed and no patch

$ ../vendor/phpunit/phpunit/phpunit -c core/ core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php --verbose
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.17-1+0~20200419.57+debian10~1.gbp0fda17
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest
..E                                                                 3 / 3 (100%)

Time: 5.34 minutes, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testValidationMessages
Behat\Mink\Exception\ElementHtmlException: The string "Sorry system error. Please save again" was not found in the HTML of the element matching css "#drupal-off-canvas".

/var/www/html/vendor/behat/mink/src/WebAssert.php:803
/var/www/html/vendor/behat/mink/src/WebAssert.php:515
/var/www/html/web/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php:284

ERRORS!
Tests: 3, Assertions: 479, Errors: 1.

SettingsTrayBlockFormTest results with workaround removed and with patch

$ ../vendor/phpunit/phpunit/phpunit -c core/ core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php --verbose
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.17-1+0~20200419.57+debian10~1.gbp0fda17
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest
...                                                                 3 / 3 (100%)

Time: 5.41 minutes, Memory: 4.00MB

OK (3 tests, 498 assertions)

Miscellaneous

In Layout Builder, we have coverage for block form messages but not for section form messages. See Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest. I'm making a note, so we can add a follow-up issue when this patch is committed.

Updated patch/interdiff files with corrected path.

It looks like the following code does do something. This patch adds is back. This should correct the test failures from #28.

+     '#attributes' => [
+       'data-drupal-selector' => Html::getId("edit-$form_id"),
+     ],

Tests passed. The tests-only patch failed, as expected. Setting back to 'Needs Review'.

you can name your patch somethingsomthing--FAIL.patch to avoid the testbot putting the issue in "needs work" on fails.

I'm not convinced the issue is properly replicated in the tests, then again a little tired so I might have missed it.

you can name your patch somethingsomthing--FAIL.patch to avoid the testbot putting the issue in "needs work" on fails.

That's good to know! Somehow I missed that up until now.

I'm not convinced the issue is properly replicated in the tests, then again a little tired so I might have missed it.

There are three tests that provide coverage:

• Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testValidationMessages
• Drupal\Tests\layout_builder\FunctionalJavascript\BlockFormMessagesTest::testValidationMessage
• Drupal\Tests\Core\Form\FormBuilderTest::testDataDrupalSelector

With the workarounds removed, both SettingsTrayBlockFormTest::testValidationMessages and BlockFormMessagesTest::testValidationMessage will fail without the patch.

FormBuilderTest::testDataDrupalSelector verifies that the 'data-drupal-selector' attribute is only set if it isn't already set. This directly tests the code change contained herein.

I don't find test coverage for Layout Builder section form messages.

If we need additional test coverage, just let me know.

ooh I see now why the changes to settings tray and layout builder were in the test-only patch. Thanks for the explanation.

They do show the issue, so it's not a test specific for this issue, but having the bug does make those tests fail. Don't know how specific we need to be in our tests, would need input from someone more up to date on test case practices.

Rerolled for latest 8.9.x.

#3103812: Layout Builder's ConfigureSectionForm forms do not display validation errors on submit adds yet another work around for this issue to remove.

Tried to address this as in #3103812: Layout Builder's ConfigureSectionForm forms do not display validation errors on submit.
Not sure, if this is the right approach.

@anmolgoyal74 I don't think you needed to change the logic from #37, that was fine. It's just that there's an additional workaround in core now (see the other two removed by the patch).

@tim.plunkett. Thanks for the feedback.
If the logic from #37 is correct. then what is the remaining task over here?

@anmolgoyal74 to remove the workaround added in #3103812: Layout Builder's ConfigureSectionForm forms do not display validation errors on submit like the patch in #37 does in ConfigureBlockFormBase for example.

Ah Okay. Got it.

I have remved the work around in this patch.
Do we also need to remove the tests added in #3103812: Layout Builder's ConfigureSectionForm forms do not display validation errors on submit

@anmolgoyal74 the tests are why we committed #3103812: Layout Builder's ConfigureSectionForm forms do not display validation errors on submit - it's great that they are they and add more proof this fix is correct.

In case anyone else bumps into the same issue, it is worth noting that Commerce has its own workaround for this issue which will need to be overridden / removed if you apply this patch.

It is in src/Plugin/Commerce/InlineForm/InlineFormBase.php line 108 in the current version:

<?php
    // Workaround for core bug #2897377.
    $inline_form['#id'] = Html::getId('edit-' . implode('-', $inline_form['#parents']));
?>

Having tested a few possibilities, the only one I have found thus far that doesn't cause issues with other commerce components is to leave that line intact and add an assignment for the data-drupal-selector attribute:

<?php
   $inline_form['#attributes']['data-dupal-selector'] = $inline_form['#id'];
?>

Having to do this suggests the patch at #45 is flawed in how it handles form elements with ids explicitly set but I haven't had enough time to investigate and find a fix for that.

Just ran into this issue with 9.4.5 and wondering if there is any update. Would be nice to see this fixed before 10.0 - thanks!

I rerolled the patch against 10.0x and fixed the custom command error. Please review it

Fixed CS error

Confirmed that the patch in #53 includes all the necessary parts for the patch. So it's the right patch.

Test failure comes from point #2 of the issue summary proposed resolution. Since we're not adding data-drupal-selectors anymore for elements with a "hardcoded" ID, a lot of form elements don't have data-drupal-selectors anymore and we relied on one in the cke5 test.

The change in how many data-drupal-selector are set is probably deserving of a change notice and release note.

The patch in #37 has the side effect that the data-drupal-selectors attribute of views exposed filters is always "views-exposed-form" and not "views-exposed-form-[view_id]-[display]".

Can't reproduce #56 on the 10.1.x branch with the patch from #55.

FYI: This patch will not be backported to 8.x

Wow, I'm surprised I haven't run into this before, as it's such a basic problem for everyone working a lot with ajax-enabled forms.

The patch from #55 still applies to 9.4.8 and fixes the issue.

Only moving back to NW for the change record + release note.

I've attempted a change notice and release note snippet. Please review.

Change record seems clear. I think this may be better suites for 10.2 though as this maybe could be disruptive for existing sites if they were using those selectors (could be wrong).

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -984,11 +989,6 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
          $element['#attributes']['data-drupal-selector'] = Html::getId($unprocessed_id);
   

   Shouldn't we override this attribute only when it isn't already set?

2. Let's extend the test coverage for form elements too. Currently tests are only covering the <form> element.

Just ran into this in Homebox 3.0.x development and can confirm the issue still exists. Patch from #55 fixes the issue.

Re #62.1:

Shouldn't we override this attribute only when it isn't already set?

Makes sense to me, so the developer is still allowed to override it, if needed for edge cases. Any downside ideas?
One could also discuss this is a "core property" starting with data-drupal- on the other hand.

#62.1:

Let's extend the test coverage for form elements too. Currently, tests are only covering the
element.

What exactly should be tested and should it be done in this context?

Re-roll of #55.

This is required for larger projects like Drupal Commerce and Homebox, so I think this is really important. Should it be set to Major for these contrib dependency reasons?

Also see #3426472: Workaround for core bug #2897377 not needed , causes bug

While this is still an important issue, we've found a heavy regression:

Views exposed filters filter button (& AJAX) are broken in Media library modals, because the hack in media_library.module:

/**
 * Form #after_build callback for media_library view's exposed filters form.
 */
function _media_library_views_form_media_library_after_build(array $form, FormStateInterface $form_state) {
  // Remove .form-actions from the view's exposed filter actions. This prevents
  // the "Apply filters" submit button from being moved into the dialog's
  // button area.
  // @see \Drupal\Core\Render\Element\Actions::processActions
  // @see Drupal.behaviors.dialog.prepareDialogButtons
  // @todo Remove this after
  //   https://www.drupal.org/project/drupal/issues/3089751 is fixed.
  if (($key = array_search('form-actions', $form['actions']['#attributes']['class'])) !== FALSE) {
    unset($form['actions']['#attributes']['class'][$key]);
  }
  return $form;
}

is not called any more!

The reason is that in class MediaLibraryHooks:

  /**
   * Implements hook_form_alter().
   */
  #[Hook('form_alter')]
  public function formAlter(array &$form, FormStateInterface $form_state, $form_id) : void {
    // Add a process callback to ensure that the media library view's exposed
    // filters submit button is not moved to the modal dialog's button area.
    if ($form_id === 'views_exposed_form' && str_starts_with($form['#id'], 'views-exposed-form-media-library-widget')) {
      $form['#after_build'][] = '_media_library_views_form_media_library_after_build';
    }
    // Configures media_library displays when a type is submitted.
    if ($form_state->getFormObject() instanceof MediaTypeForm) {
      $form['actions']['submit']['#submit'][] = '_media_library_media_type_form_submit';
      // @see field_ui_form_alter()
      if (isset($form['actions']['save_continue'])) {
        $form['actions']['save_continue']['#submit'][] = '_media_library_media_type_form_submit';
      }
    }
  }

the condition

str_starts_with($form['#id'], 'views-exposed-form-media-library-widget')

is always FALSE using this patch!

$form['#id'] is NOT starting with views-exposed-form-media-library-widget
any more, but now it is:
views-exposed-form--hWV0dUC_yXg

$form['#attributes']['data-drupal-selector'] is also "views-exposed-form"

AJAX is also entirely broken in these Media Modals!

So anyone using this patch, please be aware that it breaks your media modals!

@nod: Just saw that my case verifies #56

The patch in #37 has the side effect that the data-drupal-selectors attribute of views exposed filters is always "views-exposed-form" and not "views-exposed-form-[view_id]-[display]".

In #57 you wrote you can't reproduce it. See #70 for clear reproduction. Any ideas how we could fix that?

Current test implementation in TestFormWithDataDrupalSelector is broken IMHO?

Furthermore, we need to ensure that #56 / #70 are covered (expected to fail or we need additional tests)!

Funny... just saw that the logic was fine until #66 where it was changed. That explains why @nod couldn't reproduce the issue I ran into now with the MR and #66!

@webflo any reasons why you changed the logic in #66 (see the if case!).

So let's start a fresh MR with #55 and then implement #62 on top of that.

Static patch from MR attached until this is merged.

This one seemed to break the unit tests.

@smustgrave

This one seemed to break the unit tests.

What exactly do you mean? I guess they never were green, at least not before my recent changes?

So yes, still NW!