Replace CKEditor 4 with CKEditor 5 in the Standard profile and StandardTest

...which is weird because this config was generated by CKEditor 5 based on existing CKEditor 4 config.

This seems impossible, because settings.toolbar.rows does not exist in CKEditor 5.

Investigated … aha, looks like this is why:

    // Add "insert media from library" button to CKEditor 4 configuration, the
    // pre-existing toolbar item group labeled "Media".
    $settings['toolbar']['rows'][0][3]['items'][] = 'DrupalMediaLibrary';
    $basic_html_editor_with_media_embed->setSettings($settings);
    $basic_html_editor_with_media_embed->save();

(in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::setUp())

👆 This is modifying an existing Editor config entity and saving it as a new one, purely for upgrade path testing purposes. But CKEditor 4 is not available anymore, and hence nor is its schema.

Updating to $basic_html_editor_with_media_embed->trustData()->save(); is also insufficient, because core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php does not care about that. You'll need to disable config schema checking during ::setUp() and re-enable it afterwards.

But this issue does not include any new tests; those fails are existing test coverage. I'm assuming it's making wrong assumptions about the config?

Yes and no.

1. The current tests are correct.
2. But the current tests also assume that they can create new text editor config entities (to simulate different upgrade path scenarios!) without violating config schema.
3. Since this MR is removing the config schema for CKEditor 4 … that assumption no longer is true.
4. Therefore it's this MR's responsibility to remove that assumption … because it's this MR that's removing the CKEditor 4 module. (Of course, you can create a separate issue for it too, but then we have no way to verify that it actually works — only the MR that removes the module can test that.)

Now that CKE5 is almost stable, time to revisit this! 🥳

#4:

1. Agreed that code snippets are developer-centric. Most sites don't need it. Also, since #3263384: Add ckeditor5-code-block package and CodeBlock plugin, it's become easy to add it! (Note that that adds support for the <pre> tag too — that's required for blocks of code instead of just inline code.)
2. ✅ This was fixed recently in #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path.
3. There have been many fixes to the "GHS" functionality since then. There is one known remaining stable blocker: #3268306: [GHS] Custom/unofficial HTML tags not retained: <drupal-media>, <drupal-entity>, <foobar> — but that too has been fixed upstream already and is coming in the next CKEditor 5 release! 🥳

We should first do #3270831: Make the CKEditor 4 → 5 upgrade path work even when the CKEditor 4 module is removed. Then we should update this MR. The pain points of #4 are a thing of the past.

Isn't this still blocked on #3268306: [GHS] Custom/unofficial HTML tags not retained: <drupal-media>, <drupal-entity>, <foobar> and #3222756: Allow using images from external source? And more generally speaking, isn't this blocked on the CKEditor 5 module itself being marked stable?

OTOH, it sure would be good to have this merge request/patch green before marking CKEditor 5 stable, maybe that's what you're looking for? 😊

This is still blocked on #3222756: Allow using images from external source but the steps to get this green shouldn't be impacted by that issue.

Per @catch's comment at #3304326-10: Deprecate CKEditor 4 module in 9.5, I think we can consider this unblocked! 👍

In fact, this is now a hard blocker for that issue.

Review

1. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -154,7 +154,10 @@ public function write($name, array $data) {
   -      throw new StorageException('Failed to write configuration file: ' . $target);
   +      throw new StorageException('Failed to write configuration file: ' . $this->getFilePath($name));
   +    }
   +    else {
   +      $this->getFileSystem()->chmod($target);
   

   🤯 Is this change really necessary? This seems unrelated?

2. @@ -629,8 +629,7 @@ media_media:
        class: Drupal\ckeditor5\Plugin\CKEditor5Plugin\Media
        elements:
          - <drupal-media>
   -      - <drupal-media data-entity-type data-entity-uuid alt>
   -      - <drupal-media data-view-mode>
   +      - <drupal-media data-entity-type data-entity-uuid alt data-view-mode>
        conditions:
          filter: media_embed
   <snip>
   -    return $subset;
   +    // @todo Simplify in https://www.drupal.org/project/drupal/issues/3278636, that will allow removing all uses of HTMLRestrictions in this class.
   +    return array_merge(['<drupal-media>'], $subset->toCKEditor5ElementsArray());
   

   These changes are also unrelated; they're de facto reverting #3278636: HTMLRestrictions::fromString() bug: multiple occurrences of same tag results in only last one being respected, which landed yesterday.

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -465,7 +465,6 @@ public function testFullHtml() {
   -
   

   🤓 Unnecessary change.

4. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -507,7 +506,7 @@ public function testFullHtml() {
   -    $assert_session->responseContains('<p><a foo="bar" hreflang="en" href="https://example.com"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a></p>');
   +    $this->assertSame('<a href="https://example.com" foo="bar" hreflang="en"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a>', $page->find('css', 'article p')->getHtml());
   

   🤔 Why this change?

5. +++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
   @@ -84,22 +84,22 @@ protected function setUp(): void {
   -      Yaml::parseFile('core/profiles/standard/config/install/filter.format.full_html.yml')
   +      Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/filter.format.full_html.yml')
        )
          ->setSyncing(TRUE)
          ->save();
        Editor::create(
   -      Yaml::parseFile('core/profiles/standard/config/install/editor.editor.full_html.yml')
   +      Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/editor.editor.full_html.yml')
        )->save();
    
   -    $basic_html_format = Yaml::parseFile('core/profiles/standard/config/install/filter.format.basic_html.yml');
   +    $basic_html_format = Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/filter.format.basic_html.yml');
        FilterFormat::create($basic_html_format)->save();
        Editor::create(
   -      Yaml::parseFile('core/profiles/standard/config/install/editor.editor.basic_html.yml')
   +      Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/editor.editor.basic_html.yml')
        )->save();
    
        FilterFormat::create(
   -      Yaml::parseFile('core/profiles/standard/config/install/filter.format.restricted_html.yml')
   +      Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/filter.format.restricted_html.yml')
        )->save();
    
        $allowed_html_parents = ['filters', 'filter_html', 'settings', 'allowed_html'];
   @@ -113,7 +113,7 @@ protected function setUp(): void {
   
   @@ -113,7 +113,7 @@ protected function setUp(): void {
        Editor::create(
          ['format' => 'basic_html_without_h4_h6']
          +
   -      Yaml::parseFile('core/profiles/standard/config/install/editor.editor.basic_html.yml')
   +      Yaml::parseFile('core/modules/ckeditor5/tests/fixtures/ckeditor4_config/editor.editor.basic_html.yml')
   

   🤩👏

6. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -266,29 +266,12 @@ public function getHTMLRestrictions() {
   -      // All attributes are already allowed on this tag, this is the most
   -      // permissive configuration, no additional processing is required.
   -      if (isset($restrictions['allowed'][$tag]) && $restrictions['allowed'][$tag] === TRUE) {
   -        continue;
   -      }
   

   😊 Also accidental.

7. +++ b/core/modules/help/tests/src/Functional/HelpTest.php
   @@ -149,6 +149,12 @@ protected function verifyHelp($response = 200) {
   +        // The help for CKEditor 5 intentionally has escaped '<' so leave this
   +        // iteration before the assertion below.
   +        if ($module === 'ckeditor5') {
   +          continue;
   +        }
            // Ensure there are no escaped '<' characters.
            $this->assertSession()->assertNoEscaped('<');
   

   👍

8. +++ b/core/profiles/standard/config/install/editor.editor.basic_html.yml
   @@ -4,42 +4,56 @@ dependencies:
   +      reversed: true
   

   🤔 AFAICT this should be false? That's also what the test expectation for \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest for Standard's unchanged basic_html is! See the top of \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::provider().

9. +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
   @@ -6,8 +6,6 @@ dependencies:
   -roles:
   -  - authenticated
   
   +++ b/core/profiles/standard/config/install/filter.format.full_html.yml
   @@ -6,8 +6,6 @@ dependencies:
   -roles:
   -  - administrator
   

   🐛 We cannot change these — I know that it's really weird. See \Drupal\filter\Entity\FilterFormat::$roles and \Drupal\filter\Entity\FilterFormat::toArray() 🤷‍♀️ See #3167198: Deprecate the 'roles' property of text formats and remove it from the UI — it's a known problem.

10. +++ b/core/profiles/standard/config/install/filter.format.full_html.yml
    @@ -33,3 +31,12 @@ filters:
    +  filter_html:
    +    id: filter_html
    +    provider: filter
    +    status: false
    +    weight: -10
    +    settings:
    +      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <s> <sup> <sub> <img src alt data-entity-type data-entity-uuid data-align data-caption> <table> <caption> <tbody> <thead> <tfoot> <th> <td> <tr> <hr> <p> <h1> <pre>'
    +      filter_html_help: true
    +      filter_html_nofollow: false
    

    🐛 I know that this happens automatically by just saving this config entity again (either through the API or the UI). But note that it says status: false, so it's safe to omit. Yes, this too is weird. But adding this is more confusing than it's worth.

    This too is a known problem: #2385047: Remove disabled filters from text format configuration (reviewed that issue thanks to being made aware of it once again by this!).

Addresses #16
Re #16.4
The tests were failing despite being identical elements because of differences in whitespace. getHtml() makes that predictable.

From 28 KB to 23! 👍

--- a/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
+++ b/core/modules/ckeditor5/tests/src/Unit/HTMLRestrictionsTest.php
…
--- a/core/modules/ckeditor5/tests/src/Unit/SourceEditingPluginTest.php
+++ b/core/modules/ckeditor5/tests/src/Unit/SourceEditingPluginTest.php
…

Still seeing unrelated changes in these files? 🤔 I think all of these changes can be dropped?

Apparently 80% of the 9.4 MR I turned into a patch wasn't needed. I think this prunes everything?

28 → 23 → 18 KB! 😄

1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -507,7 +507,7 @@ public function testFullHtml() {
   -    $assert_session->responseContains('<p><a foo="bar" hreflang="en" href="https://example.com"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a></p>');
   +    $this->assertSame('<a href="https://example.com" foo="bar" hreflang="en"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a>', $page->find('css', 'article p')->getHtml());
   

   ✅ Explained at the bottom of #17.

2. +++ b/core/profiles/standard/config/install/editor.editor.basic_html.yml
   @@ -4,42 +4,56 @@ dependencies:
   +      - '|'
   +      - code
   

   👍 This is the way that the automatic upgrade path placed it. And that's fine — it's the automatic upgrade path.

   🤓🤔 But I'm wondering if we want to place the code button in a different place, so that it's not quite on its own? Perhaps in the same group as heading? Wouldn't that be better for usability?

3. +++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
   @@ -15,7 +15,7 @@ filters:
   -      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption>'
   +      allowed_html: '<br> <p> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <cite> <dl> <dt> <dd> <a hreflang href> <blockquote cite> <ul type> <ol start type reversed> <strong> <em> <code> <li> <img src alt data-entity-uuid data-entity-type height width data-caption data-align>'
   

   🐛 Let's remove <ol reversed> — that's a net addition we don't want.

Doing some manual testing prior to RTBC'ing 🤓

Manual test

👍 CKEditor 5 works fine for Basic HTML & Full HTML!

(Added before & after screenshots for both to the issue summary.)

Re-saving both Basic HTML & Full HTML in the config UI should result in empty config diffs

(Other than A) uuid and _core.default_config_hash being added, B) roles not being exported in filter_format config — see #3167198: Deprecate the 'roles' property of text formats and remove it from the UI for that, C) even disabled filters being exported in filter_format config — see #2385047: Remove disabled filters from text format configuration for that.)

Diff analysis:

+++ b/core/profiles/standard/config/install/editor.editor.full_html.yml
@@ -41,6 +44,9 @@ settings:
+    ckeditor5_list:
+      reversed: true
+      startIndex: true

This is the only problematic difference — that means this is missing from the patch!

Conclusion

Per @catch's comment at #3304326-10: Deprecate CKEditor 4 module in 9.5, we can consider this not blocked on #3238333: Roadmap to CKEditor 5 stable in Drupal 9, so … RTBC! 🥳

I was going to add the bit pointed out in the diff analysis … but apparently in the mean time there is an unexpected test failure, specifically on the line that was changed:

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
@@ -507,7 +507,7 @@ public function testFullHtml() {
-    $assert_session->responseContains('<p><a foo="bar" hreflang="en" href="https://example.com"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a></p>');
+    $this->assertSame('<a href="https://example.com" foo="bar" hreflang="en"><abbr title="National Aeronautics and Space Administration">NASA</abbr> is an acronym.</a>', $page->find('css', 'article p')->getHtml());

Can we try reverting this?

The changes made to CKEditor5AllowedTags test do not appear necessary anymore. I suspect the mix of old code in earlier patches made that change necessary for tests to pass.

Wim "Eagle Eye" Leers has already reviewed this one up and down, with his legendary ability to spot defects, and would have RTBCed this but for the test failure. I've confirmed his suggestion in #26 has been implemented. Tests are passing. What else we got? Probably nothing.

I therefore humbly bestow RTBC.

#28: I suspect the same! If not, we'd have a random failure scenario on our hands, which thankfully is not the case! 😄🥳

#29: Well this 🦅👁 is mostly just using git diff to spot these things and … the one thing that I said in #26 is missing from the patch is still missing 🙈

I see that \Drupal\Tests\standard\Functional\StandardTest is testing config schema conformance. Since Text Editors using CKEditor 5 are the first config entities in Drupal core that can be fully validated … I'm extending the spirit of that test to also do config entity validation where possible (so for now only Text Editor config entities using CKEditor 5).

Adding such test coverage will actually show that what I said in #29 is a problem: it shows that the exported config in the Standard install profile fails validation.

Test coverage — expected output:

There was 1 failure:

1) Drupal\Tests\standard\Functional\StandardTest::testStandard
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'Configuration for the enabled plugin "<em class="placeholder">List</em>" (<em class="placeholder">ckeditor5_list</em>) is missing.'
+)

Adding what I suggested in #26 should fix the failure! 🤞

Ah, okay. I misunderstood #26 and thought you were only asking for that one line in the test to be reverted. I don't have the larger context for this issue.

That said, the improved test coverage makes a lot of sense to me, now that I know more about this fancy (but exciting!) config validation stuff you're doing. We've got a fail patch proving that the test actually tests it, and the latest patch is corrected. Restoring RTBC.

Just realized something!

+++ b/core/profiles/standard/config/install/editor.editor.basic_html.yml
@@ -4,42 +4,55 @@ dependencies:
+      - uploadImage

+++ b/core/profiles/standard/config/install/editor.editor.full_html.yml
@@ -4,50 +4,48 @@ dependencies:
+      - uploadImage

These are changing in #3222756: Allow using images from external source … so we probably should hold off committing this until that lands.

Alternatively, we can commit this today, and then we'll need to update the default configuration in core/profiles/standard again in that issue.

Either way, the great news is that thanks to the validation added in #31, that both commit orders are safe: we will always be certain that the default formats + editors in Standard (that use CKEditor 5) are 100% valid 🤓😄

So actually … I think that's a reason to just go ahead here 🤓

+++ b/core/profiles/standard/config/install/filter.format.basic_html.yml
@@ -15,7 +15,7 @@ filters:
-      allowed_html: '<a href hreflang> <em> <strong> <cite> <blockquote cite> <code> <ul type> <ol start type> <li> <dl> <dt> <dd> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <p> <br> <span> <img src alt height width data-entity-type data-entity-uuid data-align data-caption>'
+      allowed_html: '<br> <p> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <cite> <dl> <dt> <dd> <a hreflang href> <blockquote cite> <ul type> <ol start type> <strong> <em> <code> <li> <img src alt data-entity-uuid data-entity-type height width data-caption data-align>'

Should we add <span> back to the list given that it should be now possible? IMO having span on it's own here does not make a lot of sense but that would make the CKEditor 5 configuration 100% equivalent to the CKEditor 4 configuration.

GREAT catch! I totally missed that in my reviews! 🙈 That is possible since #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path. I have no idea why it's not like that in this patch 😅

now <span>s in a place where it works

now press save
think about upgrading wonder why you haven't before

just <span>

🤦‍♂️🤦‍♂️
'The current CKEditor 5 build requires the following elements and attributes: <br><code><br> <p> <h2 id> <h3 id> <h4 id> <h5 id> <h6 id> <* dir="ltr rtl" lang> <cite> <dl> <dt> <dd> <a hreflang href> <blockquote cite> <ul type> <ol start type> <strong> <em> <code> <li> <img src alt data-entity-uuid data-entity-type height width data-caption data-align>
The following elements are not supported:
<span>'

Is #33 fine to work with or do we set things up to accommodate span?

I think you’ll have to also configure source editing to enable support for <span>.

Indeed. The validation added in #31 is doing its job! :)

+++ b/core/profiles/standard/config/install/editor.editor.basic_html.yml
@@ -4,42 +4,55 @@ dependencies:
+    ckeditor5_sourceEditing:
+      allowed_tags:
+        - '<cite>'
+        - '<dl>'
+        - '<dt>'
+        - '<dd>'
+        - '<a hreflang>'
+        - '<blockquote cite>'
+        - '<ul type>'
+        - '<ol start type>'
+        - '<h2 id>'
+        - '<h3 id>'
+        - '<h4 id>'
+        - '<h5 id>'
+        - '<h6 id>'

IOW: this is missing <span>.

(This actually works correctly when the upgrade path is used, the first test case in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::provider() proves this. I bet that what happened is that a manual config change was made — that's the only way for the Text Editor & Text Format to get out of sync. That makes sense when trying to quickly address feedback 😊)

Third time's a charm! 🚀

Committed 9ca6256 and pushed to 10.1.x. Also cherry-picked to 10.0.x and 9.5.x. Thanks!

This unblocked #3304326: Deprecate CKEditor 4 module in 9.5! 🥳