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

Posted self-review, mostly to explain the "why" for various things.

Pushed proper test coverage for verifying that some text format/editor combos will have the text format updated too. This was surprisingly tricky to figure out. It did not help that the 9.4.0 DB fixtures appear to have a broken basic_html text format 🤯

Anyway, I expect tests to fail on this. And after they do, I'll push the fix.

4 failures, but 2 of the 4 are unrelated functional JS tests that are failing randomly.

The last commit should've been green, but it's not. The only failure is Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testOffCanvasStyles(). This is a 100% unrelated random test failure.

This needs review! 🤓

Addressed all MR feedback.

Next: #10.

#10: nice manual test! 😄 I definitely did not think about that, so really grateful to have you review this too! I agree that it's kind of up to #3270438: Remove CKEditor 4 from core to solve this … but … it'd be great to get it out of the way now! 🤞

The root cause is \Drupal\editor\Entity\Editor::__construct():

  public function __construct(array $values, $entity_type) {
    parent::__construct($values, $entity_type);

    $plugin = $this->editorPluginManager()->createInstance($this->editor);
    $this->settings += $plugin->getDefaultSettings();
  }

It's trying to instantiate the plugin even though it may no longer exist — well, it still exists as far as the config entity knows thanks to config entity dependencies, which for all CKEditor 4-using Text Editor entities contains:

dependencies:
  module:
    - ckeditor

The next question is: does this happen only in the test (i.e. some artificial state in the test), or does it also happen in the actual upgrade path on actual sites? Well, looks like it's the latter! 😬

That leaves us with a few possible choices:

1. Harden Editor::__construct()
2. Modify \Drupal\Core\Config\Entity\ConfigEntityUpdater::update() to not load config entities — impossible 🙈

Okay, ONE choice :P

Implemented that: https://git.drupalcode.org/project/drupal/-/merge_requests/2647/diffs?co...

#15: #3270438: Remove CKEditor 4 from core will test that, right?

#16: Wow, I'm surprised!

I've always assumed the ckeditor5 would be a manual step that you opt into, either in 9.5 or 10.x

I've literally been told the opposite! 🤯 This issue is what the whole upgrade path infrastructure, ranging from \Drupal\ckeditor5\SmartDefaultSettings and thoroughly tested by \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest has always been working towards! 🤓

1. The upgrade path currently checks for 'contrib' ckeditor module, but if core's ckeditor module is missing, there'll be a missing module error and it'll be impossible to uninstall. So this would require adding a stub ckeditor module to Drupal 10.

Why/where would there be a missing module error when core's CKEditor module is "missing" (I think you mean once it's been removed?).

Part of the update to Drupal 10 would have to remove ckeditor from core.extension, just like all other removed modules & themes, no? What am I missing? 🤔

We would need to ensure that editor config entities do not get deleted despite their dependency on the ckeditor plugin & module — but that's exactly what #3270831: Make the CKEditor 4 → 5 upgrade path work even when the CKEditor 4 module is removed solved.

2. What happens if you have a contrib or custom module that integrates with ckeditor4 with a ckeditor dependency, and you port that module to Drupal 10 but not ckeditor5? If we have a stub ckeditor module in core, then you'll get no warning about this, but your ckeditor plugin will just stop working - no idea what happens to the config in this case since the editor migration can't be 1-1.

1. Those contrib/custom modules can and should provide an upgrade path. That's what \Drupal\ckeditor5\Plugin\CKEditor4To5UpgradePluginInterface exists for.
2. For any CKEditor 4 toolbar items/plugins that do not have an explicit upgrade path, their details will be logged and all the markup that they could generate _will_ still be editable, thanks to Source Editing. We have extensive test coverage for this in \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest.
3. The Drupal 10 port of such a module would work fine with the upcoming Drupal 10-compatible temporarily-maintained release of the CKEditor 4 module in contrib, over at https://www.drupal.org/project/ckeditor. That buys them time to migrate.
4. Even if we have a stub CKEditor module in core, then you will have no CKEditor 4 at all, and hence of course your CKEditor plugin will "just stop working" — nothing will ever execute it 😅

If we don't provide a ckeditor stub and this upgrade path, then you'll need to be able to uninstall ckeditor in order to complete the update, which will at least provide some warning that you need to do some extra work. With a manual step you get to find this out in your face, any upgrade path error is ephemeral.

How can you uninstall if we don't provide a stub? Because the CKEditor 4 module will not exist at all in 10.0 then? Do you mean that people would have to revert back to 9.4/9.5 and then uninstall CKEditor 4?

