Exceptions for CKEditor 5 plugin definitions containing wildcard tags when PHP is built with libxml 2.9.14

I think ... create a page with a field that uses that text format puts this in Render a site unusable and have no workaround. territory so marking this as Critical

This cannot be caused by #3259593: Alignment being available as separate buttons AND in dropdown is confusing because that hasn't been cherry-picked to 9.3.x yet.

We are currently working on a project where we have decided to upgrade to ckeditor5 from the get go.

🥳 Many thanks!

<code>Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "ckeditor5_alignment" CKEditor 5 plugin definition has a value at "drupal.elements.0" that is not an HTML tag with optional attributes: "<$text-container class="text-align-left text-align-center text-align-right text-align-justify">". Expected structure: "<tag allowedAttribute="allowedValue1 allowedValue2">". in Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition->validateDrupalAspects() (line 146 of core/modules/ckeditor5/src/Plugin/CKEditor5PluginDefinition.php).

Yay for strict assumption checking, otherwise this would've caused problems that are much harder to check.

Can you please share the specific version of Drupal 9.3 you're on? Each release of Drupal 9.3 contains updates to the CKEditor 5 module, so just saying "9.3" is not specific enough. Either the patch release (e.g. 9.3.2) or the git commit if you're chasing 9.3.x HEAD.

The error will be thrown regardless on if alignment is in the toolbar or not.

This makes sense, because it's an exception thrown from CKEditor5PluginDefinition->validateDrupalAspects(), which happens when discovering the available CKEditor 5 plugins 👍

Thanks! We'll get this sorted out ASAP!

Fixing & shortening title.

Currently running 9.3.12

I've got access to 8.0.16 will see if it happens there too

This issue doesn't occur on 8.0.16, bumping to .19 to see what changes

Ok, so the issue here is that something has changed in PHP's Dom handling such that

<__preprocessed-wildcard-text-container__ class="text-align-left text-align-center text-align-right text-align-justify" /> is no longer parsed as an XML_ELEMENT_NODE, it now is parsed as a XML_TEXT_NODE element so this check in \Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions returns an empty element set

if ($node->nodeType !== XML_ELEMENT_NODE) {
  // Skip the empty text nodes inside tags.
  continue;
}

Which causes the failure

This seems to resolve it - not sure how we can test this until drupalci has 8.0.19

Nothing obvious stands out in the changelogs https://github.com/php/php-src/compare/PHP-8.0.16...PHP-8.0.19

The libxml version for the 8.0.19 container is 2.9.14
The older version had 2.9.12

I can't see anything obvious in the libxml changelogs https://gitlab.gnome.org/GNOME/libxml2/-/compare/v2.9.12...v2.9.14?from_...

According to the spec __ is a valid start character for a XML element https://www.w3.org/TR/REC-xml/#NT-Name

But I wonder if e.g this libxml commit means libxml is stricter when parsing HTML than before?

@mstrelan reported that this issue doesn't exist on PHP 8.0.19 with libxml 2.9.12, so that does point to libxml being the issue

Updating the title

@larowlan it was PHP 8.0.16 with libxml 2.9.12 that I tested, not 8.0.19.

Oh, sorry - i mixed that up

🐰🕳 Wow, what an EPIC rabbit hole! 👏🤣

