Problem/Motivation

When editing a Text Format, I can't change the text editor option from "CKEditor" to "None".

Steps to reproduce:

Start with a base install of D8 dev or D8 beta 2
Log in as administrator
Go to Admin -> Configuration -> Content Authoring -> Text Formats and Editors
Edit a text format that has CKEditor selected. "Full HTML" is a good example.
Change text editor to "None"
Note that some of the CKEditor options still appear on the page.
Save the form.
On the Text Format list, the format you just edited will still have "CKEditor" shown as its editor.
I couldn't find any way to change to "None" without creating a new text format.

Note: Changing from "None" to "CKEditor" does work as expected. It's just impossible to change back to "None."

System specs: PHP 5.5.14, Apache 2.2.26, MySQL 5.6.17 running on OSX 10.9.5. Observed in both Chrome 38 and Firefox 33.

Proposed resolution

When choosing "None" for text editor, the CKEditor config options should disappear.
When saving the form, it should set the text editor to "None" if that's what I chose.

CommentFileSizeAuthor
#13 2372893-13.patch2.91 KBidebr
#13 2372893-13.fail_.patch1.71 KBidebr
#13 interdiff-13-9.txt1.86 KBidebr
#9 2372893-9.patch3.13 KBidebr
#9 interdiff-9-1.txt1.93 KBidebr
#9 2372893-9.fail_.patch1.93 KBidebr
#1 2372893-1.patch1.29 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Text format's editor cannot be changed from "CKEditor" to "None" » Due to AJAX/form system change, now unable to change the text editor in the UI
Status: Active » Needs review
Issue tags: -ckeditor
FileSize
2372893-1.patch1.29 KB

Reproduced. You're right. This is a regression.

You can see this even more clearly at admin/config/content/formats/manage/restricted_html. In the "Text editor" <select>, choose "CKEditor". The settings are loaded via AJAX. Then select "None" again. The settings remain.

Something must've changed in the AJAX or form system, because this was definitely working before.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Priority: Normal » Major

An inability to change the text editor is probably at least major.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »
Issue tags: +SprintWeekend2015
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: » Unassigned
Status: Needs review » Reviewed & tested by the community

The issue was introduced in #2316533: Add getValue/setValue/hasValue and isValueEmpty to FormState where an isset() was removed while doing some allround refactoring. Since getValue is used now, a check against null is better. That is exactly what the patch does.

After applying the patch, the issue is resolved. Marking this rtbc.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Is it possible to write a test for this? Ajax stuff is always a bit tricky, especially if there is no js fallback button.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

There is a no-js fallback button.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Ha, well, then there is no excuse for not having tests? ;)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Indeed.

idebr’s picture

idebr CreditAttribution: idebr commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests +Regression
FileSize
2372893-9.fail_.patch1.93 KB
interdiff-9-1.txt1.93 KB
2372893-9.patch3.13 KB

I was able to reproduce the issue reported by kdechant and I can confirm the patch by Wim Leers solves it. To make sure it doesn't break again in the future I added a test.

The last submitted patch, 9: 2372893-9.fail_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

The test as proposed in #9 seems to test the issue properly, however it is run on every invocation of selectUnicornEditor() and thus multiple times. I'd say that's unneeded and it also doesn't do any good for the performance of these tests. Wouldn't it be better to move it into a separate test method?

idebr’s picture

idebr CreditAttribution: idebr commented

Each test method triggers a rebuild of the test environment, so moving it to a separate method would not likely be beneficial for performance of the test. How about moving it to an existing test (testAddEditorToExistingFormat / testAddEditorToNewFormat)?

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs work » Needs review
FileSize
interdiff-13-9.txt1.86 KB
2372893-13.fail_.patch1.71 KB
2372893-13.patch2.91 KB

I moved the assertion to EditorAdminTest::testAddEditorToExistingFormat() so it only gets called once.

Status: Needs review » Needs work

The last submitted patch, 13: 2372893-13.patch, failed testing.

The last submitted patch, 13: 2372893-13.fail_.patch, failed testing.

Status: Needs work » Needs review

pjonckiere queued 13: 2372893-13.patch for re-testing.

pjonckiere queued 13: 2372893-13.fail_.patch for re-testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

You are right on the performance, it's not really relevant in this issue. And I think it's a good idea to move it to a specific method like you did in #13.

The patch in itself is still fine. I'm not sure why there are test failures, so I queued the patches for retesting.

The last submitted patch, 13: 2372893-13.fail_.patch, failed testing.

idebr’s picture

idebr CreditAttribution: idebr commented

HEAD was broken last night: http://cgit.drupalcode.org/drupal/commit/?id=a6d172fe212f2f334e10530c7b3...

The test passes now as expected.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

Yes, they do. I think this is good to go in now.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Title: Due to AJAX/form system change, now unable to change the text editor in the UI » Unable to change the text editor in the UI

We don't usually put any vague information about what caused the bug in the issue title :)

idebr’s picture

idebr CreditAttribution: idebr commented
Title: Unable to change the text editor in the UI » Text editor configuration UI does not update correctly when switching editors

Technically you can still change editors, its just the configuration panel does not update correctly.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +Quickfix

RTBC+1, thanks for writing a test for this!

Just one typo, that can be fixed upon commit:

+++ b/core/modules/editor/src/Tests/EditorAdminTest.php
@@ -82,6 +82,14 @@ public function testAddEditorToExistingFormat() {
+    $this->assertTrue(count($unicorn_setting) === 0, "Unicorn Editor's settings form are no longer present.");

s/are/is/

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Component: ckeditor.module » editor.module
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed c0b346b and pushed to 8.0.x. Thanks!

Fixed the nit in #24 on commit.

  • alexpott committed c0b346b on 8.0.x 
    Issue #2372893 by idebr, Wim Leers: Text editor configuration UI does...

Status: Fixed » Closed (fixed)

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