Also: no, this upgrade path error is not ephemeral — see the second screenshot in the issue summary: we log the details of the automatic upgrade path to watchdog/syslog 😊

3. Providing some automation is nice so you don't have to click through loads of forms and export config etc. Could we have a form with a batch script? Or do this in ckeditor5 install? Then it's still sort-of opt-in separate from the 10.x update.

🤔 What is the advantage of doing this in ckeditor5_install()? I would not expect that installing a module will modify my config entities? See https://git.drupalcode.org/project/drupal/-/merge_requests/2647#note_113169 for prior discussion.

#18

This is what happened to me in #10 after rm -rf core/modules/ckeditor and bypassing the failing part of the test; $this->runUpdates() fails due to "The following module is marked as installed in the core.extension configuration, but it is missing: ckeditor".

But that's just because we haven't done #3270438: Remove CKEditor 4 from core yet, which is why ckeditor is still in core.extension. That's fixed since #14?

If we are expecting users to upgrade from 9.4 onwards to 10.0 with CKEditor 4 still installed, they will run into this error before any update hooks can be run if the module is deleted entirely (and they haven't installed the contrib version).

Surely other removed modules have had to deal with this too? Or do we just leave them in the core.extension configuration? (A quick glance at #3270899: Remove Color module from core doesn't seem to have any code to remove color from it! 😦)

Part of the update to Drupal 10 would have to remove ckeditor from core.extension, just like all other removed modules & themes, no? What am I missing?

The only current way of doing this cleanly is leaving behind a stub ckeditor.info.yml (probably marked as lifecycle: obsolete) and writing a post-update hook for editor.module or system.module that uninstalls it. We did this for e.g. SimpleTest in #3110862: Remove simpletest module from core.

🤯🙈 Aha, wow! Maybe this is what explains parts of @catch' comment! I did not know that. This approach really surprises me, because … if we can't do it now, then when can we? In Drupal 11? It does make sense: this will indeed allow the stub *.info.yml to be removed in the next major, right? 😄

I looked at system_post_update_uninstall_simpletest(), that makes sense for that one module. How was this handled for color? AFAICT it isn't handled at all?

But in this case, as far as I know, uninstalling CKEditor will also delete the config that depends on it!

Exactly! That's a no-go. 😬

So I think that for ckeditor what would make sense is something like this:

/**
 * Uninstall CKEditor 4, without invoking uninstall hooks/config entity updates.
 *
 * The CKEditor 4 module cannot simply be uninstalled because Text Editor
 * config entities depending on it would be deleted; instead the automatic
 * CKEditor 5 upgrade path will update these config entities.
 */
function system_post_update_uninstall_ckeditor() {
    $config_sync = \Drupal::service('config.storage');
    $config_data = $this->config('core.extension')->get();
    unset($config_data['module']['ckeditor']);
    $config_sync->write('core.extension', $config_data);
}

I think this would have to be in the same post_update hook as the one that performs the upgrade, as right now we have no way of controlling the order that they run in: #3124766: Add equivalent of hook_update_dependencies for post update hooks

Right!

What if instead of creating a new system_post_update_uninstall_ckeditor(), I'd just add the above to this MR? AFAICT that then solves the problem? (Along with keeping a stub ckeditor.info.yml, but that's for #3270438: Remove CKEditor 4 from core. Doing the uninstall here already means that the clean removal is already handled.)

#19:

It's only been open for a week though

Because we forgot about this 🙈🙈🙈🙈🙈

and it's never been listed as a stable blocker.

Because it isn't a stable blocker. The CKEditor 5 module can be stable in 9.5 without the automatic upgrade path from 9 to 10 being ready, right?

The smooth upgrade path when you enable ckeditor5 module is great, but that's not the same as force-upgrading in a post update.

I'm not sure I understand why you say "force upgrading"? 😅

So:

• the CKEditor 4 module will be removed from Drupal 10
• this results a broken content editing experience for end users
• we can ensure there is a working content editing experience for end users, by automatically installing CKEditor 5 and running the upgrade path
• … but of course we will NOT do that if the D10-compatible release of https://www.drupal.org/project/ckeditor is installed, in that case, we do absolutely nothing

  Does that make sense? Because that is what this is aiming to achieve 😊

  Yes, but what if they don't, and we irreversibly upgrade you from ckeditor4 to ckeditor5 anyway, because we added a compulsory post update to 10.0.x that you didn't know about?

  We could retain a copy of the original pre-automatic-update-path Editor config entities, to allow a site owner to re-run the upgrade path for a particular text editor again in the future, when those modules providing a CKEditor 4 plugin have A) written an equivalent plugin for CKEditor 5, and/or B) implemented an upgrade path.

  That'd allow you to re-run parts of the upgrade path you're not satisfied with.

  In principle this is the responsibility of the site owner ("make DB back-ups" and all that), but I personally would definitely be in favor of being more empathetic with the end user! 😊

  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 and try again, or install the contrib module and follow the 10.0.x version above.

  Agreed on the first.

  Disagreed on the second. You should only have to install https://www.drupal.org/project/ckeditor if you wanted/needed to. Most sites should be fine going directly from CKEditor 4 to 5. (Also see the previous point about being able to re-run the upgrade path after upgrading more contrib modules in the future.)

  That being said, what you describe would be simpler for us (Drupal core maintainers) to implement — it'd just be harder to get the ecosystem off of CKEditor 4? 🙈

  I think it's possible to add a confirmation page when installing a module, it's not possible to do that for an update. The automated update is literally installing a module and modifying content entities and there's no way to stop it from doing that - at least not within the upgrade process itself.

  Right again! And another reason this would indeed be simpler for us (Drupal core maintainers).

  ---

  @catch's proposal summarized

  AFAICT @catch's concerns lead to the following proposal:

  Drupal 10 must require the owner of an existing Drupal 9 site to:

  • manually install the CKEditor 5 module (in Drupal 9 or 10)
  • manually uninstall the CKEditor 4 module (in Drupal 9 or 10)
  • not allow updating a Drupal 9 site with CKEditor 4 still installed to Drupal 10 without also installing the https://www.drupal.org/project/ckeditor contrib module for D10 (necessary for point 1 to still be true)

  Did I get that right, @catch? 😊

