Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object

And as @bnjmnm pointed out: this allows us to add unit tests too! :)

While working on this, identified #3231327-13: Plugin definition DX: validate ckeditor5.drupal.elements items . That will mean one less HTMLRestrictionsUtilities call to convert.

I've been working on unit tests first and foremost and have found some issues. Doing this refactor will make it far easier to detect those subtle bugs ahead of time, that would also make the automatic upgrade path not fail in those subtly broken ways (when it comes to interpreting differences between two sets of HTML restrictions, to determine what the appropriate course of action is during the automatic upgrade path).

More news tomorrow.

ℹ️See #3222852-18: <dl> <dt> <dd> by introducing "Manually editable HTML tags" configuration on Source Editing and #3222852-20: <dl> <dt> <dd> by introducing "Manually editable HTML tags" configuration on Source Editing by @bnjmnm and I, where we both strongly agreed that unit tests for all these complex comparisons are essential.

Crediting Ben and I already, for the work that led to this issue.

Yay, the unit test is working, and already finding problems! 🤓🥳

The new unit test's results before 88b0f21ed7bfe56a0479063bcd51b3d423572ee4 ("Fix ::intersect()"):

...........F...F.......F.....F.......FF.FFFF                      44 / 44 (100%)

vs after:

...........F...F.......F.....F..............                      44 / 44 (100%)

Progress!

Yay #3231327-13: Plugin definition DX: validate ckeditor5.drupal.elements items landed :)

Per #3249240-21: HTMLRestrictionsUtilities:: providedElementsAttributes() causes deprecations on PHP 8.1 by @larowlan: this is considered important by core committers. Not in the least because not having this done already meant it was pretty painful to figure out what were the acceptable changes to get the current code to work on PHP 8.1.

I'm working on the port of this MR!

Postponed #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated, #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss), #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets and #3228691: Restrict allowed additional attributes to prevent self XSS on this.

Postponed #3256616: AssertionError: assert($value === TRUE || Inspector::assertAllStrings($value)) in assert() (line 242 of core/modules/ckeditor5/src/HTMLRestrictionsUtilities.php) on this.

Looks like I triggered an infinite retesting loop in DrupalCI: https://www.drupal.org/pift-ci-job/2289771 → https://dispatcher.drupalci.org/job/drupal_patches/109074/console + https://dispatcher.drupalci.org/job/drupal_patches/109076/console + https://dispatcher.drupalci.org/job/drupal_patches/109077/ …

🙈

Asked in #drupal-infrastructure on Slack to stop it — I don't have the power.

But … at least locally, I'm now seeing

Testing /Users/wim.leers/Work/d8/core/modules/ckeditor5/tests/src/Unit
...........F...F.......F.....F..............                      44 / 44 (100%)

Time: 2.37 seconds, Memory: 8.00 MB

There were 4 failures:

which is identical to what I had in #10 👍 (and also identical to the last test result that was tested against the contrib project: https://www.drupal.org/pift-ci-job/2211769 for https://git.drupalcode.org/project/ckeditor5/-/merge_requests/130/diffs?...)

Yay, tests are working now! The new test case I added based on the bug report in #3256616: AssertionError: assert($value === TRUE || Inspector::assertAllStrings($value)) in assert() (line 242 of core/modules/ckeditor5/src/HTMLRestrictionsUtilities.php) is passing. So this will fix that issue too. Marked that issue as a duplicate.

Please also credit beatrizrodrigues and DieterHolvoet for their work on that issue! 🙏

76d34c21 added much-needed validation that will prevent much future pain. It caused a significant number of additional tests to fail.

4970a06a fixed the bugs that the new validation uncovered.

c65b4550 added more test coverage that I realized was necessary thanks to the preceding commit, which in turn caused more fixes.

05310198 added test coverage for HTMLRestrictions::union(), which was completely missing until now. Note that the implementation of ::union() is identical to that in HEAD!

Test results for the commit prior to that one:

Testing Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
.......................F...F........F.....F..................     61 / 61 (100%)

Time: 00:00.038, Memory: 6.00 MB

There were 4 failures:

Tests results for that commit:

Testing Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
.......................F...F........F.....F.....................E 65 / 81 ( 80%)
EEEE....FF....EE                                                  81 / 81 (100%)

Time: 00:00.045, Memory: 6.00 MB

