Remove CKEditor 4 from core

The big_pipe failure was for a regression test added for #2698811: BigPipe unnecessarily renders and sends multiple-occurrence placeholders multiple times when using JS — can cause JS errors.

I think it'd be reasonable to omit a regression test that involved a specific module that no longer exists. But still, happy to try to update it to use CKEditor 5 instead nonetheless.

Tried that in aa368f25dd598ba0a34177451fc3c9ef7bf94a45.

FATAL Drupal\Tests\media_library\FunctionalJavascript\CKEditorIntegrationTest: test runner returned a non-zero error code (255).

+

1) Drupal\Tests\Core\Test\PhpUnitCliTest::testPhpUnitListTests
COMMAND: vendor/bin/phpunit --configuration core --verbose --list-tests
OUTPUT:
ERROR: PHP Fatal error:  Trait "Drupal\Tests\ckeditor\Traits\CKEditorTestTrait" not found in /var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php on line 20

This is 100% certainly because the Media Library CKEditor integration is not being removed. Easy fix 🤓

See c048195bdb2ee3df37864dc88744fb4d289d96bd.

Note: equivalent tests exist in the CKEditor 5 module: \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest.

FATAL Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest: test runner returned a non-zero error code (255).

Similar to #6: the Media CKEditor integration still needs to be removed.

This one is trickier, because many core themes have overrides for the media embed preview error. So did that part in a separate commit.

See f174785157f362596d31b071dad2bc1e99fa69a3 + 1150087da3e1519b902922d48108cb95406339b1.

Note: equivalent tests exist in the CKEditor 5 module: \Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest.

The rest and jsonapi module failures are trivial: the CKEditor module was only used to interact with sample Editor config entities via REST/JSON:API for testing purposes. Easily changed. See b75618bbba904b2e3f3d2ed689bcf66736ed9df3.

There are lots of test failures in the CKEditor 5 module too. They fall in three buckets:

1. ✅ Even though I thought we'd only remove CKEditor4to5UpgradeCompletenessTest in Drupal 11, in #3239012: [11.x] Remove the CKEditor 4 upgrade path, I now realize that does not make sense, because the CKEditor 4 module's code is going to be removed in Drupal 10 and hence we also can no longer use that module's metadata to verify the completeness of the upgrade path.

   For the same reason \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5CKEditor4Compatibility no longer is needed: if CKEditor 4 does not exist anymore, there is no need to verify that both can co-exist on the same page either.

   → commit a0decf0f7e3ddfbf45866ccd1919c8a7c2c1c52f

2. ✅ Similarly, those parts of \Drupal\ckeditor5\SmartDefaultSettings that inspect a CKEditor 4 instance no longer are needed anymore either.

   → commit 54ff3b14e56005384771a3afc61798fd64d8c152

3. 🏁 This previous step has made all the infrastructure documented to be removed in #3239012: [11.x] Remove the CKEditor 4 upgrade path into actually 100% certain dead code. Not doing that here though, because A) we'll have to chase HEAD all the time, B) it's out of scope: the scope here is too see what breaks. Dead code != breakage.
4. 🏁 To fix the remaining test failures, we also need to update \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest + CKEditor5AllowedTagsTest + \Drupal\Tests\ckeditor5\FunctionalJavascript\AdminUiTest to no longer use CKEditor 4, but instead use \Drupal\editor_test\Plugin\Editor\UnicornEditor or \Drupal\editor_test\Plugin\Editor\TRexEditor. This part remains to be done.

inline_form_errors' \Drupal\Tests\inline_form_errors\FunctionalJavascript\FormErrorHandlerCKEditorTest can be deleted — the CKEditor 5 successor is right next to it: \Drupal\Tests\inline_form_errors\FunctionalJavascript\FormErrorHandlerCKEditor5Test 👍

Commit: 3c6b525254c950f674df76b4c50acb78987e2f82.

