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, almost 8 years ago
Updated 19 February 2023, over 2 years 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.
🐛 Bug report
Status

Needs work

Version

10.0 ✨

Component
Form  →

Last updated 1 day 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.

  • Needs change record

    A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

  • Needs release note

    The major change should have a special release note written to summarize the importance of the change. See Write a release note for an issue.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

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

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

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

  • Status changed to Needs review about 2 years ago
  • Comment about 2 years ago →
    🇺🇸United States ChaseOnTheWeb USA

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

  • Status changed to RTBC about 2 years ago
  • Comment about 2 years 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 about 2 years ago
    28,505 pass
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update about 2 years ago
    28,519 pass
  • Status changed to Needs work about 2 years ago
  • Comment about 2 years 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?

  • Comment 16 days ago →
    🇩🇪Germany webflo

    Re-roll of #55.

  • Comment 16 days ago →
    🇩🇪Germany webflo
  • Merge request !12493Issue #2897377 → (Open) created by oily
  • Pipeline finished with Failed
    15 days ago
    Total: 483s
    #531696
  • First commit to issue fork.
  • Pipeline finished with Failed
    15 days ago
    Total: 139s
    #531835
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024