There were 7 errors:

So … that indicates there were multiple bugs in HEAD that this issue is fixing! (Now removing the Needs tests tag — I will continue adding more test coverage, but at this point I think I've proven beyond a doubt that there are a number of bugs that this issue is solving. 👍)

73ac5784956f5b777c2c29510130fe1433e2568f should fix those failures.

3 of the 4 remaining failures (yes that "1 fail" is actually four … blame DrupalCI) should be fixed in https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co...

1. 1) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testParse with data set "tag with wildcard attribute" ('', array(true))
   Failed asserting that two arrays are identical.
   --- Expected
   +++ Actual
   @@ @@
    Array &0 (
   -    'a' => true
   +    'a' => false
    )
   

2. 2) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testParse with data set "tag with single attribute allowing multiple specific values (reverse order)" ('', array(array(array(true, true))))
   Failed asserting that two arrays are identical.
   --- Expected
   +++ Actual
   @@ @@
    Array &0 (
        'a' => Array &1 (
            'target' => Array &2 (
   +            '_blank' => true
                '_self' => true
   -            '_blank' => true
            )
        )
    )

3. 3) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testRepresentations with data set "realistic" (Drupal\ckeditor5\HTMLRestrictions Object (...), array('', '', ''), '  ', array(array('a', array(true, array('en', 'fr'))), array('p', array(true)), array('br')))
   assert($value === TRUE || Inspector::assertAllStrings($value)) in /var/www/html/core/modules/ckeditor5/src/HTMLRestrictions.php:498
   

4f07de09 passed testing. 😱 That means that the last caller of any HTMLRestrictionsUtilities code is in a section of SmartDefaultSettings with zero test coverage.

So, to be able to proceed, we need test coverage that tests that untested code path, which should then trigger a test failure. I'll add that here, but it really should not land in this MR — that makes this MR too unwieldy to review. Opened #3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) for that.

Also filed #3259179: Split ckeditor5_alignment CKEditor 5 plugin, to allow for more precise upgrade path.

Pushed #3259174-3: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) as a commit on this MR. That should now cause this to fail, fixing the scare we had in 4f07de09! 👍

Yay, that resulted in the failure we hoped:

