Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values

#3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object is in!

We can now proceed here. This patch is for now just deleting the early return that was necessary up until we'd start working on this issue. We should begin with adding more test coverage.

Only CKE5 tests.

Should fail just like #3, perhaps in more places, because >1 month worth of new tests have been added since then.

Test attribute already provided by enabled plugin.

❌ This new test should fail!

Note: this is not yet enough to remove Needs tests — we also still need to test the "disabled plugin" case.

Support attribute detection.

✅ Should pass the test added in #7.

Huh, the patches in #7 and #8 are wrong, but the interdiffs are right 🙈

That's better 😊😅

We can see now that the (correct) #7 patch causes one additional failure thanks to its extra test coverage, and the (correct) #8 patch fixes that one failure.

Progress! 🥳

Now it's time to add the test coverage that #7 mentioned is still needed. Which allows us to remove the Needs tests tag.

❌ This new test should fail!

Note: this not only tests the "support for this manually enabled attribute can be provided by some disabled plugin" case, but simultaneously verifies that SourceEditingRedundantTagsConstraintValidator finds such plugins even if it's provided by a wildcard element. 👍

Make the SmartDefaultSettingsTest failures easier to debug.

(This copies the pattern from \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::test().)

#18 made it clear that \Drupal\ckeditor5\Plugin\Validation\Constraint\SourceEditingRedundantTagsConstraintValidator::validate() is too eager.

We're almost there — it seems all of the failures are for the tricky wildcard case that #16 added.

This also made me realize that the existing test coverage is not clear enough and does not yet quite cover all edge cases. Expand it.

(For this interdiff, git diff --color-words would've been a lot clearer.)

#22 caused ValidatorsTest to fail … "harder".

This will make the <img src> test case added in #22 pass, so that we're back at the same exact set of failures as in #20. But now more firmly in the final stretch 🤓

In scrutinizing #24 I realized there was one more missing test that would not need a violation message: the case of an attribute not supported by any plugin.

CKEditor5AllowedTagsTest::testFullHtml() has been failing because <img src alt height width data-entity-type data-entity-uuid data-align data-caption> is in the source editing list, because SmartDefaultSettings cannot enable it. This has been the case all along, but we simply didn't have the validation logic nor the test coverage up until now!

It resulted in a validation error like this: The following tag(s) are already supported by available plugins and should not be added to the Source Editing "Manually editable HTML tags" field. Instead, enable the following plugins to support these tags: .

First, let's add a kernel test equivalent of CKEditor5AllowedTagsTest::testFullHtml() — to test explicitly what was tested implicitly until now.

The fix for CKEditor5AllowedTagsTest::testFullHtml() and the test case added in #28.

And finally, the fix for the tricky edge case for which a failing test was added in #16!

✅ Should pass all tests now!

The two remaining failures in the one failing test have existed since the beginning, since #3. They're very similar:

--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    'settings.plugins.ckeditor5_sourceEditing.allowed_tags.13' => 'The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Image caption (<img data-caption>).'
+)

+

--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    'settings.plugins.ckeditor5_sourceEditing.allowed_tags.13' => 'The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Image align (<img data-align>).'
+)

in both cases, the validation constraint is complaining that a particular plugin is already enabled. But … that is impossible.

Turns out that this has been a subtle bug in SmartDefaultSettingsTest for all this time! 🙃 The bug: it was using the stored FilterFormat rather than the updated one.

… and #34 fixed the two failing test cases, but it broke two others. Turns out that there's been another bug hiding in SmartDefaultSettingsTest's infrastructure for a long time! 🙃

A bug never comes alone.

🤓🤓🤓🤓🤓🤓🤓🤓 All of this is reminding me an awful lot about SmartDefaultSettings.

