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

dawehner’s picture

That tag is just used by some dedicated team.

Aki Tendo’s picture

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

dawehner’s picture

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

Mh, 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.

bill richardson’s picture

Should the tests that you are running, not be run to exclude
any files that are in the tests directory ?

Aki Tendo’s picture

I'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.

catch’s picture

Title: Widespread HTML validation issue - The ID attribute MUST be unique on the page. » [meta] Widespread HTML validation issue - The ID attribute MUST be unique on the page.
Category: Bug report » Plan

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Component: base system » theme system