There was 1 failure:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "basic_html_with_alignable_p can be switched to CKEditor 5 without problems, align buttons added automatically" ('basic_html_with_alignable_p', array(), array(array(array('bold', 'italic', '|', 'link', '|', 'bulletedList', 'numberedList', '|', 'blockQuote', 'uploadImage', '|', 'heading', '|', 'sourceEditing', '|', 'code', 'textPartLanguage', 'alignment', 'alignment:left', 'alignment:center', 'alignment:right', 'alignment:justify')), array(array(array('', '', '', '', '', '', '', '', '', '', '', '', '')), array(array('heading2', 'heading3', 'heading4', 'heading5', 'heading6')), array('un'))), '

👍

Wonderful! Just pushed a rebased branch. The following 3 commits are now omitted from the branch thanks to those issues landing:

1. https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co...
2. https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co...
3. https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co...

That should trigger some new failures, because more bugs will now be exposed. In particular, the data structure validation added to the constructor (in validateAllowedRestrictionsPhase4()) should detect that there is a problem with the computed union for wildcard tags (<$block>).

It is no coincidence that wildcard handling also is the very last remaining thing for which SmartDefaultSettings is calling HTMLRestrictionsUtilities. I already have extra explicit test coverage locally that I will push tomorrow, as well as progress on fixes for those.

More tomorrow!

That should trigger some new failures, because more bugs will now be exposed. In particular, the data structure validation added to the constructor (in validateAllowedRestrictionsPhase4()) should detect that there is a problem with the computed union for wildcard tags (<$block>).

That happened 🥳 This is the error that is causing all test failures:

The "$block" HTML tag has attribute restriction "class", but it is not an array of key-value pairs, with HTML tag attribute values as keys and TRUE as values.

… but there is no unit test yet showing what the root cause. The root cause is that the computed union (of all elements supported by all available CKEditor 5 plugins) is not correct. This is also broken in HEAD but the absence of validation makes it impossible to see.

So, just pushed https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co... which adds a unit test that makes that easy to see 👍

"Not currently mergeable" → due to #3248177: Language toolbar item cannot be removed from the toolbar just having landed half an hour ago, this resulted in a simple conflict. Rebased!

Alright, as promised in #35: pushed wildcard test cases for ::union() that reproduced the failure. Then started to apply the same wildcard test cases to ::diff() and ::union(). I realized I was doing a lot of copy/pasting … and so I refactored that into a single test method that tests all 3 operations, and a single data provider that has a lot of:

    yield 'attribute restrictions are different: <a hreflang="en"> vs <a hreflang="fr">' => [
      'a' => new HTMLRestrictions(['a' => ['hreflang' => ['en' => TRUE]]]),
      'b' => new HTMLRestrictions(['a' => ['hreflang' => ['fr' => TRUE]]]),
      'diff' => 'a',
      'intersection' => new HTMLRestrictions(['a' => FALSE]),
      'union' => new HTMLRestrictions(['a' => ['hreflang' => ['en' => TRUE, 'fr' => TRUE]]]),
    ];

test cases in its data provider. This ensures that every operation has the exact same test coverage (I had made mistakes in my copy/pasting before! 😬), and also shows very clearly how different operations have different effects. Also, I added the ability to not copy/paste needlessly: instead of a HtmlRestrictions object as an expectation, you can also specify 'a' or 'b' — meaning the result of a→operation(b) is … exactly operand a or operand b. This makes the test coverage a lot more maintainable.

Next up: making all those tests actually pass!

We went from

Testing Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
................................................................. 65 / 88 ( 73%)
.....FF..FF..........EE                                           88 / 88 (100%)

Time: 00:00.047, Memory: 6.00 MB
<snip>
Testing Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest
EEEEEE.E..                                                        10 / 10 (100%)

Time: 00:07.687, Memory: 6.00 MB

to the commit that fixed ::union() getting us to:

Testing Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
................................................................. 65 / 88 ( 73%)
.....FF..FF............                                           88 / 88 (100%)

Time: 00:00.047, Memory: 6.00 MB

… and no other failures 😄

Now the commit fixing ::diff() and ::intersect() should get us to 100% green again. 🤞

Once that has happened, I can refactor away the last use of \Drupal\ckeditor5\HTMLRestrictionsUtilities::getWildcardTags() from SmartDefaultSettings like I said yesterday in #34 … 🤓🥳

VICTORY: https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co... → this was the last use of HTMLRestrictionsUtilities.

#3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) added test coverage to HEAD for this piece of code. Now it is drastically simplified, thanks to the new HTMLRestrictions value object having ::diff(), ::intersect() and ::union() operators. That means that individual pieces of code no longer need to have complex logic (in this case: eight levels of nested if and for statements!!!!!) to interpret/compare \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions() arrays! 🥳

Note that I’m only doing minimal simplification here — the goal here is to refactor HTMLRestrictionsUtilities away, not to refactor SmartDefaultSettings. So I’m resisting that temptation, to land this as soon as possible.

In the commit linked above, you can see ~60 lines of complex code disappear. The complexity is now in those 3 operations. We just use those operations. And those operations are very thoroughly unit-tested. Therefore fixing any edge cases that I might have missed will be orders of magnitude simpler the next time 😊

Note that this merge request only handles the allowed top-level key of \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions() — forbidden_tags is currently ignored. But that is no regression compared to HEAD anyway. #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated will have to add support for that. We could do it here too, but then the scope will become even bigger.

Note that if you look at the total set of changes at https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs, then you’ll notice that every file sees a net reduction in lines of code — with the exception of the new value object’s class and its test coverage of course. There’s a net increase of ~1000 lines of code. But ~700 of those are for test coverage! Much of the remaining 300 is due to additional comments.
(And again, note that SmartDefaultSettings will be able to get simplified further, but I consider that out of scope here.)

Follow-ups that will be unblocked once this lands:

• #3259493: [GHS] Unable to limit attribute values: ::allowedElementsStringToHtmlSupportConfig() does not generate configuration that CKEditor 5 expects
• #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets
• #3228691: Restrict allowed additional attributes to prevent self XSS
• #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss)
• #3231336: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated

I'll push a few clean-up commits tomorrow, but this is fundamentally ready for review. I'd be happy to walk reviewers through the changes in a call! 🤓

Since my last comment:

1. Addressed @bnjmnm's feedback
2. Finished test coverage for HTMLRestrictions
3. Ensured we pave the path instead of putting roadblocks up for #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss): b31e98a8 fixes an oversight I made in 739d10b
4. Updated FundamentalCompatibilityConstraintValidator to use the new value object operations instead of custom array logic in 39c99b2f + de2a1b57
5. Updated SmartDefaultSettings to use the new value object operations instead of custom array logic in f9956225 + 5603a62c

Thanks to this issue, we discovered #3259179: Split ckeditor5_alignment CKEditor 5 plugin, to allow for more precise upgrade path, which led to #3259367: [upstream] Allow configuring *which* HTML tags can be aligned, which now led to #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal …

Walked @bnjmnm through most of the commit history starting at the beginning and going all the way to https://git.drupalcode.org/project/drupal/-/merge_requests/1628/diffs?co.... Things made sense to him so far. He made 3 great suggestions:

1. rename ::parse() to ::fromString(), for symmetry with ::fromFilterPluginInstance() and ::fromTextFormat()
2. for the diff and intersection set operations, one piece is tricky at first sight, but trivial once you realize that the validation of the array structure in the constructor provides allows one to make assumptions about the array structure — a simple @see makes that very clear
3. rename ::union() to ::merge(), because A) ::union() sounded unfamiliar and ::merge() does not (because array_merge()), B) "union" is not actually a verb/action, and "diff" and "intersect" are, but "unionize" … means something entirely different :P

