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 ✨ [upstream] Use CKEditor 5's native and UX Needs work .

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 → .

Running the test-only feature

But @wim leers left 1 question and 1 nitpicky.

LEaving in review.

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 ✨ [upstream] Use CKEditor 5's native and UX Needs work 😅

I was actually confused for a second haha.

Test-only feature failed as expected.

Post_update ran without issue on a standard profile.

So think this is good.

📌 [upstream] Update CKEditor 5 to 40.0.0 Needs review landed, this needs a rebase & conflict resolution.

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

There is a failure in the tests, but it looks like that is unrelated and I'm not sure how to rerun tests with gitlab, this looks rtbc imho

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

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

Extremely flakey!

Also agree this looks good!

I posted couple comments in the MR

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...

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 👏

Threads appear to be answered and pipeline showing green. No randoms this time!

Left a comment on the MR regarding the post update.

Reposting my question from Slack: Does the upgrade path work from both Drupal 9 and CKE4 as well as from CKE5/Drupal 10 content getting updated a second time? Moving the fix to presave would also help with that aspect, but it still needs its own consideration since the source data formats from the two versions would be different.

#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.

This change involves an upgrade path but the update is small, self-contained, has no BC concerns that I can see, and fixes a major bug, therefore I think the impact outweighs the disruption and therefore this can be committed during the beta phase of 10.2.0. It's also nice that we found and fixed extra config validation violations here!

Committed and pushed bcf5a9159e to 11.x and ec24a58c6d to 10.2.x. Thanks!

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

Yes, this is correct :) Anything that could be in the database but also potentially in config shipped with a module.

Automatically closed - issue fixed for 2 weeks with no activity.

joshuami → changed the visibility of the branch 3396628-tighten-sourceeditingredundanttagsconstraintvalidator to hidden.