EditorDialogAccessTest + EditorPrivateFileReferenceFilterTest + EditorAdminTest all should be updated to no longer use CKEditor 4, but instead use \Drupal\editor_test\Plugin\Editor\UnicornEditor or \Drupal\editor_test\Plugin\Editor\TRexEditor.

This is similar to what I wrote in #9.4.

Issue created: #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests.

While awaiting DrupalCI, I bet there are now 25 remaining failures.

How do these break down?

• 12: #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests will fix the 6 test failures in editor module and the 6 test failures in ckeditor5 module.
• 6: Quick Edit, which is being removed before Drupal 10 anyway.
• 7: all default config-related test failures (1 in "slow" and 1 in "Config") as well as all update path-related test failures (2 in aggregator, 2 in migrate_drupal_ui, 1 in update) are occurring because this MR is currently deleting the Standard install profile editor.editor.* default config. There was no upgrade path from Drupal 7 to the Drupal 8|9 CKEditor 4 text editor configuration, so simply creating new default editor.editor.* (as described in #3) should fix these.

In other words, we'll get down to 19 without extra effort (i.e. just deleting Quick Edit like is already happening), to 12 by adding two simple YAML files to this MR and to zero by doing #3270734, which we can already start doing.

So I propose we first do #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests — which needs to happen anyway.

#13: Yep, but I wanted to prove that the long list of ~50 failures all over the place was not actually scary. That's also why I have scoped commits per integration module: to make it easy to transfer what I did here to other issues. 🤞

The prediction I had in #12 became a reality — although it's currently sitting at 27 failures instead of 25. ConfirmClassyCopiesTest has 2 failures, but that's a trivial thing to update/address.

I'm stopping the work on this, I feel like I've turned enough of the unknowns into knowns to feel confident that this will not be painful to address 😊

We discovered that there aren't any templates or libraries shared between CKEditor 4 and CKEditor 5. Because of that, I think we should simply remove all of the references to CKEditor 4 from Claro, Olivero and Umami as part of this issue. We can leave the references in Seven, Classy and Stable since they are going to be removed from core before Drupal 10.

Repurposing this issue to actually remove CKEditor 4 — stay tuned for the issue summary update to follow the common "module removal" issue template.

In any case this is blocked on #3304326: Deprecate CKEditor 4 module in 9.5.

This is now the CKE4 equivalent of #3227033: Remove Quick Edit from core for Quick Edit.

The newly created #3306563: [Meta] Tasks to remove CKEditor (4) from core and move to contrib is the parent issue now and is the CKE4 equivalent of #3274155: [Meta] Tasks to remove Quick Edit from core and move to contrib for Quick Edit.

It should be possible for me to create an MR that is mostly green, so that we're ready to do this 😊

4 failures!

Not bad for the first try! 😄

Breakdown:

1. EditorPrivateFileReferenceFilterTest → is already being fixed in #3306715: Replace ckeditor with editor_test in editor_private_test.info.yml, nothing to do here.
2. UpdatePathTestBaseFilledTest, UpdatePathTestBaseTest and CKEditor5UpdateImageToolbarItemTest are all update path tests, and they all fail in the same way: The link Continue was not found on the page. I'll figure that out tomorrow 👍

How I created this MR

In case this MR needs to be recreated again, I tried to semi-automate it most of the way:

Straight removals — module:

git rm -rf core/modules/ckeditor
git ci -nam '`git rm -rf core/modules/ckeditor`'

git rm -rf core/assets/vendor/ckeditor
git ci -nam '`git rm -rf core/assets/vendor/ckeditor`'

git rm -f core/modules/ckeditor5/tests/src/Kernel/CKEditor4to5UpgradeCompletenessTest.php
git ci -nm 'Remove `\Drupal\Tests\ckeditor5\Kernel\CKEditor4to5UpgradeCompletenessTest`.'

Straight removals — theme overrides:

git rm -f core/themes/stable9/css/media/plugins/drupalmedia/ckeditor.drupalmedia.css
git rm -rf core/themes/stable9/css/ckeditor
git ci -nm 'Remove `stable9` CKEditor 4 overrides.'

git rm -rf core/themes/stable/css/ckeditor
git rm -f core/themes/stable/templates/admin/ckeditor-settings-toolbar.html.twig
git ci -nm 'Remove `stable` CKEditor 4 overrides.'

git rm -f core/themes/claro/css/theme/ckeditor*
git ci -nm 'Remove `claro` CKEditor 4 overrides.'

git rm -f core/themes/olivero/css/theme/ckeditor-frame.*
git ci -nm 'Remove `olivero` CKEditor 4 overrides.'

Partial updates:

git apply -3v partial-updates-c5e50e3fe2b0d69a507965962fe3eecda4713008-do-not-test.patch
git ci -nam 'Update `*.info.yml`, `*.libraries.yml` and a few `*.css` files.'

git apply -3v tooling-2022-08-31-do-not-test.patch
git ci -nam 'Update tooling.'

(c5e50e3fe2b0d69a507965962fe3eecda4713008 was 10.0.x HEAD at the time!)

Conclusion

I should be able to get it down to 1 failure tomorrow, and #3306715: Replace ckeditor with editor_test in editor_private_test.info.yml will get it down to zero.

(Also: #3285054: Add ckeditor5-stylesheets: false to Claro and Olivero (and fix it 😅) landed after the MR was created/while tests were running, so I'll need to update the MR anyway.)

The one editor_private_test failure

That means this issue is not only postponed on #3304326: Deprecate CKEditor 4 module in 9.5, but also on #3306715: Replace ckeditor with editor_test in editor_private_test.info.yml. ⇒ bumping to [PP-2]

(No code changes will be necessary here; all we need to do is wait.)

The 3 update path test failures

AFAICT the reason for the failures in all update path tests is simply that the 9.4.4 dumps have the CKEditor 4 module installed but it's suddenly disappeared.

So we need to add a stub ckeditor.info.yml file. This is what @longwave mentioned over at #3304736-18: Provide a good UX to ensure the CKEditor 4 to 5 update is always done before upgrading to Drupal 10 unless the contrib module is installed.

But per @catch's guidance at #3304736-19: Provide a good UX to ensure the CKEditor 4 to 5 update is always done before upgrading to Drupal 10 unless the contrib module is installed and subsequent comments:

[…] why I don't think we should provide a stub - it could lead to silent error conditions […]

and

9.5.x:
Install ckeditor5, update your settings, uninstall ckeditor 4 easily because it's still in core.

10.0.x:
Install ckeditor 4 from contrib, update your settings, uninstall ckeditor 4 easily because you already installed it from contrib.

10.0.x but didn't read the instructions:
Update to 10.0.x, missing module warning on update.php, abort the update, and do the 9.5.x steps, or install the contrib module and follow the 10.0.x steps.

👆 In other words, this MR is functionally ready and "done". The part that is still missing is that complex (and hitherto unseen as far as I know?!) dance of providing a good UX for making the above happen — @xjm summarized and retitled that other issue well over at #3304736-26: Provide a good UX to ensure the CKEditor 4 to 5 update is always done before upgrading to Drupal 10 unless the contrib module is installed.

⇒ bumping to [PP-3] to indicate this is also blocked on that issue. We will likely need code changes here after that, so this is the one hard blocker to do any further work here.

P.S.: Please credit @longwave, @catch and @xjm for their work on #3304736, that's made this much easier to diagnose! 🙏

Rebased the MR for #3285054: Add ckeditor5-stylesheets: false to Claro and Olivero (and fix it 😅). That issue conflicted with the partial-updates-* diff from #24. Updated. Other steps remain unchanged.

Nothing left to be done here.

This cannot be committed until https://www.drupal.org/project/ckeditor/releases/1.0.0 is released, which in turn is currently blocked on the Drupal Security Team approving #3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project.

Note: Because #3306938: Restore olivero_post_update_add_olivero_primary_color() was fixed, there are now 5 failing tests, not 4. OliveroPostUpdateTest means that The 3 update path test failures should now really be The 4 update path test failures.

#3306715: Replace ckeditor with editor_test in editor_private_test.info.yml landed.

AFAICT this is now only blocked on #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x, not #3304736: Provide a good UX to ensure the CKEditor 4 to 5 update is always done before upgrading to Drupal 10 unless the contrib module is installed anymore. Updating title to indicate 1, not 2 blockers. 👍

#3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x is in. Re-testing.

I'm not sure what to do (if anything needs to be done in the first place) with these ckeditor-references that are still floating around:

- https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit...
- https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit...
- https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit...

Excellent catches, @Spokje! Especially those "for example"-style comments. And TIL yarn spellcheck:make-drupal-dict, I had no idea we had that 😄

Looks like this is … indeed still ready for review, hopefully the next person won't find anything 🤞

Excellent catches, @Spokje! Especially those "for example"-style comments.

Just to be clear (blatantly playing the non-native speaker card): The linked "stragglers" in #31 are still present in the MR. I haven't touched those, since I'm completely unsure if they are eligible for deletion or a rewrite or a "whatever".

Aha! I thought your Removed a few stragglers commit handled those 😅 So apparently #31 lists the remaining stragglers after that commit. 👍

• https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit... is arguably/theoretically a BC break. It's harmless to have that metadata in there. I think we should keep that one, but remove the reference to CKEditor (4).
• https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit... should now point to the CKEditor 5 example :)
• https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/edit... → all of these can be updated trivially (s/ckeditor5/ckeditor5 and s/CKEditor/CKEditor 5/)

The key remaining thing here (after @Spokje addresses The Three Stragglers) is the manual testing. That requires https://www.drupal.org/project/ckeditor/releases/1.0.x-dev to get an initial release (RC1, not stable). Will make that happen next.

after @Spokje addresses The Three Stragglers

Well, that "boomeranged" quickly... :)