FYI:

1711 insertions, 511 deletions.

→ this breaks down into:

• 866 new lines in the new \Drupal\ckeditor5\HTMLRestrictions
• … minus the 284 lines of the old and now deleted \Drupal\ckeditor5\HTMLRestrictionsUtilities
• 730 new lines in the new \Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
• all files that previously used something from HTMLRestrictionsUtilities have stayed equally big (2 files), or became smaller (7 files), and their logic always became simpler.

Addressed all of @lauriii's feedback. Implicitly addresses some of @larowlan. Will do Lee's tomorrow :)

Now also addressed all of @larowlan's feedback.

P.S.: it took four minutes for this to even begin queueing a test run 🤯 Also, all those "this line changed in version X of the diff" auto-comments are useless in the context of the d.o issue, because the parent context is not displayed. So much distraction on this issue 😔

Patch to make this committable to 9.3, 9.4 and 10.0.

Yep, that specific comment by @larowlan was also addressed. I just didn't mark it resolved to give @larowlan the opportunity to provide feedback, since that was the only non-trivial change in response to his feedback 😊🤓

Apparently 9.3.x differs from 9.4.x in nits — $ git diff origin/9.3.x origin/9.4.x -- core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraint* core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEd* yields 9.3-differences.txt.

git apply -3 handles these graciously. patch -p1 does not.

Uploading a patch-compatible 9.3.x patch. Re-uploading the patch in #51 to make it clear it's targeted for both 9.4.x and 10.0.x. Also uploading diff-between-patches.txt to show the actual differences between these two huge patches: it's caused entirely by 9.3-differences.txt!

YAY! This unblocks seven issues, all now have patches, one RTBC'able, one in progress, one PoC, 4 indicating next steps:

1. #3259493-14: [GHS] Unable to limit attribute values: ::allowedElementsStringToHtmlSupportConfig() does not generate configuration that CKEditor 5 expects — a 1 K patch adds both test coverage and a fix :)
2. #3260853-5: [GHS] Partial wildcard attributes (<foo data-*>, <foo *-bar-*>, <foo *-bar>) and attribute values (<h2 id="jump-*">) not yet supported — explicit failing tests and a partial fix — this is my next focus
3. #3228691-10: Restrict allowed additional attributes to prevent self XSS — PoC patch
4. #3231328-7: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets — failing test coverage patch
5. #3231336-4: Simplify HtmlRestrictions and FundamentalCompatibilityConstraintValidator now that "forbidden tags" are deprecated — failing @todo-removal patch
6. #3231334-4: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) — failing @todo-removal patch
7. #3260857-3: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values — failing @todo-removal patch

@aitala: woah! Can you share your exact filter_html configuration, or alternatively drush cget filter.format.YOUR_TEXT_FORMAT? That way I can try to reproduce the bug. I don't yet see how it's possible that this change caused breakage for you, but I take it very seriously! If you could provide me that info, I promise to dig into it immediately.