Ironic that @larowlan was not able to reproduce this on 8.0.16 but on 8.0.19 (#9), and @mstrelan was able to reproduce it on 8.0.16 (#19). Maybe different builds of PHP with different libxml builds?

The change in #11 looks like a reasonable work-around. 👍 But it also means that \Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions() must suffer from the same problem, since the change in #11 is making CKEditor 5's PHP code deviate from the example in there, where it has had:

    // Protect any trailing * characters in attribute names, since DomDocument
    // strips them as invalid.
    $star_protector = '__zqh6vxfbk3cg__';
    $html = str_replace('*', $star_protector, $html);

since #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default.

No! 🙈 That's different! That was designed to allow attributes such as data-*. Successfully being able to use double underscore prefixes for tags is not an assumption we can therefore make based on pre-existing code.

So … time to test thoroughly across different PHP versions, especially thanks to @larowlan's findings in #10. I created a script (attached) to run on 3v4l.org: https://3v4l.org/2C0tY. It behaves consistently across all PHP versions. 😬 But it's still possible we need to do #11 since for 3v4l.org we cannot see what the libxml version is that is being used.

I'm hoping that @larolwn and @mstrelan can run this script on their respective environments where they were able to reproduce this. That should allow us to confirm the fix in #10.

fwiw i also couldn't replicate on 8.0.16 with 2.9.12

This is what @mstrelan reported to me

So he got the same results as me

Actually, this gets worse on 9.3.13

Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "ckeditor5_globalAttributeDir" CKEditor 5 plugin definition has a value at "drupal.elements.0" that is not an HTML tag with optional attributes: "<* dir="ltr rtl">". Expected structure: "<tag allowedAttribute="allowedValue1 allowedValue2">". in Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition->validateDrupalAspects() (line 142 of core/modules/ckeditor5/src/Plugin/CKEditor5PluginDefinition.php).

i.e the * element isn't supported either

Ah, but the patch above handles that :homer-bushes:

#23: not surprised, because that also gets transformed into a double underscore prefixed tag name, which #11 is indeed already addressing too.

So, even though this is very difficult to reproduce, because it depends on the specific libxml version that your PHP is built against, it's clear that A) some people are running into it, B) it's safe to change it.

This is a 100% non-public/internal change: it's a temporary data structure in a single function.

I went with a double underscore prefix simply to prevent a clash with a potentially custom tag. #11 changes that from a prefix of __preprocessed-TYPE-attribute__ to preprocessed-TYPE-attribute. Chances of a user having a tag called preprocessed-mytag-attribute are … essentially the same as the code currently in HEAD.

So let's land this 😊 Writing a failing test for this is impossible without having the Drupal Association provision DrupalCI with specific builds of PHP with a specific version of libxml, and that's definitely disproportional/excessive.

Per #23.

Committed and pushed c81f543972 to 10.0.x and 8faa516a49 to 9.5.x and 7c21c8f1f3 to 9.4.x and c858497aa0 to 9.3.x. Thanks!

diff --git a/core/modules/ckeditor5/src/HTMLRestrictions.php b/core/modules/ckeditor5/src/HTMLRestrictions.php
index aef9b29c89..800a13e922 100644
--- a/core/modules/ckeditor5/src/HTMLRestrictions.php
+++ b/core/modules/ckeditor5/src/HTMLRestrictions.php
@@ -356,8 +356,8 @@ private static function fromObjectWithHtmlRestrictions(object $object): HTMLRest
    */
   public static function fromString(string $elements_string): HTMLRestrictions {
     // Preprocess wildcard tags: convert `<$text-container>` to
-    // `<__preprocessed-wildcard-text-container__>` and `<*>` to
-    // `<__preprocessed-global-attribute__>`.
+    // `<preprocessed-wildcard-text-container__>` and `<*>` to
+    // `<preprocessed-global-attribute__>`.
     // Note: unknown wildcard tags will trigger a validation error in
     // ::validateAllowedRestrictionsPhase1().
     $replaced_wildcard_tags = [];
@@ -382,7 +382,7 @@ public static function fromString(string $elements_string): HTMLRestrictions {
     unset($allowed_elements['__zqh6vxfbk3cg__']);
 
     // Postprocess tag wildcards: convert
-    // `<__preprocessed-wildcard-text-container__>` to `<$text-container>`.
+    // `<preprocessed-wildcard-text-container__>` to `<$text-container>`.
     foreach ($replaced_wildcard_tags as $processed => $original) {
       if (isset($allowed_elements[$processed])) {
         $allowed_elements[$original] = $allowed_elements[$processed];

Fixed the code comments on commit.

D'oh, sorry, I should've caught those comments needing to be updated 🙈

Thank you!

Getting reproducible issue with newer libxml on the latest Debian bookworm (drupal-ci image for PHP 8.3) #3383577: TextSummaryTest:testLength() fails on some libxml versions