Problem/Motivation

CKSource has updated the CKEditor5 47.6 fork for Drupal with bug fixes and made a public copy of the fork.

This is to bump to the latest version on the fork for the Drupal core 10.6.x, 11.3.x, 11.4.x, 11.5.x, and 11.x branches.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3625998

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

godotislate created an issue. See original summary.

brevilo’s picture

brevilo commented
Related issues: +#3624195: CKEditor 5 library metadata references non-existent upstream version 47.6.3

Please don't just bump to "the latest version on the fork". Ensure you pick a version that's based on a public release of the upstream repo. This is the only way to retain proper compatibility with derivative package builds like ckeditor5-markdown-gfm. See this related issue for details.

Thanks

godotislate opened merge request !17367

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs review

Tried updating this with the public repo:

  • Applied the script changes from #3622641: Update CKE build script to allow a private path
  • Cloned the CKEditor5 public fork
  • Built the packages there with:
    pnpm install
pnpm run release:prepare-packages --compile-only --verbose # See note below*
  • Followed the documented Drupal core CKEditor 5 update steps, only with the change of setting the CKEDITOR_PACKAGES_PATH env variable pointing to the CKEditor local repo's release/npm directory:
    CKEDITOR_PACKAGES_PATH=/var/www/html/ckeditor5/release/npm yarn run vendor-update
CKEDITOR_PACKAGES_PATH=/var/www/html/ckeditor5/release/npm yarn run build:ckeditor5
CKEDITOR_PACKAGES_PATH=/var/www/html/ckeditor5/release/npm yarn run build:ckeditor5-types

The result was a relevant change to only one file, and it backports to 11.5.x, 11.4.x, 11.3.x, and 10.6.x cleanly, though there will likely need to be separate MRs for the performance test changes.

*I'm not completely sure this was the right command. The release/npm directory was created as expected, but the performance test is showing that script bytes increased by 6K, which seems to be higher than would be expected.

Re: #2

Please don't just bump to "the latest version on the fork". Ensure you pick a version that's based on a public release of the upstream repo.

This is the release process provided by CKSource. 47.7.* is for CKSource's LTS customers only, and 48 has a completely different build process that would break even more Drupal 11 contrib and custom integrations. There have been inquiries and discussions about alternatives, but so far this is what is available.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

I think this should update the version number of all ckeditor libraries in core.libraries.yml.

godotislate changed the visibility of the branch 3625998-update-ckeditor5-version to hidden.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
Status: Needs work » Needs review

Changed the version in core.libraries.yml to 47.6.3-drupal.1 and updated the license URLs as well.

Edit: FWIW, I did initially try yarn add ckeditor@https://github.com/drupal-ckeditor/ckeditor5.git, but apparently yarn can't install from git repos that use pnpm yet.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks right to me. Double checked the latest tag really is -drupal1 on https://github.com/drupal-ckeditor/ckeditor5/tags

brevilo’s picture

brevilo commented

Changed the version in core.libraries.yml to 47.6.3-drupal.1 and updated the license URLs as well.

Please let me reiterate that choosing version 47.6.3 as the basis will still be in conflict with the publicly available CKeditor packages. Please see comment #2.

Thanks

catch’s picture

catch
he/him
Primary language English
commented

@brevilo yes we know, we don't like it either. However we also can't arbitrarily break hundreds of thousands of sites in a patch release, and even if we did do that, we'd do it in a dedicated issue with a whole announcement process before it, not this one.

See #3588899: [policy, no patch] Move ckeditor to contrib for some more discussion.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The first line of the minified ckeditor DLL file now contains a dictionary that was missed from the older 47.6.3 release; it was present in other releases. This is also why the performance test had to change.

The rest of the file differs by 22 bytes which I believe is just a fix for Safari.

  • longwave committed 7a76a898 on 11.5.x 
    task: #3625998 Update CKEditor5 version for Drupal core 10.6, 11.3, 11.4...

  • longwave committed abc67c39 on 11.x 
    task: #3625998 Update CKEditor5 version for Drupal core 10.6, 11.3, 11.4...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 11.x and cherry-picked to 11.5.x.

