SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets

This should wait for #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object to finish; that will make this simpler.

Once #3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) lands, the impact of this will be much clearer thanks to us being able to refine that test coverage in this issue! 😊

#3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute) seems to already solve this … but it does not. It still picks the first CKEditor 5 plugin which addresses a need.

This issue is basically saying we should pick the most specific one. That would avoid (to the degree this is possible) that the upgrade path results in more tags/attributes/attribute values being allowed after the upgrade than before.

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

This adds the necessary test coverage: this should fail.

(And actually, the test coverage should expect the center and justify alignment buttons to appear, not the generic one. But this initial patch demonstrates the problem most clearly. A different set of buttons being selected is what will help meet this expectation.)

(And actually, the test coverage should expect the center and justify alignment buttons to appear, not the generic one. But this initial patch demonstrates the problem most clearly. A different set of buttons being selected is what will help meet this expectation.)

And actually … if #3259593: Alignment being available as separate buttons AND in dropdown is confusing lands first, then that part will remain the same, and then the patch in #7 is perfect in terms of updated expectations!

As of #3263384-10: Add ckeditor5-code-block package and CodeBlock plugin, this issue blocks #3263384, and since that is Major, I have to bump this up too. Fortunately we already have a test case: #7.

This updates \Drupal\ckeditor5\SmartDefaultSettings::addToolbarItemsToMatchHtmlAttributesInFormat() to make the test in #7 pass.

Note: #3263384: Add ckeditor5-code-block package and CodeBlock plugin needs us to minimize the superset of tags, not attributes. So more work is needed: \Drupal\ckeditor5\SmartDefaultSettings::addToolbarItemsToMatchHtmlTagsInFormat() should also be updated. But this is already a key step forward.

Will tackle #11 next, but would love a review in the meantime — hoping @bnjmnm has time, since he was the original architect of this algorithm 🤓😊

cspell…

