Problem/Motivation

While working on CKEditor 5, I noticed that I kept getting a CKEditor 5 validation error even after I was no longer on CKEditor 5. I first blamed the CKEditor 5 module.

Then I learned it's a bug in editor.module.

editor_form_filter_admin_format_validate

Steps to reproduce

  1. Install ckeditor.module + ckeditor5.module
  2. Go to /admin/config/content/formats/add
  3. Switch to CKEditor 5
  4. Switch to None
  5. Switch to CKEditor (4)
  6. Expected: CKEditor 4 admin UI
    Actual: CKEditor 5 validation error (Invalid toolbar value.) and no CKEditor 4 admin UI

Proposed resolution

Upon switching text editors (including to '' aka "None"), editor_form_filter_admin_format_validate() gets called before editor_form_filter_format_form_alter() … which means that $form_state still contains the old editor plugin.

Remaining tasks

  1. Passing fix-only patch
  2. Failing test-only patch; test can simply match the steps to reproduce above
  3. Passing fix+test patch

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#13 3230829-13.patch2.29 KBmarcvangend
#13 3230829-13-TEST-ONLY-FAIL.patch1.71 KBmarcvangend
#6 interdiff-3230829-2-6.txt1.51 KBmohit_aghera
#6 3230829-6.patch2.1 KBmohit_aghera
#6 test-only-3230829-6.patch1.51 KBmohit_aghera
#5 Screen Shot 2022-01-10 at 3.04.40 PM.png231.51 KBkristen pol
#5 Screen Shot 2022-01-10 at 3.01.22 PM.png165.38 KBkristen pol
#2 3230829-2.patch596 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 10.0.x-dev » 9.3.x-dev
Status: Active » Needs review
StatusFileSize
new 3230829-2.patch596 bytes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Issue tags: +Needs tests, +Bug Smash Initiative

Triage fairy 🧚‍♂️

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Needs work
StatusFileSize
new Screen Shot 2022-01-10 at 3.01.22 PM.png165.38 KB
new Screen Shot 2022-01-10 at 3.04.40 PM.png231.51 KB

Thanks for the issue and patch.

  1. Tested with and without the patch using the steps to reproduce and have confirmed the patch fixes the issue (see screenshots)
  2. Code change is simple and make sense and addresses just the issue in the issue summary

Moving back to "needs work" for tests.

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new test-only-3230829-6.patch1.51 KB
new 3230829-6.patch2.1 KB
new interdiff-3230829-2-6.txt1.51 KB

Adding test cases and test-only patch.

The last submitted patch, 6: test-only-3230829-6.patch, failed testing. View results

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

Thanks for the fix and the test.

Re #6, maybe I'm missing something, but I don't quite understand why the test was implemented as a FunctionalJavascript test. The issue is about a form submit handler, so I assume it does not depend on the browser. If so, a Functional test (in the \Drupal\Tests\editor\Functional\EditorAdminTest class?) should suffice.

mohit_aghera’s picture

mohit_aghera commented

Thanks for the feedback @marcvangend
The current form seems to be using Ajax and we need to wait till a response comes back.
For that, I needed some method that allows us to wait for ajax call.
Mainly functional Javascript test provides a similar method, so I considered that.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

Sorry Mohit, you are absolutely right. This one is triggered by the select box, not by the save button. FunctionalJavascript is the way to go.

The code looks good to me, just one thought about the documentation. The order in which the different editors are tested (ckeditor > none > ckeditor5) is important to reproduce the error. I would like to see that documented in the test. Otherwise future developers may think it makes sense to re-order some lines and unknowingly break the test. How about something like this?

  /**
   * Tests that editor creation works fine while switching text editor field.
   *
   * The order in which the different editors are selected is significant,
   * because the form state must be changed accordingly.
   * @see https://www.drupal.org/project/drupal/issues/3230829
   */
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Good point @marcvangend. That would be a nice improvement.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Needs work

Moving back to needs work for above feedback.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new 3230829-13-TEST-ONLY-FAIL.patch1.71 KB
new 3230829-13.patch2.29 KB

New patches to address #10.

The last submitted patch, 13: 3230829-13-TEST-ONLY-FAIL.patch, failed testing. View results

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the updated patch. Marking RTBC based on:

1. Issue summary is clear and up-to-date with steps to reproduce.

2. The only changes to latest patch are to the comments in the test.

3. Code was already tested in #5.

4. Test-only patch fails as expected.

5. Patch passes tests.

6. Code has already been reviewed above.

Thanks, everyone!

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Issue summary: View changes

Updated remaining tasks in issue summary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3230829-13.patch, failed testing. View results

mohit_aghera’s picture

mohit_aghera commented

Triggered the test bot again. Current pipeline failures seem different from to the changes we've made.

Update:
FunctionalJavascript tests seems flacky
Discussion thread: https://drupal.slack.com/archives/C1BMUQ9U6/p1644322854924229

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Thanks so much, @mohit_aghera and @marcvangend for adding test coverage! :D 🙏

Re-tested. The failures are definitely unrelated; they indicate infrastructure is broken.

Re-RTBC'ing because it was green a while ago, and nothing has changed in these code paths since then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3230829-13.patch, failed testing. View results

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Reviewed & tested by the community

Marking RTBC again as test failures are unrelated.

  • catch committed a76814b on 10.0.x 
    Issue #3230829 by mohit_aghera, marcvangend, Wim Leers, Kristen Pol:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!

  • catch committed b9bf2bb on 9.4.x 
    Issue #3230829 by mohit_aghera, marcvangend, Wim Leers, Kristen Pol:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay! I just verified that there's nothing to update in core/modules/ckeditor5, and this does fix the scenario described in the issue summary 👍

  • catch committed fef14c8 on 9.3.x 
    Issue #3230829 by mohit_aghera, marcvangend, Wim Leers, Kristen Pol:...

Status: Fixed » Closed (fixed)

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