Problem/Motivation

#3396133: CKEditor 5 JS error when editing certain content raised confusion around the fact that the CKEditor 5 support that was added in #3272732: Drupal 10 & CKEditor 5 readiness works only on Drupal >=10, and not also on Drupal 9.5.

(The tests specifically are skipped on <10: https://git.drupalcode.org/project/entity_embed/-/blame/ac6f2fe13b18f5f55c43badd6f46195776e2351c/tests/src/FunctionalJavascript/CKEditor5IntegrationTest.php#L71-73)

But … it was actually only ever tested against Drupal 10.1 😅 Combined with the fact that Drupal core minors update CKEditor 5 to new major versions, and based on past experience we know that sometimes a CKEditor 5 major release in a new Drupal core minor results in contrib modules needing to re-build their JS and tag a new release, that is then compatible only with that minor.

Steps to reproduce

N/A

Proposed resolution

  1. Automatically run tests on the current minor (10.1) — already the case in HEAD
  2. Automatically run tests on the previous minor (10.0) — this would ensure we know whether we need distinct releases
  3. Automatically run tests on the next minor (10.2) — this would ensure we know about upcoming need for a new release of Entity Embed for that core branch

Remaining tasks

User interface changes

API changes

Data model changes

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:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Reviewed & tested by the community
Issue tags: +ckeditor5

Results:

  1. ❌ Failed on 10.0.x (which uses CKEditor 5 v35.4.0): https://git.drupalcode.org/project/entity_embed/-/jobs/222870
  2. ✅ Passed on 10.1.x (which uses CKEditor 5 v38.0.1 in 10.0.0 and v38.1.0 in 10.1.0 and v39.0.1 in 10.1.3: https://git.drupalcode.org/project/entity_embed/-/jobs/222854
  3. ✅ Passed on 10.2.x (which uses CKEditor 5 v39, and soon v40 — see #3393557: [upstream] Update CKEditor 5 to 40.0.0): https://git.drupalcode.org/project/entity_embed/-/jobs/222855

This that this actually would be a pretty strong reason to start the following branching strategy:

  1. 2.0.x of Entity Embed for Drupal 10.0.x, but recompile for 10.0.x)
  2. 2.1.x for Drupal 10.1.x (straight branch from 8.x-1.x)
  3. 8.x-1.x of Entity Embed for Drupal 9.5.x (revert #3272732: Drupal 10 & CKEditor 5 readiness)
  4. in the future, 2.2.x for Drupal 10.2.x or 10.3.x or …

ℹ️ This is similar to Editor Advanced Link, which has 2.1.x for Drupal 10.0.x and 2.2.x for Drupal 10.1.x — although they managed to make 2.2.x also work on 10.0.x. But that's not always possible.

@Dave Reid, I briefly mentioned the above last Friday, but I'm not sure you fully realized this. I suspect you won't like this 😅 But unfortunately it's out of my hands — it's a policy thing on the CKEditor 5 side.

They are working on doing fewer breaking changes — which is why they didn't go from https://github.com/ckeditor/ckeditor5/releases/tag/v38.0.0 directly to https://github.com/ckeditor/ckeditor5/releases/tag/v39.0.0, but did https://github.com/ckeditor/ckeditor5/releases/tag/v38.1.0 and tried doing https://github.com/ckeditor/ckeditor5/releases/tag/v38.2.0-alpha.1, but eventually never tagged a 38.2.0, and instead had to go to https://github.com/ckeditor/ckeditor5/releases/tag/v39.0.0.

But they have still got some ways on maturing into avoiding breaking changes.

wim leers’s picture

Assigned: Unassigned » dave reid
Status: Reviewed & tested by the community » Needs review

I just wrote "2.0.x for Drupal 10.0.x", but I think it's equally reasonable for Entity Embed to require Drupal 10.1.x, and just say that that is the first and only version to support CKEditor 5.

P.S.: the self-RTBC here is only for the added test jobs. But still, it's failing on 10.0.x, so really … this need discussion with @Dave Reid to determine how he wants to proceed here.

Dave Reid made their first commit to this issue’s fork.

dave reid’s picture

I think I've got it covered by skipping the CKEditor5 test on 10.0, using the new GitLab CI variables for OPT_IN_TEST_PREVIOUS_MAJOR, OPT_IN_TEST_PREVIOUS_MINOR, and OPT_IN_TEST_NEXT_MINOR.

dave reid’s picture

Everything is passing now. Merging.

  • Dave Reid committed f52be693 on 8.x-1.x authored by Wim Leers
    Issue #3396568 by Wim Leers, Dave Reid: Updated GitLab CI workflow with...
dave reid’s picture

Status: Needs review » Fixed
wim leers’s picture

👍 Looking good! 😊

Status: Fixed » Closed (fixed)

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