Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by

Tightening this validator should make \Drupal\Tests\standard\Functional\StandardTest fail.

Which in turn should require us to write the missing update path hook.

This blocks #3274635: [upstream] Use CKEditor 5's native <ol type> and <ul type> UX.

Turns out this is caused by:

        // If the entirety (so not just the tag but also the attributes, and not
        // just some of the attribute values, but all of them) of the HTML
        // elements being configured to be edited via the Source Editing plugin
        // is supported by a CKEditor 5 plugin, complain. But if some attribute
        // or some attribute value is still not yet supported, do not generate a
        // violation message.
        // If there is overlap, but some attribute/attribute value is still not
        // supported, exit this iteration without generating a violation
        // message. Essentially: when assessing a particular value
        // (for example `<foo bar baz>`), only CKEditor 5 plugins providing an
        // exact match (`<foo bar baz>`) or a superset (`<foo bar baz qux>`) can
        // trigger a violation, not subsets (`<foo bar>`).
        if ($is_attr_overlap && !$source_enabled_element->diff($overlap)->allowsNothing()) {
          continue;
        }

This was introduced by #3260857: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values.

It was done intentionally (see #3260857-42: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values), but … I think the example here in Drupal core for <ol start type> and <ol start> actually being natively supported is a pretty convincing reason to actually tighten this: it should complain about <ol start>.

If we'd done that, then #3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) could not have forgotten to write the appropriate update path!

Just realized something: \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::computeCKEditor5PluginSubsetConfiguration()'s case 'ckeditor5_list': should also be updated.

Oops, #9 was meant to be posted on #3274635: [upstream] Use CKEditor 5's native <ol type> and <ul type> UX 😅

#3393557: [upstream] Update CKEditor 5 to 40.0.0 landed, this needs a rebase & conflict resolution.

Hoping @smustgrave can review this one more time — found a pretty big bug during rebasing 😅

Retesting — Nightwatch & Functional JS have been a bit flakey this week…

Thanks to @lauriii, a new edge case has been found: because SourceEditingRedundantTagsConstraintValidator was originally designed to handle tags, this code made sense:

    // An array of tags enabled by every plugin other than Source Editing.
    $enabled_plugin_elements = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($other_enabled_plugins), $text_editor, FALSE));
    $disabled_plugin_elements = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($enableable_disabled_plugins), $text_editor, FALSE));
    $enabled_plugin_plain_tags = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($other_enabled_plugins), $text_editor, FALSE, TRUE));
    $disabled_plugin_plain_tags = new HTMLRestrictions($this->pluginManager->getProvidedElements(array_keys($enableable_disabled_plugins), $text_editor, FALSE, TRUE));

However … in the case of plugins with optional additional attributes, the above will result for

      'settings' => [
        'toolbar' => [
          'items' => [
            'numberedList',
            'sourceEditing',
          ],
        ],
        'plugins' => [
          'ckeditor5_list' => [
            'properties' => [
              'reversed' => FALSE,
              'startIndex' => FALSE,
            ],
            'multiBlock' => TRUE,
          ],
          'ckeditor5_sourceEditing' => [
            'allowed_tags' => [
              '<ol start type>',
            ],
          ],
        ],

in concluding that for the enabled ckeditor5_list plugin, the following things can be generated: <ul> <ol> <li> … because of

            'properties' => [
              'reversed' => FALSE,
              'startIndex' => FALSE,
            ],

IOW: what's missing here is what additional attributes could be generated by enabling optional settings! 🐣

Now Umami tests fail 👍 https://git.drupalcode.org/issue/drupal-3396628/-/pipelines/47417/test_r...

And it's not just <ol start type> being made editable through "Source Editing" that triggered a violation:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for editor.editor.basic_html with the following errors: 0 [settings.plugins.ckeditor5_sourceEditing.allowed_tags.7] The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: <em class="placeholder">List (&lt;ol start&gt;)</em>., 1 [settings.plugins.ckeditor5_sourceEditing.allowed_tags.14] The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: <em class="placeholder">Media (&lt;drupal-media data-view-mode&gt;)</em>.

👍

Fixing Umami's editor config nicely triggered a validation error in the corresponding filter_format config:

Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testConfig
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'The CKEditor 5 "<em class="placeholder">Media</em>" plugin's "<em class="placeholder">Allow the user to override the default view mode</em>" setting should be in sync with the "<em class="placeholder">Embed media</em>" filter's "<em class="placeholder">View modes selectable in the &quot;Edit media&quot; dialog</em>" setting: when checked, two or more view modes must be allowed by the filter.'
+)

👍

Fixed that too now — ready for review again, and WHAT A GREAT FIND from @lauriii 👏

#23: did that. 👍

#24:

Does the upgrade path work from both Drupal 9 and CKE4 as well as from CKE5/Drupal 10 content getting updated a second time?

Yes.

Drupal 9 and CKEditor 4

This part was covered by the original #3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) issue already. See git show e935c438 -- core/modules/ckeditor5/src/Plugin/CKEditor4To5Upgrade/Core.php for the updated 4 → 5 upgrade path, and git show e935c438 -- core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php for the explicit test coverage. Note the

-              '<ol start type>',
+              '<ol type>',

, meaning that from that point onwards, it wouldn't configure ckeditor5_sourceEditing anymore to enable the user to set <ol start>.

This means that any site upgrading from CKEditor 4 to CKEditor 5 using a release that includes that commit does not need this update path, i.e. it will be a no-op.

Test coverage proving the no-op update path: CKEditor5UpdateOlStartReversed + core/modules/ckeditor5/tests/fixtures/update/editor.editor.test_format_list_ol_start_post_3261599.yml (added just now to explicitly test this case too 👍)

For my personal knowledge is there a good rule of thumb about when post_update should be used vs save?

I've been struggling with this too, even after @catch has pointed this out … 3 times in 2 months? 🙈

I think the rule is: for all config entities, always 🤔