HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers

Actually, it's not just that. It's also ::toGeneralHtmlSupportConfig(). Adding test coverage for that too.

At least #3 is a trivial fix. The assertion is pointless and wrong, so we can just remove it.

#9: Woah … yes indeed … 😬 Great find! Sorry for misleading you there 🙈

Messed around with RecursiveIteratorIterator and RecursiveArrayIterator to replace array_merge_recursive but couldn't make it work like I want to.

Too bad! 😬

Reproduced the failure locally on 9.5.x with both PHP 7.4 and 8.0.

$ git log --oneline -n 1
47c9c02376 (HEAD -> 9.5.x, origin/9.5.x) Issue #3274651 by Wim Leers, nod_, alexpott: Impossible to enable <ol type> or <ul type> with GHS: switch to List's successor, DocumentList
$ php --version
PHP 8.0.12 (cli) (built: Oct 21 2021 14:38:26) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.12, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.12, Copyright (c), by Zend Technologies
$ curl -O https://www.drupal.org/files/issues/2022-05-28/core-html-3274648-14.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 14968  100 14968    0     0  33410      0 --:--:-- --:--:-- --:--:-- 33336
$ git apply -3v core-html-3274648-14.patch
Checking patch core/drupalci.yml...
Applied patch to 'core/drupalci.yml' cleanly.
Checking patch core/modules/ckeditor5/src/HTMLRestrictions.php...
Applied patch to 'core/modules/ckeditor5/src/HTMLRestrictions.php' cleanly.
Checking patch core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php...
Applied patch to 'core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php' cleanly.
Checking patch core/package.json...
Applied patch to 'core/package.json' cleanly.
Applied patch core/drupalci.yml cleanly.
Applied patch core/modules/ckeditor5/src/HTMLRestrictions.php cleanly.
Applied patch core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php cleanly.
Applied patch core/package.json cleanly.
$ vendor/bin/phpunit --configuration core/phpunit.xml core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
PHPUnit 8.5.26 #StandWithUkraine

Testing Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest
............................................................F..  63 / 159 ( 39%)
..FF........................................................... 126 / 159 ( 79%)
.................................                               159 / 159 (100%)

Time: 4.47 seconds, Memory: 10.00 MB

There were 3 failures:

1) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testRepresentations with data set "realistic" (Drupal\ckeditor5\HTMLRestrictions Object (...), array('<a href hreflang="en fr">', '<p data-* class="block">', '<br>'), '<a href hreflang="en fr"> <p ...> <br>', array(array('a', array(array('href', true), array('hreflang', array(array('/^(en|fr)$/'))))), array('p', array(array(array(array('/^data-.*$/')), true)), array(array('/^(block)$/'))), array('br')))
assert($value === TRUE || $tag === '*' && $value === FALSE) in /Users/wim.leers/Work/d8/core/modules/ckeditor5/src/HTMLRestrictions.php:1100

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:737
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:659
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:235
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:194

2) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testRepresentations with data set "<h2 id="jump-*">" (Drupal\ckeditor5\HTMLRestrictions Object (...), array('<h2 id="jump-*">'), '<h2 id="jump-*">', array(array('h2', array(array('id', array(array('/^(jump-.*)$/')))))))
assert($value === TRUE || $tag === '*' && $value === FALSE) in /Users/wim.leers/Work/d8/core/modules/ckeditor5/src/HTMLRestrictions.php:1100

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:737
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:659
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:235
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:194

3) Drupal\Tests\ckeditor5\Unit\HTMLRestrictionsTest::testRepresentations with data set "<ol type="1 A">" (Drupal\ckeditor5\HTMLRestrictions Object (...), array('<ol type="1 A">'), '<ol type="1 A">', array(array('ol', array(array('type', array(array('/^(1|A)$/')))))))
assert($value === TRUE || $tag === '*' && $value === FALSE) in /Users/wim.leers/Work/d8/core/modules/ckeditor5/src/HTMLRestrictions.php:1100

/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestResult.php:737
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:626
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:659
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:235
/Users/wim.leers/Work/d8/vendor/phpunit/phpunit/src/TextUI/Command.php:194

FAILURES!
Tests: 159, Assertions: 528, Failures: 3.

Are you sure you have zend.assertions=1 in your php.ini?

This now blocks #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path, which is major and a stable blocker.

Review

As the author of the current \Drupal\ckeditor5\HTMLRestrictions::merge() (after having adapted it from @bnjmnm's code, but sprinkled a lot of extra complexity on it): thank you, this is MUCH simpler! 🤩🙏

Even the diffstat say this pretty objectively (when you exclude the new test cases and the DrupalCI changes to restrict to CKE5 tests only):

$ git diff HEAD
 core/modules/ckeditor5/src/HTMLRestrictions.php | 144 ++++++++++++++++++++++++---------------------------------
 1 file changed, 61 insertions(+), 83 deletions(-)

1. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -694,113 +694,90 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
   +  private function _merge_config_arrays(array $array1, array $array2): array {
   

   Two things:
   A) this is stateless. So let's change this to static
   B) Drupal core does not use underscore-based naming in methods.

2. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -694,113 +694,90 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
   +      // There is no keys in common, simply append the arrays.
   

   s/is/are/

3. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -694,113 +694,90 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
   +      // There are some keys in common that needs to be merged.
   

   s/needs/need/

4. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -694,113 +694,90 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
            // If the HTML tag restrictions for only one of the two operands was a
            // boolean, then the result of array_merge_recursive() is an array
            // containing the complete contents of the non-boolean operand plus an
            // additional key-value pair with the first numeric key: 0.
   

   Comment is outdated. Needs to be rewritten.

5. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -694,113 +694,90 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
              // If the boolean was FALSE (meaning: "no attributes allowed"), then
              // the other operand's values should be used in an union: this yields
              // the most permissive result.
   

   Comment is outdated. Can be deleted altogether I think 🥳

Final review round — will RTBC next 🤓 🥳

1. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -693,6 +693,67 @@ public function doIntersect(HTMLRestrictions $other): HTMLRestrictions {
   +   * Merge the configuration array according to HTMLRestrictions rules.
   ...
   +   *   The first restriction array.
   ...
   +   *   The second restriction array.
   ...
   +  private static function mergeConfigurationElements(array $array1, array $array2): array {
   

   "configuration array" vs "restriction array" vs "configuration elements"

   Let's make these consistent.

   This is referring to \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions()'s "allowed elements".

   That's why \Drupal\ckeditor5\HTMLRestrictions::$elements is described as "An array of allowed elements."

   So I propose to call this mergeAllowedElementsLevel(). Why "level"? Because it is able to recurse one time, because this processes one level: either the tags' attributes, or the attributes' attribute values.

2. +++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
   @@ -1109,7 +1074,10 @@ public function toGeneralHtmlSupportConfig(): array {
   +            // transform the "1" string into 1 the number when it is used as an
   

   s/transform/transforms/

Yay! This unblocked #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path and #3274635: [upstream] Use CKEditor 5's native <ol type> and <ul type> UX, plus it allowed me to close #3279470-7: HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state 🥳