Just look at this:

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingRedundantTagsConstraintValidator.php
@@ -57,32 +70,64 @@ public function validate($value, Constraint $constraint) {
+    $disabled_plugin_overlap = $disabled_plugin_tags
+      // Merge the enabled plugin tags, to allow wildcards to be resolved.
+      ->merge($enabled_plugin_tags)
+      // Compute the overlap.
+      ->intersect($source_enabled_tags)
+      // Avoid including the overlap known to exist for just enabled plugins.
+      ->diff($enabled_plugin_overlap);
     foreach ([$enabled_plugin_overlap, $disabled_plugin_overlap] as $overlap) {
       $checking_enabled = $overlap === $enabled_plugin_overlap;
       if (!$overlap->isEmpty()) {

Especially now that we just finished #3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets.

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingRedundantTagsConstraintValidator.php
@@ -90,16 +135,34 @@ public function validate($value, Constraint $constraint) {
+        // Skip plugins that provide a subset, only mention the plugin that
+        // actually provides the overlap.
+        // For example: avoid listing the image alignment/captioning plugins
+        // when matching `<img src>`; only lists the main image plugin.
+        if (!$overlap->diff($plugin_capabilities)->isEmpty()) {
+          continue;
+        }

… and this is pretty much a less advanced version of \Drupal\ckeditor5\SmartDefaultSettings::computeNetNewElementsForPlugin()!

What SourceEditingRedundantTagsConstraintValidator does is basically the same thing as SmartDefaultSettings: the "needed" elements are the ones configured by the user as "manually editable tags"; the goal is to find enabled or disabled plugins that meet those needs, because that would mean that a better solution exists, and hence that should be the validation error.

While we could do that, perhaps that's best left as a follow-up? I think that'd be too much of a scope increase. Especially because it would require refactoring SmartDefaultSettings, to intentionally open up some private methods.

#38

• 1+4: Of course — just wanted to keep this fast to iterate on initially :D
• 2: ✅ Note that this is just for generating a message, not for actually enabling some plugin. But yes, in principle this should take into account how much "surplus" would be added, and that then is basically the same as \Drupal\ckeditor5\SmartDefaultSettings::computeSurplusScore() — in other words: all the more reason to do what I proposed in #36 … 🤓
• 3: ✅ Yes, the INVALID Source Editable tag already provided by plugin and another available in a not enabled plugin test case in ValidatorsTest::testPair(). Attached a patch here to prove that without this, tests fail.

phpcs getting in the way just when proving something … 😬

#36 seems like something that could be handled by a follow-up.

Follow-up created: #3271179: Update SourceEditingRedundantTagsConstraintValidator to reuse SmartDefaultSettings. Already posted PoCs there that are largely passing tests.

1. The failing test in #40 proved what I wrote in #39, in response to #38.3.
2. #42: That's a pre-existing limitation. Dealing with such nuance is out of scope here; the scope here was only to make it actually inspect attributes too. And actually … #3271179: Update SourceEditingRedundantTagsConstraintValidator to reuse SmartDefaultSettings would make that 10x simpler to do, because SmartDefaultSettings already contains this kind of logic! I'm happy to include that in that scope too.
3. Attached patch runs all tests again, not just CKE5.

Huh, apparently the patch does not apply to 9.3 due to a slight difference in 9.3 caused by #3106216: Remove unused variables from core 😬

$ git diff origin/9.3.x origin/9.4.x
   private function pluginsSupplyingTagsMessage(HTMLRestrictions $overlap, array $plugin_definitions): string {
     $message_array = [];
     $message_string = '';
-    foreach ($plugin_definitions as $plugin_id => $definition) {
+    foreach ($plugin_definitions as $definition) {
       if ($definition->hasElements()) {
         $plugin_capabilities = HTMLRestrictions::fromString(implode(' ', $definition->getElements()));
         foreach ($plugin_capabilities->intersect($overlap)->toCKEditor5ElementsArray() as $element) {

git apply -3 applies the patch in #44 just fine … but here's a 9.3-specific one anyway.

The 9.3 patch only fails because #3263384: Add ckeditor5-code-block package and CodeBlock plugin has not yet been committed to 9.3, which will happen in #3269651: Update Drupal 9.3.x to CKEditor 5 v34.0.0 along with other un-backported issues. IMHO we should apply the same strategy here: commit to 10+9.4, and cherry-pick the commit in #3269651, along with #3263384.

1. ✅ Correct!
2. ✅ Clarified :)
3. ✅ Great suggestion — followed your example exactly :)
4. ✅ Clarified :)
5. ✅ No. We start with the evaluated plugin's capabilities ($plugin_capabilities). If it has wildcards, we first merge the elements already allowed by enabled plugins ($enabled_plugin_restrictions). This allows the wildcards of $plugin_capabilities to resolve into concrete tags. Then when we omit the $enabled_plugin_restrictions again, the resolved tags remain. These are the tags that would actually be added.

   I first thoguht you meant "what if there is a combination of wildcard and concrete tags" in $enabled_plugin_restrictions. This shows that that is not the case:

   

   Then I realized you might've meant $plugin_capabilities instead. Well, this shows that that is also not the case:
   

   As you can see, in both cases, the result in $test contains the net new HTML elements supported by adding the new plugin whose capabilities are shown in $plugin_capabilities.

   I tried to be thorough 🤓

6. ✅ Fundamental compatibility errors do not have a property path on the text editor object. That's why the "path" of the violation is the empty string. Examples:
   

   '' => 'CKEditor 5 needs at least the &lt;p&gt; and &lt;br&gt; tags to be allowed to be able to function. They are not allowed by the "<em class="placeholder">Limit allowed HTML tags and correct faulty HTML</em>" (<em class="placeholder">filter_html</em>) filter.',
   

           '' => [
             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.',
           ],
   

   See \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair(), which contains:

       if (!$all_compatibility_problems) {
         foreach ($violations as $i => $violation) {
           // Remove all violations that are not fundamental — these are at the
           // root (property path '').
           if ($violation->getPropertyPath() !== '') {
             $violations->remove($i);
           }
         }
       }
   

   Based on context, I assume any remaining fundamental errors are in '', but that's just a guess.

   That's right! The difference is that \Drupal\Tests\ckeditor5\Kernel\CKEditor5ValidationTestTrait::validatePairToViolationsArray() doesn't omit anything (because in tests we want to be as strict and complete as possible), whereas \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() allows you to omit those :)

#51's 9.3.x patch failed only due to #3263384: Add ckeditor5-code-block package and CodeBlock plugin not yet having been cherry-picked. Let's verify that that is true.

Follow-up: #3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by.