HTMLRestrictions::fromString() bug: multiple occurrences of same tag results in only last one being respected

Also ran into this over at #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path: https://git.drupalcode.org/project/drupal/-/merge_requests/2310/diffs?co....

#8: wow, great catch! 👍

Unrelated failure in ComposerHookTest::testComposerHooks(). Sounds like a recent 10.0.x regression. Queued test against 9.5.x.

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -274,11 +281,21 @@ public function getHTMLRestrictions() {
+          // Mark the tag as allowed, assigning TRUE for each attribute name if
+          // all values are allowed, or an array of specific allowed values.
+          $restrictions['allowed'][$tag] = [];

This comment does not seem to match the logic? I think it was just moved, but not updated? 🤔

#3283795: ComposerHooksTest is broken on latest DrupalCI PHP container fixed that (unrelated) fail on D10. Re-testing.

#13: the comment says it's marking TRUE, but it doesn't do that. I just realized the original location for this comment was similarly confusing though!

Further down, it says basically the same thing, but in the actual place where that does happen:

          if (empty($allowed_attribute_values)) {
            // If the value is the empty string all values are allowed.
            $restrictions['allowed'][$tag][$name] = TRUE;
          }
          else {
            // A non-empty attribute value is assigned, mark each of the
            // specified attribute values as allowed.
            foreach ($allowed_attribute_values as $value) {
              $restrictions['allowed'][$tag][$name][$value] = TRUE;
            }
          }

so I propose this, which I think would be clearer:

diff --git a/core/modules/filter/src/Plugin/Filter/FilterHtml.php b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
index a044bd8a45..07a12b449d 100644
--- a/core/modules/filter/src/Plugin/Filter/FilterHtml.php
+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -281,12 +281,11 @@ public function getHTMLRestrictions() {
           continue;
         }
 
-        // If the same tag was present before in the configuration do not
-        // override existing contents.
+        // If the tag is not yet present, prepare to add attribute restrictions.
+        // Otherwise, check if a more restrictive configuration (FALSE, meaning
+        // no attributes were allowed) is present: then override the existing
+        // value to prepare to add attribute restrictions.
         if (!isset($restrictions['allowed'][$tag]) || $restrictions['allowed'][$tag] === FALSE) {
-          // When the tag did not exist previously, mark the tag as allowed,
-          // assigning TRUE for each attribute name if all values are allowed,
-          // or an array of specific allowed values.
           $restrictions['allowed'][$tag] = [];
         }
 

Unrelated Layout Builder failure.

🤩 Unit tests are green, I expect all tests to be green, so … 🚢

Great catch, @bnjmnm!

@nod: you're right that the comment is rather terse. It says that if this issue (#3278636) is fixed, we'll be able to update the Media plugin's PHP logic to not use HtmlRestrictions at all anymore. See #4.

So that means changing

    elements:
      - <drupal-media>
      - <drupal-media data-entity-type data-entity-uuid alt data-view-mode>

to

    elements:
      - <drupal-media>
      - <drupal-media data-entity-type data-entity-uuid alt>
      - <drupal-media data-view-mode>

in ckeditor5.ckeditor5.yml and

  public function getElementsSubset(): array {
    $all_elements = $this->getPluginDefinition()->getElements();
    $subset = HTMLRestrictions::fromString(implode($all_elements));
    $view_mode_override_enabled = $this->getConfiguration()['allow_view_mode_override'];
    if (!$view_mode_override_enabled) {
      $subset = $subset->diff(HTMLRestrictions::fromString('<drupal-media data-view-mode>'));
    }
    // @todo Simplify in https://www.drupal.org/project/drupal/issues/3278636, that will allow removing all uses of HTMLRestrictions in this class.
    return array_merge(['<drupal-media>'], $subset->toCKEditor5ElementsArray());
  }

to something far simpler, like:

  public function getElementsSubset(): array {
    $subset = $this->getPluginDefinition()->getElements();
    $view_mode_override_enabled = $this->getConfiguration()['allow_view_mode_override'];
    if (!$view_mode_override_enabled) {
      $subset = array_diff($subset, ['<drupal-media data-view-mode>']);
    }
    return $subset;
  }

(This is better, because it reduces the reliance on the technically private HtmlRestrictions in the CKEditor 5 plugin implementations that Drupal core contains. The bug that this issue is fixing prevented that!)

 19 | WARNING | [x] Unused use statement

Unrelated failure in Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderQuickEditTest::testQuickEditIgnoresDuplicateFields().

RTBC, and re-testing.

I don't think this is Minor at all, this can seriously hamper the DX, as the #27 interdiff proves.