Yes agreed it wouldn't be a stable blocker but it would be an as yet undiscovered 10.x beta blocker then.

Yup, unfortunately! That's why I tagged it Drupal 10 beta blocker right away. Did I forget a tag maybe?

Don't see how we can do that because this also affects filter configuration in some cases and storing an extra text format... Gets messy quickly since those are referenced.

Right, we'd need to store both. But for the text format, we'd only need to store filter_html's configuration. That's definitely doable.

Yes, but... they could just install ckeditor5 in Drupal 9 before they update.

I think this means you're saying we should have very strong messaging in Drupal 9.5 to nudge site owners to upgrade from CKEditor 4 to 5, maybe even have a warning in the status report?

but we can't 100% know the contrib version isn't installed if extension discovery is stale for any reason, because then core will see the core version and not the contrib one

Can't we force this to be re-discovered? 🤔

A backup helps you if you get a big fatal error during or immediately after upgrade. It often does not help you if something quietly goes missing from your site and you don't notice it for a day or week afterwards because by then you might have new content, or have already discarded your backup.

Definitely true! Which is why I've always felt it was such an enormous responsibility to make this automatic upgrade path as solid as possible 😰😊

But … I I don't see how in this case we could end up deleting people's data? If you use Full HTML, all markup is retained in CKEditor 5 just like in 4. If you use filter_html, we go through great lengths to ensure that as much of tags allowed by filter_html use CKEditor 5 functionality, but for anything where we can't do that (and that would include all of the custom/contrib CKEditor 4 plugins!), we ensure that you have all of the unsupported tags, attributes and attribute values made explicitly editable using the Source Editing functionality. Which means that it's guaranteed that all markup that CKEditor 4 would have retained (remember, CKEditor 4's "ACF" is also configured to allow only the markup allowed by filter_html!) will also be retained by CKEditor 5.

So unless you can point to a flaw in that last paragraph, I respectfully disagree that this could ever result in data loss. The worst that can happen here is that your CKEditor 5 text editor is less feature-rich than your CKEditor 4 one, because custom/contrib CKEditor 4 plugins are not yet compatible with CKEditor 5.

And that is why I'm not quite convinced yet by your proposal 😅 The risk is squarely on the Text Editor configuration side, not data loss. I (currently) think that is preferable to having to deal with getting >90% of Drupal 10 sites off "the CKEditor 4 contrib module for Drupal 10 that You Really Should Not Use 🧟‍♀️" before the CKEditor 4 EOL in 2023.

ℹ️ The chosen approach here determines whether we need to do #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x.

P.S.: I've been quiet only because I have nothing to add in the mean time: I understand @catch's proposal now, I can see how it can work 👍 I expressed some doubts, but I could be convinced. I could also be convinced about the approach that the current MR implements, which I implemented based on discussions with the other Drupal core release manager besides @catch: @xjm.

tl;dr: I'm awaiting @xjm's feedback on this issue 😊 (Made that more explicit now.)

@Wim Leers, @nod_, @bnjmnm, and I discussed this at length yesterday and agreed that we were all comfortable with not having this in an update hook once we understood the concerns.

+1

I do think we might want to provide a manual one-click script that batches the editor updates and consolidates information about it since this upgrade is a > 90% usecase, rather than the < 5% usecase that most module removals are.

-1

The point is that you have to inspect every particular text format & editor. Then batching it anyway kind of defeats the purpose of this changed approach.

I've begun by pushing #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x forward: the issue summary there has been written to reflect @catch's proposal on this issue and on the related issues. This is soft-blocked on that. I'll continue pushing this forward next week. Unassigning until then to indicate it's fine for others to push this forward until I continue 🤓

#3270438: Remove CKEditor 4 from core is no longer postponed, so this is now less blocked 👍 Will continue this tomorrow!

Situation update

Since we last touched this issue:

1. CKEditor 5 became stable in >=9.5 and the default in >=9.5
2. CKEditor 4 was deprecated in Drupal 9.5
3. CKEditor 4 was removed from Drupal 10
4. https://www.drupal.org/project/ckeditor_codemirror has its first CKEditor 5-compatible release, with upgrade path (I tweeted about that) — this will help ~10K Drupal 9 sites — thanks @wells! 👏
5. https://www.drupal.org/project/entity_embed has a PoC for adding CKEditor 5 support: #3272732-13: Drupal 10 & CKEditor 5 readiness — this will help ~70K Drupal 9 sites
6. a lot of movement on the upgrade coordination, especially by @mustgrave and @shelane: https://www.drupal.org/node/3273985/revisions 🙏🥳

In short: a lot has happened. The ecosystem is working towards CKEditor 5 support 👍

Getting this issue back on track

This issue's original intent — providing an automated upgrade path — was the course of action in comments #1 through #15 and MR 2647.

@catch argued in #16 that this should be manual, not automated. Discussion followed in #17–#25 and culminated in @xjm summarizing the conclusion in #26.

I'm proposing a concrete implementation of @catch's vision here:

— the text matches the warning on https://www.drupal.org/project/ckeditor.

🤔 I could imagine it go slightly further: list the text formats that are currently using CKEditor 4, and explicitly instruct to uninstall the CKEditor 4 module too?

(Please ignore the total disregard for translatability and proper use of safe markup in the patch — it's a PoC to discuss. It should apply the same logic as $create_extension_incompatibility_list() does in system.install to generate this markup.)

Right, but this is going quite a bit further, because all those other modules are less … "essential" than CKEditor 4, and definitely are less disruptive removals/upgrades.

I'm glad to see that issue RTBC already, it definitely should land before this.

Then I think this issue could indeed special case CKEditor 4 and exempt it from that list, and instead do something like what I'm proposing? 🤔

I'm not sure if we'd want to remove ckeditor special-casing from system.install or move this logic into system.install, but probably one or the other, so marking needs work.

I think we can keep everything #3294914: Create dedicated error section for missing removed core modules/themes on update unchanged. That ensures this exists:

This patch merely adds an additional section , which provides helpful additional context:

(Changes since last patch: I removed the use of Markup::create() and computed the list of text formats/editor using CKEditor 4.)

⚠️⚠️⚠️⚠️ Due to the d.o composer facade limitations, many Drupal 9.4 sites have already installed the https://www.drupal.org/project/ckeditor contributed module. Those sites will never see this! 😱

This makes me wonder whether we should add a version check too, to explicitly warn sites that do have the https://www.drupal.org/project/ckeditor module installed that they too should make the switch at some point.

Went ahead and did that, and refined the text in the process:

Drupal 10 site without https://www.drupal.org/project/ckeditor

Drupal 10 site with https://www.drupal.org/project/ckeditor

Tagging String change because this unavoidably adds more UI strings…

phpcs

@catch: hah, that works too 🤓 That would sidestep the core review process for sure…

… but given that we definitely need to point users to the contrib module anyway, we'll need something to land here, so we might as well do that?

Sounds like this could have prevented #3374544: TypeError: array_intersect(): Argument #1 ($array) must be of type array, string given in array_intersect() (line 203 of .../core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php)..

This would likely have prevented #3396771: ckeditor plugin does not exist.

I tried many times to push this forward but failed to convince core committers.

Closing this while triaging the issue queue.