To help reviewing #11:

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   -          $plugin_support = HTMLRestrictions::fromString(implode(' ', $definition->getElements()));
   -          // Do not inspect just $plugin_support, but the union of that with the
   -          // already supported elements: wildcard restrictions will only resolve
   -          // if the concrete tags they support are also present.
   -          $potential_future = $provided->merge($plugin_support);
   -          // This is the heart of the operation: intersect the potential future
   -          // with what we need to achieve, then subtract what is already
   -          // supported. This yields the net new elements.
   -          $net_new = $potential_future->intersect($still_needed)->diff($provided);
   +          [$net_new, $superset] = self::computeNetNewElementsForPlugin($provided, $still_needed, $definition);
   

   This has just been moved into a new helper, that computes exactly $net_new as it exists in HEAD, but also $superset (which is basically how much adding this plugin results in "overshooting": which HTML elements it adds that we didn't actually want/need).

2. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   -                $plugins_to_enable_to_support_attribute_config[$plugin_id][$attribute_name][$tag_name] = $attribute_config;
   +                $superset_score = count($superset->getAllowedElements(), COUNT_RECURSIVE);
   +                if (!is_array($attribute_config)) {
   +                  $plugin_candidates[$tag_name][$attribute_name][$attribute_config][$superset_score] = $plugin_id;
   +                }
   +                else {
   +                  foreach ($attribute_config as $allowed_attribute_value => $allowed_attribute_value_config) {
   +                    $plugin_candidates[$tag_name][$attribute_name][$allowed_attribute_value][$allowed_attribute_value_config][$superset_score] = $plugin_id;
   +                  }
   +                }
   

   Rather than selecting a plugin, this merely lists all candidates, and provides a score to select one later.

3. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   -            // Fewer attributes are still needed.
   -            $still_needed = $still_needed->diff($net_new);
   

   Because we didn't select, only found candidates, this no longer needs to be updated in every loop.

4. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +      // Make a selection in the candidates: minimize the superset score, to
   +      // avoid generating supersets whenever possible.
   

   This now makes a selection. It chooses the plugin which adds what we needs with the smallest possible superset.

5. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
   @@ -515,7 +515,8 @@ public function provider() {
   -              'alignment',
   +              'alignment:center',
   +              'alignment:justify',
   

   Tada! 🤩

Test coverage for what I described as still remaining in #11.

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -406,6 +406,40 @@ private function addToolbarItemsToMatchHtmlTagsInFormat(FilterFormatInterface $f
   +    $superset = $potential_future->diff($needed)->diff($baseline);
   
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +                $superset_score = count($superset->getAllowedElements(), COUNT_RECURSIVE);
   

   The explanation for $potential_future is very clear, I think it would be worth doing the same for $superset.

   Also, wouldn't $superset be an inaccurate name since it diffs against baseline? Perhaps something like $surplus_additions would be more accurate and descriptive?

2. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
                foreach ($net_new->getAllowedElements() as $tag_name => $attributes_config) {
                  foreach ($attributes_config as $attribute_name => $attribute_config) {
   -                $plugins_to_enable_to_support_attribute_config[$plugin_id][$attribute_name][$tag_name] = $attribute_config;
   +                $superset_score = count($superset->getAllowedElements(), COUNT_RECURSIVE);
   +                if (!is_array($attribute_config)) {
   

   Would it be better to move $superset_score up a few levels of nesting? The performance benefits of avoiding the repeat calls are likely negligible. The biggest benefit to me is the move makes it more apparent to code viewers that the score is not dependent on which iteration within the loop(s) it is being determined.

3. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,23 +479,57 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +      // Make a selection in the candidates: minimize the superset score, to
   +      // avoid generating a superset whenever possible.
   

   I think the superset terminology could be changed to more accurate represent what is happening - find the candidate with the lowest [surplus elements?] score to avoid allowing excess tags/attributes when possible.

   Using the broader mathematic terms in HTMLRestrictions makes sense since it's responsible for making the actual comparisons. When dealing with the data processed by HTMLRestrictions, shifting the descriptions to be more context specific could be helpful.

4. The $superset_score is computed like this:

                   $superset_score = count($superset->getAllowedElements(), COUNT_RECURSIVE);
   
   

   Is there any risk of giving a more permissive superset a lower score due to it configuring attributes/attribute values as TRUE instead of an array of allowed values?

   • If it's NOT a risk, see if there's an organic way to mention that in a comment.
   • If there is a risk, but it's minimal and addressing would significantly bloat the scope of this issue, a comment explaining that would be fine, perhaps it could point to a @todo to an issue of not-yet-determined priority.
   • If the risk is enough to get ugly, consider addressing in this issue's scope.

6. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
   @@ -515,7 +515,8 @@ public function provider() {
   -              'alignment',
   +              'alignment:center',
   +              'alignment:justify',

   I was going to point mention this test depends on the redundant (and likely to be removed) alignment plugin, but more tests were added as I was reviewing this 🙂

\Drupal\ckeditor5\Plugin\CKEditor5PluginManager::findPluginSupportingElement() is too simplistic: it returns the first match it founds, without any consideration for whether it's the best (minimal!) match. That's what the test coverage added in #16 is proving through its failure.

That means that even though that's a public API, we should deprecate it; its only use is being dropped in this patch. I'll leave that for the next patch iteration.

This patch iteration drastically simplifies SmartDefaultSettings. Rather than having lots of complex logic to deal with the "HTML restrictions" array structure, we can now leverage the infrastructure that #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object introduced! That removes the need for the two addToolbarItemsToMatchHtmlTagsInFormat() + addToolbarItemsToMatchHtmlAttributesInFormat() methods — just one will do now! :)

P.S.: Can I just point out that diffstat for the interdiff? 🤓 🤩

 3 files changed, 110 insertions(+), 114 deletions(-)

→ fixing things by having less code, and that's even without deleting the now unused \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::findPluginSupportingElement()! 🥳 Rare occasion, but so nice when it happens!

#18:

1. ✅ Ohhhh, good point! Done!
2. ✅ Hah, already did that in #19 — out of necessity, but happily also fixes this! 😁
3. ✅ Works for me — done!
4. ✅ Hm … this is a good point. Thinking about this … 🤔 … I think the risk is minimal (very few if any CKEditor 5 plugins will ever allow ALL attributes on a given tag!), but I think we can mitigate that fairly easily, so … here we go!
5. (empty)
6. 😁

This patch makes the MR in #3263384: Add ckeditor5-code-block package and CodeBlock plugin pass

Great! :D Thanks for checking that :)

Do you think the issue described over on #2710427 is related to what y'all are working on here? (#2710427: Broken "Allowed Tags" updating: after all values for an attribute are allowed, it should not be overridden to allow only certain attribute values)

I tried the patch on #20, and it didn't fix the issue we're having on that other issue, but holy heck does SmartDefaultSettings look and feel related, so I figured it can't hurt to ask.

Thank you!

This issue definitely cannot be related, since this issue only affects the CKEditor 5 module!

1. Not sure I undertand why we use the string <unspecified:true> and <unspecified:false>, we add a bunch of sprintf and then do a preg_match to see what's the value. Why the "<>". We can do a TRUE/FALSE key in the array maybe? then the pregmatch can be replace by is_bool. It's a magic key either way, not like people will see what's in there.

2. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,39 +394,160 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +              $surplus_count = count($surplus_additions->getAllowedElements(), COUNT_RECURSIVE);
   

   This makes it so that <code class data-language> has the same score of 3 as <code class="language">. I would expect the first to have 3 and the second to have 2 as the score, which means having the count to stop at depth 2 (if we start depth at 1) and ignore the rest no?

3. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,39 +394,160 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +              $surplus_score = $surplus_count + 100 * count(array_filter($surplus_additions->getAllowedElements(), function ($tag_config) {
   +                return $tag_config === TRUE;
   +              }));
   

   In the case of codeblock, if the code plugin has <code data-language> for the elements config, then this doesn't work as expected and codeBlock is used instead because the 2 plugins share the same score and codeBlock is the last plugin to be processed (which overrides the plugin_id for the surplus_score.

   It's fine if we assume that the most appropriate plugin has the least amount of additional attributes

4. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,39 +394,160 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +                $plugin_candidates[$tag_name][$non_specific_attribute][$surplus_score] = $plugin_id;
   

   I would use ...[$plugin_id] = $surplus_score;
   to avoid overwrites.

+++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
@@ -444,39 +394,160 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
+                $enabled_for_tags_message_content .= "$label (for tags: $tags) ";
...
               $enabled_for_attributes_message_content .= "$label (";
...
                 $enabled_for_attributes_message_content .= " for tag: <$tag_name> to support: $attribute_name";

Was a problem previously but this is not translatable like this.

#26:

1. It's just a string representation for the values in [$tag => TRUE] and [$tag => FALSE] (as opposed to [$tag => […]]). I wanted to make 100% sure that it could not be mistaken/interpreted for an attribute name. By using this weird syntax, it's very obvious that this is NOT an attribute name 🤓

   We cannot use TRUE or FALSE as the key names, sadly. That's what I had at first — it gets cast to integers automatically: https://3v4l.org/k2BKZ. That's why I made these string representations. But … you're right, it does work 99% of the time, and it's for internal use only. The only thing that's kinda tricky is the interpretation of what is a boolean key when iterating over them, because they're no longer booleans. But we can deal with that.

   Implemented your suggestion! 😊

   … and just prior to uploading the patch, I noticed that it fails. So there is a subtle problem caused by relying on the PHP typecasting after all. But please do double-check the changes I made!

2. Hm … 🤔 I think you're right that <code class data-language> would need a higher score than <code class="language">. But I disagree with stopping the count at depth 2 — because <code class="language foo bar"> should have a higher score than <code class="language">. Will tackle that next.
3. See previous point.
4. I … do not understand 🙈 Also, I've never even seen that syntax! 🤯

#27: yep, definitely out of scope here. I think it makes sense to tackle this as part of #3245967: Messages upon switching to CKEditor 5 are overwhelming.

Now addressing #26.2+3.

For #4 it's not real syntax, just didn't want to copy paste the code... This is trying to avoid overwrites when 2 plugin have the same score.

For #2 stopping at depth 2 isn't the right solution you're right.

This issue definitely cannot be related, since this issue only affects the CKEditor 5 module!

Whoops! Thanks for explaining for me!

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -515,8 +513,8 @@ private function addToolbarItemsToMatchHtmlElementsInFormat(FilterFormatInterfac
   +          $selected_plugins[$selected_plugin_id] = array_intersect_key($reason, [FALSE => TRUE]);
   

   Yeah this coupled with the implicit casting to number is not great for readability/long term maintainance. Maybe we can find a couple of words, like attributes-none and attributes-any

2. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -538,7 +536,9 @@ private function addToolbarItemsToMatchHtmlElementsInFormat(FilterFormatInterfac
   -              if (preg_match('/^\<unspecific:(true|false)\>$/', $attribute_name)) {
   +              // TRICKY: we cannot use strict equality checks here because PHP
   +              // converts boolean array keys to numerical keys.
   +              if ($attribute_name == TRUE || $attribute_name == FALSE) {
   

   I think it's the preg_match that bothered me the most here. We could also do a in_array for the two possible values instead of a regex. That would be less bad (for me at least)

#32:

1. Maybe we can find a couple of words, like → the problem is that those technically are valid attribute names. That's why I went with the angular brackets.
2. 👍

AFAICT #32 is saying that the previous approach kind of was okay/clearer in some contexts, but that the preg_match() was especially weird/confusing. I'm then also assuming that the use of angular brackets made it look like HTML and made it extra confusing. SO! Simpler proposal: rather than <unspecific:true> and <unspecific:false> (mine in #20) or the problematic TRUE/FALSE (#28), I'm also not going to go with attributes-none or attributes-any as proposed in #32 because that too can lead to edge cases. I've got something far simpler: -true- and -false-, or -any- and -none-, because leading dashes are not allowed in attributes. No confusion with HTML tags possible :)

I'm gonna go with -attributes-any- and -attributes-none-, since that stays closer to the spirit of #32! 😊

This addresses #26.4 — this indeed is far clearer/better/simpler! I'm really glad you caught that! 😄

Very nice solution to prefix the names by "-" making sure they are not valid attributes. +1 to the new names, it's clearer and more… specific than "unspecific" :)

This addresses #26.2+3. Still needs tests.

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,39 +394,178 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +              $surplus_score += 100 * count(array_filter($surplus_additions->getAllowedElements(), function ($tag_config) {
   

   that's supposed to be 1000 here I think

2. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -444,39 +394,178 @@ private function addToolbarItemsToMatchHtmlAttributesInFormat(FilterFormatInterf
   +              //   values. Tweak the base heuristic:  equate this case with
   

   Missing a line, comment not finished here.

3. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_plugin_conditions_test/ckeditor5_plugin_conditions_test.ckeditor5.yml
   @@ -12,3 +12,16 @@ ckeditor5_plugin_conditions_test_plugins_condition:
   +      - <code class="lang-*">
   

   technically it's language-* that is allowed here, that's what most people use, not "lang" since it's a test pretty minor comment though.

Refactor to make it possible to write tests, and make it possible to write better docs.

(Marking Needs work for the tests.)

#37.1 & #37.2: yep, and the tests I'm working on will should confirm/reveal those bugs :)

#37.3: Well, it was just meant to be a placeholder equivalent for #3263384: Add ckeditor5-code-block package and CodeBlock plugin anyway — the codeBlockPlaceholder toolbar item will also need to be changed in #3263384!

Tests written. This was tricky, because no unit tests existed yet. Writing kernel tests for this would be extremely unwieldy. Fortunately there was one precedent already in the CKEditor 5 module's unit tests 👍

Tests should fail, because of #37.1 at the very least.

And here's the fix. This fixes #37.1 + #37.2. #40 added tests to show that #26.2 + #26.3 have been addressed, but tests were failing; now they should pass, which means that it is indeed proven.

If @nod_ finds more nuance he wants to add to the surplus scores, we can now easily do that, because expanding test coverage is now trivial 👍

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -326,88 +325,149 @@ protected function getEnabledCkeditor4Plugins(EditorInterface $editor): array {
   +              // Count the number of surplus additions: tags, attributes,
   

   I would move the surplus count in a separate method it'll get complex pretty fast.

2. I think we're missing two (maybe 3) levels:
   1. Attributes names

      config	score	expected*
      <code data-dada data-tata>	203	2200
      <code data-*>	102	10000

   2. Attributes values

      config	score	expected*
      <code class="language1 language2">	203	1002
      <code class="language*">	102	1010

*I guess what I expect the score to be is something like:

That would give something like (when looking at the support for the code tag):

<pre>
<code 
  class="language-*">

<code 
  class="language-php language-js">

<code 
  class="language-php language-js"
  data-library="highlightjs something">

<code *>

<code class>

<pre>
<code 
  data-config-* 
  data-options-*
  data-highlight-library
  class="language-*">

We have to stop somewhere though, so I don't know how much effort should go into this upgrade path given it's a one-off thing with a low effort to fix problems if they show up (interface is on a single page, with fancy ajax UI, the set of buttons available is limitted).

#43:

1. 👍 I'd been contemplating exactly that! Done.
2. Hm … wildcards at the attribute level and the attribute value level. Right. Will tackle that next.

We have to stop somewhere though […]

I agree. But better to get it right now, while both your head and mine are in this space, rather than having to revisit. Tackling.

This just addresses #43.1. And in doing so, I realized that we've been computing the surplus score far too often all this time 🙈 yay for refactoring!

Just realized that we can't have more than 9 additional thing otherwise it'll break down. maybe an array of integers instead of an integer would be better?

*I guess what I expect the score to be is something like: […]

🤩🤩🤩🤩

The tables with your perspective and the samples were both super helpful! There's only one bug in them, in the very last row, it says 1 attributes without value restriction — but there's actually three of those. So the expected score is actually 1022310 instead of 1022110

Oops, this is the right interdiff for #46 — I just re-uploaded the #45 interdiff in #46 🙈

#45: >9 tags is not a problem (because it's already the biggest number). Only >9 attributes etc matter. I do not think it's going to happen very often that a single plugin explicitly enables that many. And even if that happens, the results will still be sorted by decreasing surplus score. If we're entering that territory, we're seeing some absurdly exotic CKEditor 5 plugins. So … yeah … I'm fine with that letting happen — this issue is a huge leap forward already, especially now that we've implemented your #43 proposal 🤓

yeah that's fair enough, +1

cspell 😬

D'oh, I only implemented #43 in #46, and added \Drupal\Tests\ckeditor5\Unit\SmartDefaultSettingsTest::testSurplusScore() … but forgot to update the expected scores in \Drupal\Tests\ckeditor5\Unit\SmartDefaultSettingsTest::providerComputeCandidates() 🙈🙈🙈🙈

I was a little bit overenthusiastic obviously.

This looks great to me. Spend a lot of time on this so the comment seems enough for me.

Since it's so smart now we can even simplify things so that #3268174: Bug in CKE 4 → 5 upgrade path "format" does not always map to "heading", it could map to "codeBlock" too, or both, or neither is not actually a blocker for #3263384: Add ckeditor5-code-block package and CodeBlock plugin.

As far as review goes, took me some time to get into it, adding a dump/breakpoint in computeCandidates and addToolbarItemsToMatchHtmlElementsInFormat helps (I like the dump because it makes the ajax call fail and we can just retrigger it as necessary without reloading the page).

Right, but #3268174: Bug in CKE 4 → 5 upgrade path "format" does not always map to "heading", it could map to "codeBlock" too, or both, or neither already landed, so it's already not a blocker anyway :P

(I like the dump because it makes the ajax call fail and we can just retrigger it as necessary without reloading the page).

Hah, this is similar to one of my debugging productivity tricks: I just do a exit; before a response is sent, then I can simply re-run an XHR request from the browser console :)

Rerolled after #3268174: Bug in CKE 4 → 5 upgrade path "format" does not always map to "heading", it could map to "codeBlock" too, or both, or neither landed, which conflicted in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest. Both were adding new test cases in the same place. Easy fix.

#55:

1. ✅ Done.
2. ✅ That was indeed an exaggeration.
3. ✅ Done.
4. 🤔 Good question. It does not. It also does not in HEAD. Making that happen is out of scope here.

   🤓 But I also don't think this is an issue in 99.99% of cases. There always is an explicit toolbar item to support a particular tag. ckeditor5_imageCaption, ckeditor5_imageAlign, ckeditor5_drupalMediaCaption and media_mediaAlign are all examples of this kind of dependent plugin, where it not only is automatically enabled based on some conditions but also adds support for additional elements (<img data-caption>, <img data-align>, <drupal-media data-caption>, <drupal-media data-align> respectively). But all four of these only do that if a filter is enabled and a toolbar item is present. All other plugins that are conditionally enabled do not add more elements: ckeditor5_linkImage, ckeditor5_linkMedia and ckeditor5_imageResize depend on other plugins but do not add more elements. ckeditor5_htmlSupport and ckeditor5_pasteFromOffice are enabled unconditionally but don't add more elements either.

   👉 But … how do we guarantee this? Well, the good news is that we can. We can simply add more validation to \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition::validateDrupalAspects() to prevent "risky conditions". Shall I open a follow-up to address that 0.01% case?

5. ✅ HAH! Simple typo, from an interim iteration. Fixed.
6. ✅ Good catch: we converted it from an array to an HTMLRestrictions value object, but didn't update these if-tests. (The other one was if (!empty($unsupported)) {). Fixed both.
7. ✅ Fixed, but interesting that PHP allowed this to work?! 🤯
8. Well … it's simpler than it was in HEAD, where we had two of these beast-like methods 😅 I think what would simplify this a lot is if I added a selectCandidate() helper method. Will do that in the next iteration.

I just realized after posting #58 that I forgot to update the unit test manually, because PHPStorm's refactoring functionality isn't smart enough for that just yet … 🙈

This addresses #55.8 as described in #58.8, and expands the unit test coverage accordingly.

Self-RTBC'ing because:

1. #58/#59 were only trivial changes, literally requested by core committer @lauriii
2. the interdiff for this comment is only moving already RTBC'd logic into a helper method and adding trivial changes to already RTBC'd unit test coverage

+++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
@@ -532,92 +639,9 @@ private function addToolbarItemsToMatchHtmlElementsInFormat(FilterFormatInterfac
-              $selected_plugins[$selected_plugin_id][$attribute_name][$tag_name][$allowed_attribute_value] = $allowed_attribute_value_config;

+++ b/core/modules/ckeditor5/tests/src/Unit/SmartDefaultSettingsTest.php
@@ -142,6 +148,8 @@ public function providerGetCandidates(): \Generator {
+      ['test_foo' => ['-attributes-none-' => ['foo' => NULL]]],

FYI: the order in the "selected plugins" array is weird (as you can see: the "attribute name" reason is in a higher level in the tree structure than the "tag name" reason 🙃). But this already is the case in HEAD.

Changing that is trivial, but I was just trying to minimize change.

I think this contributed to This method is a beast .

1. ✅ Done.
2. ✅ BETTER: I was able to remove most of this, the refactor in #60 made it clear that this can be much simpler now 🥳
3. 🤣
4. ✅ Tried to clarify.
5. ✅ No reason at all, other than to minimize the amount of change: I wanted to show that we are not modifying the logic that generates messages from this array structure; this array structure already exists in HEAD. But I think at this point it'd be clearer to just rename $plugins_to_enable_to_support_attribute_config to $selected_plugins. So … did that.

   (The confusing remaining part is described in #61. We could change that here too if you prefer. I don't feel strongly either way. #3245967: Messages upon switching to CKEditor 5 are overwhelming will probably have to change things in this area anyway.)