Contrib.social

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

Open on Drupal.org →
Created on 25 July 2017, over 7 years ago
Updated 29 October 2023, over 1 year ago

Problem/Motivation

#1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests → fixed some important problems around managing HTML ID uniqueness by appending a random suffix. The random suffix means that the ID should not be used in JS and CSS targeting. Instead, that patch created a data-drupal-selector attribute to be used in JS and CSS selectors in lieu of IDs. That issue's change record → says:

For forms there is an automatically added additional attribute: data-drupal-selector corresponding to the element id before randomization.

The problem is that this turns out to not be true for the <form> itself. It also turns out to not be true for any form element whose #id is already explicitly set to something returned by Html::getUniqueId(), which is what code is supposed to use when explicitly setting an element's #id.

The reason is that FormBuilder::doBuildForm() has this code:

if (!isset($element['#id'])) {
...
}
else {
  $element['#attributes']['data-drupal-selector'] = Html::getId($element['#id']);
}

Which means any element with an already random-suffixed #id from Html::getUniqueId() receives a correspondingly random-suffixed data-drupal-selector. And that includes the <form> itself, since FormBuilder::prepareForm() initializes $form['#id'] to a random-suffixed value.

Proposed resolution

  • Fix FormBuilder to never base data-drupal-selector on an already randomized ID. Instead, it should always be based on the non-random information from which the ID is derived.
  • While we're at it, make sure we never clobber an already existing data-drupal-selector value. This allows code that explicitly sets a form or element #id to also optionally set the data-drupal-selector from the same pre-randomized information. Note that code that explicitly sets #id is not required to also set data-drupal-selector. Those two matching (other than the presence/absence of the random suffix) is a developer convenience, not a requirement.

Release notes snippet

In earlier releases, the data-drupal-selector attribute on form elements was unintentionally randomized 🐛 $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose Needs work . Developers who use this attribute in CSS or Javascript selectors should review the change notice for how the automatic generation of the data-drupal-selector attribute → has changed.

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Form  →

Last updated 2 days ago

Created by

🇺🇸United States effulgentsia

Live updates comments and jobs are added and updated live.
  • Blocks-Layouts

    Blocks and Layouts Initiative. See the #2811175 Add layouts to Drupal issue.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

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

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States ChaseOnTheWeb USA

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

  • Status changed to RTBC over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    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).

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    28,505 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    28,519 pass
  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇫🇮Finland lauriii Finland
    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.
  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    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?

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024