[11.x] Remove the CKEditor 4 upgrade path

>14 months have passed since #11: it's now late 2023, tomorrow is December 1!

https://www.drupal.org/project/ckeditor → will be unsupported soon.

Time to start exploring our options.

I was on the fence about deprecating or not but asked the other rms. Catch replied saying that in this it is ok to remove them. He pointed out that plugins are internal and these are a specific use case. Plus, since these will be in use for year or two, possibly three, it would be more disruptive to deprecate.

Based on that I started to remove files. I did remove some provider tests from SmartDefaultSettingsTest but there remains a lot of CkEditor4 strings in setup().

This will fails tests so settings to NW.

Very interesting, I sort of expected this to not happen 😅 Then again, sites can't go from Drupal 9 to 11 directly (i.e. no bypassing Drupal 10), so … it makes sense to do this.

It triggers … Feels 😅 to be removing this — I spent a lot of time building this. And I built it for clean ejection when it was no longer needed, which is NOW, so it does feel kinda satisfying (cathartic?) to see how nicely this is removing ~1250 LoC (and more soon) 🤩

MR reviewed — I explained which other test cases (which you already alluded to, @quietone!) that are safe to remove can be identified 😊

Changing the issue title to indicate we'd only remove this in 11.x - this means sites that are still using ckeditor4 will be able to use the upgrade path on 10.x, which is going to be maintained until around 2026, years after ckeditor4's EOL.

The reason I think this would be more disruptive to deprecate than just remove outright in 11.x is because if we deprecate we either have to remove usages (which we wouldn't want to do in 10.x, there are still well over 100,000 Drupal 9.5 sites), or workaround the deprecation notices in tests etc., which is not really deprecating then. We offer 'best effort' bc and deprecations for internal code, and in this case the best we can do is none at all.

+1 to no deprecations, but maintaining this throughout Drupal 10 and a clean removal in Drupal 11, given that sites already must go from 9 to 10 before they can go to 11.

Failing on PHPStan:

  139    Parameter $stylesheets_message of method                             
         Drupal\ckeditor5\Plugin\Editor\CKEditor5::__construct() has invalid  
         type Drupal\ckeditor5\CKEditor5StylesheetsMessage.                   

Simple fix fortunately: that is just no longer necessary :)

10.3 has been branched. Does that mean this can land now? It's unlikely the affected will change in 10.3 too, so it's also unlikely that this'll make MRs more difficult to land?

@Wim yes :)

Merged in upstream changes. Then finished what @quietone started: removed all traces of CKEditor5StylesheetsMessage.

The test cases in SmartDefaultSettingsTest are now almost all stale/irrelevant: they're specifically testing an update from CKEditor 4, and that's no longer necessary.

Because going forward, SmartDefaultSettings can and will only look at the HTML restrictions of the text format. Will tackle that next.

https://git.drupalcode.org/project/drupal/-/merge_requests/6329/diffs?co... got us down to 2 failing tests: SmartDefaultSettingsTest as predicted in #27, plus \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testSwitchToVersion5().

I just pushed a commit that should make the latter pass, the commit message explains how/why.

I recommend reviewing the test coverage changes using

Only two <code>@todo

s are left. This is 98% ready.

So long ckeditor4!

Following to review later.

And now it will be green! 😄 Whew, this was quite a lot of work: to keep the good parts of SmartDefaultSettings that are still relevant now that CKEditor 4 is gone.

Bumping to given the diffstat of +367, −1731.

Reran the javascript failing test multiple times but seems to consistently fail :(

Shoot!

Ckeditor just doesn't want to go away!

Merged in latest 11.x.

The failure is reproducible locally:

TypeError: Cannot read properties of null (reading 'length')
    at Object._parseSetting (http://localhost/subdirectory/core/modules/filter/filter.filter_html.admin.js?v=11.0-dev:310:39)
    at http://localhost/subdirectory/core/modules/filter/filter.filter_html.admin.js?v=11.0-dev:95:30
    at Array.forEach (<anonymous>)
    at Object.attach (http://localhost/subdirectory/core/modules/filter/filter.filter_html.admin.js?v=11.0-dev:90:9)
    at http://localhost/subdirectory/core/misc/drupal.js?v=11.0-dev:166:24
    at Array.forEach (<anonymous>)
    at Drupal.attachBehaviors (http://localhost/subdirectory/core/misc/drupal.js?v=11.0-dev:162:34)
    at http://localhost/subdirectory/core/misc/drupal.init.js?v=11.0-dev:32:12
    at HTMLDocument.listener 

… but it'd be solved automatically by 📌 Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 Fixed .

📌 Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 Fixed landed!

All green! 😄 🥳

90% of this MR is trivial.

The 10% that is non-trivial is in SmartDefaultSettings. For it to continue to work (to provide default settings based on HTML restrictions of a text format, which 99% of the time means based on filter_html configuration), a few additions were necessary.

You might question the need for that. Why:

1. It makes it easy for existing text formats to adopt CKEditor 5
2. It minimizes the changes to SmartDefaultSettingsTest

Removal seems good. But question should this code be moved to the ckeditor4 contrib module?

I can imagine there are some still on ckeditor4 and they may upgrade to D11 while still trying to get to ckeditor5.

I think it's entirely reasonable to require people to update to ckeditor5 on Drupal 10 before going to Drupal 11, they've got until 2026 to do that and still be supported.

Good point, forgot about the 2 year overlap

I read through everything again and there was a few things I thought 'should we be deprecating this in 10.x first' and then I thought we don't want to deprecate it in 10.x because we want people to use it there to get onto ckeditor5, and then I re-read the issue and found us discussing the same thing already including me months ago and all the other release managers.

Committed/pushed to 11.x, thanks!

catch → closed merge request !6329

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

We've still got a couple of ckeditor4 editor config exports supporting various tests, which I think possibly might not be for actual ckeditor4 upgrade path testing at all, but in which case should probably be renamed. Opened 📌 Remove redudundat hook_ENTITY_TYPE_presave() and ViewsConfigUpdater methods Active for that and some other issues when I tried to remove ckeditor5's overall 9-10.3 upgrade path.