Deprecate CKEditor 4 module in 9.5

Added the changed record and a stub paragraph on the wiki page.

This is also blocked on #3271097: [P-2] Replace CKEditor 4 with CKEditor 5 in the Standard profile and StandardTest (not sure if soft- or hard-blocked — in theory the latter, but maybe we can be more pragmatic?)

Trying to do this in reverse order won't be easier. There are 50 failures in the MR and a lot of them are tests that happen to use the standard profile, so it would mean marking all of them @group legacy then having a follow-up to unmark them later. But in the meantime they could mask issues with other module removals, Symfony 6.2 updates, which are ongoing.

As far as I can tell we are down to one actual Drupal blocker to marking ckeditor5 stable (the image insert URL one) + five accessibility issues of which around half are fixed in the next cke5 release. So if we can get that image insert issue done + update to the ckeditor5 release due in a week, we're down to two in-progress upstream issues which are likely to require zero Drupal changes apart from a dependency update.

Given that I think there are two options that are easier and would probably be OK:

1. Put ckeditor5 into the standard profile before it's marked stable.

2. Mark ckeditor5 stable with the couple of upstream accessibility issues outstanding, on the basis that they'll be fixed before rc.

I don't know if #1 would also run into test failures, #2 would just be a decision we have to make.

But either way, we'd then be able to complete the ckeditor4 deprecation and removal cleanly in time for beta, without introducing workarounds we have to undo later, on the basis that the last stable blockers will be fixed before rc. Also the fact that the bugs were identified in good time is many times better than if we'd identified them after already marking it stable.

We haven't done this before with an experimental module but we've also never had one where the last remaining blockers were on an upstream team we were in close contact with, so I think it would be OK. It's also not adding any new release blocking technical debt since the release is already blocked on it.

Wow, indeed! I had not realized previously what a chicken & egg situation we were in 🐣.

Heh, now multiply that by quickedit, bartik, seven, stable, forum, aggregator, HAL ;)

Pretty sure we'll need both #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests and [#14663486] to happen before we're able to do this one.

We'll also need to replace ckeditor with ckeditor5 in the 9.4.0 database dumps in Drupal 10.1.0. I'm not 100% tests of those will start failing when ckeditor is marked deprecated, but they definitely will when we actually remove it in Drupal 10, and we've been trying to commit the deprecation and removal patches somewhat close to each other since it's the removal patch that really proves the removal is clean, as we've found out multiple times to our cost already.

But hopefully this'll be quite minimal after that (just marking any ckeditor4-only tests @group legacy which should all be in ckeditor module + .info.yml file changes.

@Wim Leers currently the 9.4.0 database dump, which is in Drupal 10, contains ckeditor, which means when you test updates to Drupal 10, ckeditor is installed.

If we remove ckeditor from Drupal 10, but don't remove it from the 9.4.0 database dumps, those tests will fail due to the ckeditor module being missing from the filesystem - so we have to remove it from the dumps in 10.0.x

Simultaneously, we're adding ckeditor5 to the standard profile in 9.5.0, but if we want the standard upgrade path tests to run with ckeditor5 installed, we'll need to add it to the database dumps used for tests - currently we only have 9.4.0 database dumps, none for 9.5.0 or 10.0.0 (and might not add those until just before Drupal 11, certainly can't add them before 9.5.0 is released anyway).

More or less we need to replicate what we're expecting real sites to do, which is install ckeditor4 and uninstall ckeditor5.

Just added comments on #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 - first I've seen of that issue and so far I don't think we should be trying to do it at all.

Agreed! But I'd expect us to create new 9.5.0 dumps instead, instead of pretending CKEditor 5 was stable and the default in 9.4.0

We need to test the upgrade path from 9.4.0 (actually 9.4.4 for sites with ckeditor installed and this is what the dumps use in reality) to Drupal 10.0.0 and eventually to Drupal 10.LAST.0 - so the only options are for that dump to have ckeditor5 in it, or to uninstall ckeditor4 and have all the generic upgrade paths without either module installed, unless we add the stub which I don't think we should.

Almost there - looks like this needs #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests to land first.

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

Fixed the two remaining test failures, this run should be green.

I reviewed the MR and CKEditor 4 is deprecated and that tests are changed as needed.

Reviewing the comments I see that #12 has not been addressed. Setting to NW for that to be resolved.

The merge request looks good from a code perspective. The change record looks good too; I updated it to fix a bad link.

The only question I had was concerning CKEditor's API -- there's a whole plugin type it provides, and two test traits. I don't see explicit deprecations for those. I raised that to Wim and he pointed out that Quick Edit also has similar APIs that are not explicitly deprecated. Our assumption is that the fact that the module is deprecated as a whole covers it from a governance perspective, and any tests which enable the deprecated module will generate errors.

So assuming we're right about that, I don't see any reason to block this.

Good plan to sort out the dumps in #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x, and good that we don't need to update any upgrade tests to @group legacy (although would expect the removal patch itself to fail hard, unless we don't have ckeditor in the main dumps at all).

We haven't been deprecating APIs within deprecated modules because the contrib version of the module won't have those deprecations - i.e. it's not the API that we're deprecating but the fact that the module is in core.

P.S.: I think you want #3270438: Remove CKEditor 4 from core Remove CKEditor 4 from core to be green prior to committing this, right?

Yep, that's the current practice for module removals. It also helps with e.g. grep to make sure all the necessary deprecations are covered. Which is related to...

Question: shouldn't we also deprecate the core/ckeditor asset library in here? 🤓

Yes. Do we also need a blocking issue to move the asset library into CKEditor's own namespace? NW for that.

Reviewed the MR, the release note and the CR and all of them look good for me except one change which I was surprised to see in the MR. I don't think that should block this issue since I don't think it's related to this issue. I think this is ready to be committed to unblock #3270438: Remove CKEditor 4 from core which is also RTBC! 🎉

Committed/pushed to 9.5.x, thanks!