HTMLRestrictionsUtilities:: providedElementsAttributes() causes deprecations on PHP 8.1

Titling the issue properly. \Drupal\ckeditor5\HTMLRestrictionsUtilities seems a super important class. The docs seem a bit thin as I have no idea if PHP 8.1 is allowing us to find a significant issue or not.

The patch attached fixes the deprecation.

I cannot get \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest to pass locally regardless of PHP version because...

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "restricted_html can be switched to CKEditor 5 after dropping the two markup-creating filters (3 upgrade messages)" ('restricted_html', array(true, true), array(array(array('heading', 'bold', 'italic', '|', 'link', 'blockQuote', 'code', 'bulletedList', 'numberedList', 'sourceEditing')), array(array(array('heading2', 'heading3', 'heading4', 'heading5', 'heading6')), array(array('<cite>', '<dl>', '<dt>', '<dd>', '<a hreflang>', '<blockquote cite>', '<ul type>', '<ol start type>', '<h2 id>', '<h3 id>', '<h4 id>', '<h5 id>', '<h6 id>')))), '<br> <p>', array(array('CKEditor 5 only works with HT...ymore.', 'CKEditor 5 only works with HT...ymore.')), array('The following plugins were en...</em>.', 'The following tags were permi...d&gt;.', 'This format's HTML filters in...d&gt;.'), array('CKEditor 5 needs at least the...ilter.'))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     '' => Array &1 (
-        0 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert URLs into links</em>" (<em class="placeholder">filter_url</em>) filter implies this text format is not HTML anymore.'
-        1 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert line breaks into HTML (i.e. &lt;code&gt;&amp;lt;br&amp;gt;&lt;/code&gt; and &lt;code&gt;&amp;lt;p&amp;gt;&lt;/code&gt;)</em>" (<em class="placeholder">filter_autop</em>) filter implies this text format is not HTML anymore.'
+        0 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert line breaks into HTML (i.e. &lt;code&gt;&amp;lt;br&amp;gt;&lt;/code&gt; and &lt;code&gt;&amp;lt;p&amp;gt;&lt;/code&gt;)</em>" (<em class="placeholder">filter_autop</em>) filter implies this text format is not HTML anymore.'
+        1 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert URLs into links</em>" (<em class="placeholder">filter_url</em>) filter implies this text format is not HTML anymore.'
     )
 )

The array order is swapped.

Also in Drupal\Tests\ckeditor5\Kernel\ValidatorsTest the line $restricted_html_format_filters = Yaml::parseFile('profiles/standard/config/install/filter.format.restricted_html.yml')['filters']; causes problems for local tests. It assumes that the tests are being run in from /core which is incorrect. If I fix this line to determine the path relative to the test then the tests fails for the same array ordering reasons above.

We need to fix this brittle-ness. Will create an issue for making these test able to run locally.

Opened #3249263: CKEditor 5 needs validate the filters in the correct order - CKEditor 5 tests fail locally. for the local fails.

@andypost this get's set to FALSE in \Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlFilterArray() specifically the line that does:

          $elements[$tag] = FALSE;

It has nothing to do with upgrade paths.

I think I agree that #9 is the correct fix. But then we need to follow on from this and change\Drupal\ckeditor5\HTMLRestrictionsUtilities::allowedElementsStringToHtmlSupportConfig from

      assert($attributes === FALSE || is_array($attributes));
      if (is_array($attributes)) {
        foreach ($attributes as $name => $value) {
          assert($value === TRUE || Inspector::assertAllStrings($value));
          $to_allow['attributes'][$name] = $value;
        }
      }

to

      assert(is_array($attributes));
      foreach ($attributes as $name => $value) {
        assert($value === TRUE || Inspector::assertAllStrings($value));
        $to_allow['attributes'][$name] = $value;
      }

I think we might be able to lose the assert(is_array($attributes)); because we're going to foreach on it anyway.

And \Drupal\ckeditor5\HTMLRestrictionsUtilities::cleanAllowedHtmlArray() can also be cleaned up to be just

  public static function cleanAllowedHtmlArray(array $elements): array {
    // When recursively merging elements arrays, unkeyed boolean values can
    // appear in attribute config arrays. This removes them.
    return array_map('array_filter', $elements);
  }