Doesn't backport cleanly to 11.4, 11.3 or 10.6 because the performance test refactor was not backported. I think they will need updating separately so marking this for backport instead of skipping that change and blindly committing.

catch opened merge request !17383

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Needs review

Pushed an 11.4 backport

catch’s picture

catch
he/him
Primary language English
commented

11.3 has more conflicts than just the performance tests, concentrating on getting the 11.4.x MR green.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

11.4.x MR looks the same as 11.x other than the performance test numbers.

  • longwave committed 6d0e6099 on 11.4.x 
    task: #3625998 Update CKEditor5 version for Drupal core 10.6, 11.3, 11.4...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.4.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Pushed to 11.4.x.

godotislate opened merge request !17385

godotislate opened merge request !17386

godotislate’s picture

godotislate
he/him
commented
Status: Patch (to be ported) » Needs review

10.6.x: https://git.drupalcode.org/project/drupal/-/merge_requests/17386
11.3.x: https://git.drupalcode.org/project/drupal/-/merge_requests/17385

Apparently neither needed performance test adjustments, but I think that's because that specific test isn't in those branches.
Otherwise the conflict was in core.libraries.yml, because 11.3.x and 10.6.x don't have ckeditor fullscreen.

godotislate’s picture

godotislate
he/him
commented
Version: 11.3.x-dev » 10.6.x-dev
godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Actually, since the actual JS file backport was clean, and the conflict resolution in core.libraries.yml was trivial, I'm going to go ahead and move this to RTBC for 11.3 and 10.6

aitala’s picture

aitala commented

Hi,

Am I correct that this version does not fix the issue raised in https://github.com/ckeditor/ckeditor5/issues/20237 ?

Thanks,
Eric

catch’s picture

catch
he/him
Primary language English
commented

@aitala I think that's why they put out the new release? The repo contains this commit: https://github.com/ckeditor/ckeditor5/commit/17899047791161e271598fd6071... so unless I'm missing something, yes it does fix it.

aitala’s picture

aitala commented

@catch Yes, you are correct, I just had a difficult time spotting that in the repo.

Thanks,
Eric

  • longwave committed d04d5829 on 11.3.x 
    task: #3625998 Update CKEditor5 version for Drupal core 10.6, 11.3, 11.4...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d04d582950e to 11.3.x. Thanks!

Committed and pushed 3eddabf708b to 10.6.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed 3eddabf7 on 10.6.x 
    task: #3625998 Update CKEditor5 version for Drupal core 10.6, 11.3, 11.4...
neclimdul’s picture

neclimdul
He/Him
commented

This did break ckeditor5_premium_features. #3629880: Premium plugins fail to load on Drupal 11.4.9: the CDN path uses core's 47.6.3-drupal.1 version

This might be an unexpected consequence for developers based solely on the changelog. It was for me.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

This is related to #3624195: CKEditor 5 library metadata references non-existent upstream version 47.6.3 but there is nothing we can do while the Drupal CKEditor LTS releases exist separately to the standard CKEditor npm packaging. This will likely happen again if there is another LTS release and the version number is bumped in some other unexpected way.

catch’s picture

catch
he/him
Primary language English
commented

CKEditor premium features is maintained by the CKSource team who are also responsible for how the LTS is handled, so I think it would be reasonable for someone to open an issue in that queue to ask them if they have a way to mitigate breakage of their own module with the next LTS release of ckeditor.

jonathan webb’s picture

jonathan webb commented

As a maintainer of a site with CKEditor Premium, the last couple of rounds of Drupal core security updates have been pretty frustrating. Could Drupal consider proactively marking conflict with incompatible versions of CKEditor Premium in composer.json?

neclimdul’s picture

neclimdul
He/Him
commented

I can say everyone is feeling that pain. But I don't think its reasonable for core to maintain a supported list of a contrib module, core maintainers have enough to maintain.

I think catch is right, CKSource needs to be involved in a longer term fix. I've opened up that issue to hopefully start that dialog and make sure we're a bit more confident the CKEditor contrib ecosystem can be stable going forward. I hope I captured the concerns well but feel free to clarify.

#3631234: Long Term fix for LTS support

jonathan webb’s picture

jonathan webb commented

Thank you!

Status: Fixed » Closed (fixed)

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