Mark CKEditor 5 stable

Pairing with @lauriii on this.

@lauriii and I followed the pattern established by #3291797: Refactor Drupal 10 settings tray / off-canvas to use modern CSS.

I don't see a mention of custom modules in the CR. It would be worth at least a mention that any custom work thrown at ckeditor4 over the course of 8.x and 9.x will have to be either trashed or rewritten.

Likewise, it should probably be clarified that the automatic upgrade path is guaranteed for core functionality only. Any site owners that have any contrib cke4 modules installed should proceed with caution.

Changes looks good to me +1

Left nitpick remark in MR: https://git.drupalcode.org/project/drupal/-/merge_requests/2712#note_116861

Ready to be committed to 9.5.x; still needs a 10.0.x + 10.1.x patch.

I'm not sure if we want to retroactively mark this stable in 9.4.x? 🤔 That'd mean that CKEditor 5 would be experimental in 9.4.0 through 9.4.5 and stable in 9.4.6. I don't think we've ever done that before.

Let's do the 10.x and 9.x commits together.

Also a question on the MR (which also affects the 9.4 backport question).

I can understand marking them internal, but why couldn't we have started out like that, is it just an oversight?

I don't think the pattern to mark internal. existed when these library definitions were created so we didn't consider that at the time. Also, there was some uncertainty about whether we should mark the libraries internal or not but now it's very apparent that it's required to be able to keep up with the faster release cadence that CKEditor 5 is following.

This was all raised up now given that the Stable related tests started failing after marking CKEditor 5 stable. We started by bypassing those libraries manually in the tests given that we had already marked most of the code inside those libraries internal within the code itself. As we were doing that, we realized that we have some other libraries following the pattern of prefixing an internal library with internal., so we thought we might have to follow that too. We could theoretically split this step into another issue, but we could only test that it was all done correctly on this issue, hence the proposal to do it all in a single issue.

OK I think that is fair enough. I don't know what it means for a 9.4 backport, but we should get this into 10.1-9.5 first anyway.

Looks like a real test failure on the MR still.

This should address the test failures.

The patch in #20 does not apply to 9.5.x, because the IE11 warnings that CKE5 will show in Drupal 9 have been removed in Drupal 10. So we'll need two patches. 😬

The MR is against 9.5.x and should be up to date ✌️

#22 Aha, glad to be wrong! 😄

Change record created: https://www.drupal.org/node/3308362

The release notes in the issue summary were really targeted at experimental module users, not stable module users. So, updated that. Also incorporated @Luke.Leber's concerns.

Finally, crediting all participants in this issue as well as all contributors to the contributed CKEditor 5 module.

The release notes should usually just be a couple of lines pointing to the change record. Since they were nearly identical I went ahead and made that change.

Also tagging for release highlights for both 9.5.0 and 10.0.0

OK then. Committed/pushed to 10.1.x and cherry-picked to 10.0.x, and then the MR diff to 9.5.x separately.

This was a massive effort, really nice to see the module stable and ckeditor4 close to removal.

Moving back to 9.4.x for discussion, will get second+ opinions from the other release managers.

If this was only marking the module stable, I'd definitely be keen to backport to 9.4 to encourage people to start switching asap, with the libraries moving around I am not so sure, but given it's unstable in 9.4 maybe we can just backport it like we have other changes.

but given it's unstable in 9.4 maybe we can just backport it like we have other changes.

This is what I was thinking 🤓 It's experimental in 9.4, so … why not?

Thanks to all people who made this happen!

So there are a dozen of modules that now need an update path to make their module not only compatible with D10 but also with CKEditor 5. Could a CKEditor update day be organised, maybe during a D10 porting day ? :D :D

I admit I could use some help with 2 modules that have custom filters ;)

I was initially neutral on backporting this to 9.4

Then I thought it would be a very good thing if new 9.4 sites started on ckeditor5 for the next three months - this would be a very good reason to backport.

Then I realised that the standard profile in 9.4 still has ckeditor 4 enabled instead of ckeditor 5, so if we don't also backport that change, then new sites will probably still end up with ckeditor4 installed.

So... no idea tbh.

Changing Standard install profile in a patch release would be unprecedented and quite possibly disruptive.

Making the CKEditor 5 module stable in 9.4.6 would not be disruptive IMHO; if anything, it would be empowering: it'll allow sites to switch to CKEditor 5 now, before 9.5.0 is even out.

So I personally still think that backporting this issue to 9.4.x but not #3307186: Mark CKEditor 5 stable is the best course of action 🤓

it'll allow sites to switch to CKEditor 5 now, before 9.5.0 is even out.

Yes I still think that would be useful, it's just going to be a smaller number of sites.

Agreed. We might as well not do it I guess … but on the other hand, it might convince some additional sites to make the switch before upgrading to Drupal 10 (because "stable" instead of "experimental"). Which means fewer things to worry about for them when they do make the switch to 10.

So IMHO: no technical benefit, fair amount of psychological/ecosystem benefit ⇒ nice-to-have.

The benifits are pretty clear to do so. Although they are quite minimal i think. People who want to prepare would do so anyhow.

What i don't like about it is the fact this goes contrary to policy. This normally ends up generating a lot of discusssion and back and forths. In a time where a lot of the people who would discuss this are REALLY needed to make sure we get Drupal 10 beta ready. Not sure this is something i'd propose, just for that reason.

But the genie is out of the box already, so eh. I wouldn't mind, but i don't think there is really much benifit in doing so.

Maybe this was discussed already and missed by me, but if ckeditor 5 is going to be marked stable in 9.4.6, mustn't the 9.4.x ckeditor5 code be made up-to-date as well? Some recent issues were committed to 9.5.x but not to 9.4.x.

but given it's unstable in 9.4 maybe we can just backport it like we have other changes.

I think it would be great to have as much as possible of ckeditor5 available in 9.4, wether marked stable or not.

I had noticed two critical issues from the roadmap not being committed to 9.4.x:
#3294908: Configuration overlaps between Styles and other CKE5 plugins
#3268306: [GHS] Custom/unofficial HTML tags not retained: <drupal-media>, <drupal-entity>, <foobar>

Maybe there are more, critical or not critical.

The ckeditor5 module component shows more Closed(fixed) 9.5.x-dev issues. So it appears that ckeditor5 is not really up-to-date in 9.4.x. Is that the case or are these special issues?

@Eric_A: you're absolutely right. git diff 9.4.x 9.5.x -- core/modules/ckeditor5 makes that very clear.

Discussed this with @alexpott a bit. We've backported a lot of the diff between 9.5 and 9.4 which is great, however also we were only able to do that because ckeditor5 is still experimental in 9.4. If there's a minor-but-not-patch-safe fix that needs to go in within the next couple of months, it'll be easier to backport if ckeditor5 is still experimental - once we mark it stable everything gets a lot stricter.

So I think it's probably better to leave things as it is, so that we can keep backporting for the next few weeks, and then 9.5 will be released soon anyway.

Going to go ahead and mark fixed against 9.5.x

👍 I had similar thoughts! 😊