Problem/Motivation

In #3393557: [upstream] Update CKEditor 5 to 40.0.0 a change to the ckeditor5.schema.yml file was made which placed a label under mapping which creates an invalid config schema.

This issue was fixed in #3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. as a small sub-change of the larger PR.

The problem is that the bug is introduced in 10.2 and the fix was made in an issue which is only shipped as Drupal 11 and will not be backported. This means that without further intervention the ckeditor5.schema.yml file will be invalid for the rest of the Druapl 10 lifecycle.

Tools such as POTX no longer work for any project that runs it with the 10.x version of the ckeditor5 module.

Steps to reproduce

Proposed resolution

Pick out the moving of the label property to the correct location (above mapping) and backport this as bugfix to 10.2.x and the rest of the 10.x versions.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3418691

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

Kingdutch created an issue. See original summary.

Kingdutch opened merge request !6421

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Active » Needs review

Created a merge request against 10.2.x with the required change.

Credits to ronaldtebrake for debugging this in our codebases and finding the culprit.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Issue summary: View changes
Related issues: +#3393557: [upstream] Update CKEditor 5 to 40.0.0, +#3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc.
dww’s picture

dww
we/he/they
commented
Issue tags: +Bug Smash Initiative

Good find! The change itself is small, looks good, and makes sense.

However, I'm not sure if we need a test for this. Typically we do, but this seems like a bit of a weird case. I'm going to ping some folks in #bugsmash in Slack about it to get more input. But tentative +1 to RTBC'ing this as-is to fix this before any more 10.2.x releases go out.

catch’s picture

catch
he/him
Primary language English
commented

I think it's OK to stop the bleeding without test coverage, but we should open a follow-up to add it so we don't do a similar thing again.

dww’s picture

dww
we/he/they
commented

Great, thanks! Needs followup #3418770: Add automated test coverage that POTX works against all of core

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Oh, and I guess that means I can RTBC this, since I didn't touch the code. Thanks again!

dww credited ronaldtebrake.

dww’s picture

dww
we/he/they
commented

Crediting @ronaldtebrake by request

dww’s picture

dww
we/he/they
commented

Also crediting @kingdutch for the MR and myself for reviews, followup, etc.

  • larowlan committed ea0ae600 on 10.2.x 
    Issue #3418691 by Kingdutch, dww, ronaldtebrake: ckeditor5 module has an...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 10.2.x

larowlan closed merge request !6421

Status: Fixed » Closed (fixed)

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