Can also remove the if (is_array($attributes)) { check in \Drupal\ckeditor5\HTMLRestrictionsUtilities::toReadableElements()

Hmmm.... the problem is the code in \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::findPluginSupportingElement() with the change in #9 this is behaving differently as an empty array is considered to be offering less broad supported than an array with some elements. If this was a FALSE then it'd be considered to broader.

This is the key part of the code:

        // If the selected plugin and the plugin being checked both have arrays
        // for $tag configuration, they both have attribute configuration. Check
        // which attribute configuration is more permissive.
        if ($selected_plugin && is_array($selected_config) && is_array($provided_elements[$tag])) {

I think this has two consequences. The fix in #9 is not correct we need to keep the FALSE value to mean all all attributes. But also it means that the fix in #2 is wrong. We should not go from a FALSE value to an array value because that is making the config less broad.

After running tests and mucking about with ckeditor5 I'm not convinced the solution proposed #13 #13 is correct. Discovered where this ArrayPI comes from - it's all documented in \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions() - we need to support 'tag' => FALSE and 'tag' => ['class' => TRUE]

The fix in #2 is the correct fix but I think we can improve the documentation here to make it easier to work out in the future.

Patched attached is the same as #2 with the following additions:

• Improved the documentation of \Drupal\ckeditor5\HTMLRestrictionsUtilities to point to \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions() as that explains the array structure.
• Fixed incorrect @return type in HTMLRestrictionsUtilities::toReadableElements()
• Renamed HTMLRestrictionsUtilities::providedElementsAttributes to HTMLRestrictionsUtilities::addAllowedAttributeToElements() because that is what it does. I'm not sure what the original name was trying to say. The docs make it clear what the method does.... Adds allowed attributes to the elements array. - I've not changed them
• Used an early return instead of a variable in the method as this is clearer and less complex.
• Removed some premature optimisation noticed when renaming the method - there's no need to repeat the logic test for working out if tag/attribute combination is already set to allow all.
• Fixed the reference to \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::WILDCARD_ELEMENT_METHODS - that constant does not exist there. It's \Drupal\ckeditor5\HTMLRestrictionsUtilities::WILDCARD_ELEMENT_METHODS

I think of this is in-scope because the current docs and method name make it harder to work out what is going on.

I am digging into this too, but as part of reproducing this I ran into significant PHPUnit performance pains, and so I got nerdsniped into fixing those — see #3249443: drupal_phpunit_find_extension_directories() uses infinite recursion ⇒ more directories = slower tests.

Here's why the deprecation is occurring: https://3v4l.org/ahgI7

Fixing the issue title to reflect the fact that the current behaviour is correct we just don't want it to trigger a deprecation.

1. #2:
   

   \Drupal\ckeditor5\HTMLRestrictionsUtilities seems a super important class. The docs seem a bit thin as I have no idea if PHP 8.1 is allowing us to find a significant issue or not.

   I 💯% agree! That's why for a long time I've been advocating for #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object to happen, with thorough unit test coverage and significantly improved docs. In fact, I already made good progress on that issue, but newly CKEditor 5 JS-related data loss criticals forced me to pushing it forward.

   (It's not yet been moved into core's ckeditor5.module component because I still need to move the MR over.)

2. #8: the interpretation FALSE means that no attributes allowed for tag is correct. But
   

   looking at data structures it could be optimized

   is wrong. We can't and shouldn't change this. This structure has meaning: it's the same structure as \Drupal\filter\Plugin\FilterInterface::getHTMLRestrictions()'s return value.

3. For that reason, #9 is incorrect, as #13 also concluded.
4. #13: Yep, it's an ArrayPI. Well, actually … it's not. It's just a complex datastructure with a lot of semantics embedded in the structure of the array. Wait, I guess that means it is an ArrayPI? 🤓😅

   You saying that makes me want to point to #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object once more. Reading between the lines, I suspect you agree that issue is critically important?

5. #15: +1 to all that, and … sorry you had to suffer through it. This pain is why I've been advocating for that for months 🙈
   

   +++ b/core/modules/ckeditor5/src/HTMLRestrictionsUtilities.php
   @@ -141,21 +153,35 @@ public static function cleanAllowedHtmlArray(array $elements): array {
   +    if (isset($elements[$tag]) && $elements[$tag] === FALSE) {
   +      // If the tag is already allowed with no attributes then the value will be
   +      // FALSE. We need to convert the value to an empty array so that attribute
   +      // configuration can be added.
   +      $elements[$tag] = [];
   +    }
   

   This is the critical addition to avoid the PHP 8.1-only deprecation error.

The change looks good and makes sense to me. Nice work with all the documentation additions @alexpott.

Looking forward to seeing how #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object ends up - plus one for a value object.

Committed 65aaec6 and pushed to 9.4.x.

Backported to 9.3.x