Pushed commit c80f01a5035f9c743906000778943883d768d50b on 14:52 GMT.

No idea when this shows up in here, but I've addressed #34.

Putting this back to NR.

$ git show
commit c80f01a5035f9c743906000778943883d768d50b (HEAD -> 3270438-remove-cke4, drupal-3270438/3270438-remove-cke4)
Author: Spokje <Spokje@3386132.no-reply.drupal.org>
Date:   Tue Sep 6 16:52:30 2022 +0200

[snipped]

$ git push --set-upstream drupal-3270438 HEAD
Enter passphrase for key '.ssh/id_rsa':
Everything up-to-date
Branch '3270438-remove-cke4' set up to track remote branch '3270438-remove-cke4' from 'drupal-3270438'.

That commit is in the branch (https://git.drupalcode.org/issue/drupal-3270438/-/commit/c80f01a5035f9c7...), but (at least for me) is not showing up in the MR.

#37: Don't worry, this is "just" GitLab being incredibly slow. The Drupal Association is aware. Apparently the current hardware is VERY underpowered: https://drupal.slack.com/archives/CGKLP028K/p1661863372047599

https://www.drupal.org/project/ckeditor/releases/1.0.0-rc1 is live 🚀

#3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project is needed prior to tagging 1.0.0. But went ahead and tagged 1.0.0-rc1 so we can proceed here and in #3306563: [Meta] Tasks to remove CKEditor (4) from core and move to contrib.

To manually test this, I think this is the test script we should follow — this test plan needs review:

1. Install Drupal 9.5.x in the Standard install profile.
2. Install this MR of Drupal 10.0.x
3. Per #3227033-188: Remove Quick Edit from core and #3188544-19: [policy discussion] Address Composer namespacing issues when extensions move between core and contrib, do composer require drupal/ckeditor-ckeditor until — after #3308347: Ensure that ckeditor does not get special core treatment is done we should be able to do composer require drupal/ckeditor instead, but that has not yet happened.
4. Go to /node/add/article, CKEditor 4 should still work fine.

Steps should be:

1. Install Drupal 9.5.x in the Standard install profile.
2. Enable ckeditor module (disabled by default in standard 9.5.x)
3. Configure the Basic HTML format to use ckeditor
4. Install this MR of Drupal 10.0.x
5. Run composer install
6. Per #3227033-188: Remove Quick Edit from core and #3188544-19: [policy discussion] Address Composer namespacing issues when extensions move between core and contrib, do composer require drupal/ckeditor-ckeditor until — after #3308347: Ensure that ckeditor does not get special core treatment is done we should be able to do composer require drupal/ckeditor instead, but that has not yet happened.
7. Visit /update.php or clear caches manually
8. Go to /node/add/article, CKEditor 4 should still work fine.

We might need to do a 9.4.x => 9.5.x => 10.0.x update but we'll need to remove the rdf module between 9.4 and 9.5. Not sure what's best. steps above works.

#41: hah, I forgot that we just disabled CKE4 as being the default even though I worked on that 🙈🤣

#41 looks more accurate for sure. Thanks!

To be explicit I did run the steps in #41 and they are working as intended.

Tested this locally with Umami using steps from #41 and all worked as expected 🎉 Also confirmed I was able to change from the contrib CKEditor 4 to CKEditor 5 using the upgrade path.

1. Confirmed there aren't any remaining mentions to CKEditor 4 ✅
2. I guess technically we would not have to make changes to classy.info.yml because it will be removed from Drupal 10 but I think what is being done there is fine ✅
3. I confirmed that the hook_help updates make sense ✅
4. Confirmed that the MR doesn't remove anything that I think shouldn't be removed, although I am very sad to see Llama CSS including ultra Llama mode be removed 😭🦙 ✅

Based on that, I think this is ready to be committed whenever #3304326: Deprecate CKEditor 4 module in 9.5 is done 🎉

🥳

I just committed #3304326: Deprecate CKEditor 4 module in 9.5 to 9.5.x since the patch here also looks good.

On #3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project @greggles asked for a week for feedback five days ago, so I think we are good to proceed there tomorrow/Friday.

#3308347: Ensure that ckeditor does not get special core treatment is still open but we know that process works with other core module removals, so as long as it's open I think it's fine.

All this to say I think we're good to proceed here, I'll wait until some time tomorrow just to give a little more time for +/-1s on #3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project and potentially a 1.0 for the contrib module if we think that's settled (and because it's late now). Of course other committers welcome to commit this whenever too!

Checked the timelines on #3306909: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project and it's already had over a week for feedback from the security team, a couple of +1s and no objections. Given it was just confirming existing policy, marked it fixed.

I think that's reasonable! Proceeding in #3306630-32: Split the core 9.5.x ckeditor history into a new 1.0.x branch and cut a 1.0.0 release. 👍

Oops — I meant to post #49 on #3306909 🙈 Did that in #3306909-13: Get Drupal Security Team approval to commit vendor/ckeditor.js to the drupal/ckeditor project.

https://www.drupal.org/project/ckeditor/releases/1.0.0 is live, which AFAICT means this is unblocked 😊

@catch pointed out that the MR does not apply to 10.1.x.

Unfortunately this:

$ git diff 10.0.x 10.1.x -- core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
diff --git a/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
index 0e43f969e4..eca98b75fe 100644
--- a/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -355,7 +355,7 @@ public function getJSSettings(Editor $editor) {
 
     // Parse all CKEditor plugin JavaScript files for translations.
     if ($this->moduleHandler->moduleExists('locale')) {
-      locale_js_translate(array_values($external_plugin_files));
+      locale_js_translate(array_values($external_plugin_files), $language_interface);
     }
 
     ksort($settings);

means that we need a separate 10.0.x and 10.1.x patch 😬 For a single line! :P

(That change was introduced by #1014086: Stampedes and cold cache performance issues with css/js aggregation.)

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

This needs release notes.

#55: done.

Also created the missing change record: https://www.drupal.org/node/3308802.