Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Yesterday I added an additional validation class to the Template library which asserts that the ID attribute of outgoing HTML documents is unique. It passed its own unit, and those of the Template library cleanly, but has 1,640 fails system wide. I double checked a handful of these fails - it's not the the new code that is failing, its the existing code.
The id attribute of every element on the page must be unique. Pages that fail to follow this guideline will not validate. More seriously, the behavior of JavaScript's document.getElementById() function is UNDEFINED by the standard when multiple id's are encountered - all current browsers return the first element in the document possessing the id, ignoring the others, but there is no guarantee that any of them will continue to do this.
Is the code usable in this state? Yes, but this failure can and will cause java script debugging to be more difficult than it should be. These failure are errors and need to be removed as quickly as possible. Individually I wouldn't consider them release blocking - collectively I would because there's potentially a large amount of code this will cause problems for.
Proposed resolution
There's really nothing that can be done but go through the tests and fix the problems one at a time. Refer to this patch test...
https://qa.drupal.org/pifr/test/1015673
This is the assertion that is being failed by the code regarding id's...
/**
* Nothing here is in use when assertions are turned off - these are just
* the tests ID's should conform to.
*/
protected function validate( $value ) {
// If there isn't an id there's nothing to validate.
if(!$value) {
return true;
}
// CSS identifiers are case insensitive. We'll respect the user's chosen
// casing on display but internally we'll store the id's for comparison by
// all lower case.
$key = strtolower($value);
// ID's are unique across the whole document, not just a given element's
// attribute list, which is why we're using a static here.
if (isset(static::$issued[$key])) {
return false;
}
// Test for valid CSS identifier pattern.
if (!preg_match('/^((?!\d)(?!\-\-)(?!\-\d)([\x{002D}\x{0030}-\x{0039}\x{0041}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-\x{FFFF}]+))$/u', $value)) {
return false;
}
// Good to go. Register the key for later use to remove this id from the
// list of those issued when necessary, then register this id as in use.
$this->key = $key;
static::$issued[$key] = $key;
return true;
}
Note that the class has a __destruct method to unregister an issued id if the attribute is deleted, and its own unit test verifies that is working. To discover which id is being repeated plug in an echo "$value"; exit; or exception throw immediately after
if (isset(static::$issued[$key])) {
With that offender in hand do a find against the output html of the page in question. Again, the handful I've checked there has indeed been a repetition of id's.
Remaining tasks
Go through the failed tests and bring the code in question into compliance.
User interface changes
None.
API changes
None.
Comments
Comment #1
dawehnerThat tag is just used by some dedicated team.
Comment #2
Aki Tendo CreditAttribution: Aki Tendo commentedIs there a tag to use when something has the potential to be critical then? I know I don't have the authority to mark it so.
Comment #3
dawehnerMh, good question ... in general we have to go over all major issues anyway, but I don't think that duplicate IDs in the views / fields UI is critical.
Comment #4
bill richardson CreditAttribution: bill richardson commentedShould the tests that you are running, not be run to exclude
any files that are in the tests directory ?
Comment #5
Aki Tendo CreditAttribution: Aki Tendo commentedI'm not sure I understand #4. There's the unit tests specific to the Attribute system in \Drupal\Tests\Core\Template\AttributeTest.php, and the patch I wrote has a revised version of that file with about 3x as many assertions as the 8.0.x one due to the tightening of the code and the testing of that tightening. That said, the patch should also be able to pass every test in Drupal when run all tests is done. Currently this isn't happening. That is usually a sign of something wrong in the patch but in this case it reveals problems in the code that were previously unrevealed. This is going to happen a lot as assertions start getting added to Drupal. It will be painful, but on the far side of that pain lies a more stable and easier to work with code base.
So no, I don't think I should be excluding other tests. A good patch passes all tests.
Comment #6
catchThis can't be resolved until the work in #2346799: Replace #ID selectors for data-drupal-selector attribute is finished.
#766146: Support multiple forms with same $form_id on the same page is related.
#2503239: Consider using <label><input></input></label> instead of <label for="foo"></label> is also a problem but unfixable.
Moving to a plan.
Comment #10
joelpittetComment #19
apaderno