Switching between editors on the format configuration causes errors upon save

Interesting find! Will look into this as soon as I find the time.

Reproduced.

I was fairly confident that this is a direct consequence of #2502785: Remove support for $form_state->setCached() for GET requests. But turns out it's not.

Now, I don't see why we would even need to use form state in this form. But if I try to remove it, then neither editor_form_filter_admin_format_validate() nor editor_form_filter_admin_format_editor_configure() are called anymore… which makes absolutely zero sense. (See attached patch.)

The reason this problem exists, is the atrocious DX of Form API, particularly when using AJAX.

I literally don't understand. I spent an hour debugging this, and I understand less of Form API now than before I started. I also don't want to spend days debugging every single detail of Form API to figure out what crazy edge case is causing this behavior.

This looks like related, or even maybe duplicate of #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error

Thanks swentel!

Let's get feedback.

Here's the flow as I see it.

1)
Go to /admin/config/content/formats/manage/basic_html
Initially, $form_state->get('editor') === NULL
editor_form_filter_format_form_alter() runs, specifically line 113
Now, $form_state->get('editor') exists, and $form_state->get('editor')->isNew() === FALSE, since it is the editor from config

2)
Switch "Text editor" select to "None"
$form_state->get('editor')->isNew() === FALSE, since it is the editor from config
editor_form_filter_format_form_alter() runs again, also line 113
Initially, $form_state->get('editor') is the entity from config, so isNew() === FALSE
editor_form_filter_admin_format_editor_configure() runs, as it is the submit handler associated with the hidden 'Configure' button, which is set as the #ajax][trigger_as element for the "Text editor" select element
The value in form state for the editor is '', signifying "None", as was selected.
So line 192 runs, $form_state->set('editor', FALSE)

3)
Switch "Text editor" select back to "CKEditor"
editor_form_filter_format_form_alter() runs, but because it checks for === NULL and the value is FALSE, nothing happens there.
editor_form_filter_admin_format_editor_configure() runs, and it moves into the conditional on line 195, which then creates a new Editor entity, for which ->isNew() returns TRUE

4)
Click "Save configuration"
editor_form_filter_admin_format_submit() runs, retrieves the editor, and saves it
The check in EntityStorageBase on 424, if ($id_exists && $entity->isNew()) {, is TRUE
The exception is thrown

To sum up: the form_alter is run every time you do anything.
Because the select element is set to trigger the hidden "Configure" button, it runs on change
And the Configure button's #submit runs even when AJAXified.

This is not a behavioral change from D7 or anything. IMO, the logic around the === '', === NULL, and setting to FALSE is very confusing.

My proposal would be to simplify this check, and always load from storage when there is no entity yet.
I manually tested for both edit and add forms.
Needs automated tests still.

Here's some tests, and a more accurate fix.

Had to switch the #empty_value from '' to '_none' to get Mink to find the option. See also #2448545: Convert '_none' option to a constant, deprecate form_select_options(), deprecate form_get_options() for removal, move form_select_options() to a new class.

Hmm, my patches got mixed up. Slight difference between the do-not-test and the PASS, I prefer the one in do-not-test.
(That patches intended to be one showing the changes without the _none portion, but I guess I got my diffs mixed up)

OMG, thanks for the super fast feedback, and the obviously superb research in #12 that fully got to the bottom of this! ❤️❤️ 👏

I prefer the one in do-not-test.

I do too. Mind reuploading that one?

+++ b/core/modules/editor/tests/src/FunctionalJavascript/EditorFilterFormConfigurationTest.php
@@ -0,0 +1,54 @@
+ * @todo.

Suggested: Tests Text Editor's integration with the Text Format configuration form.

I think this can then be RTBC :)

Added those docs, and filled in others.

Applied the patch from #18 and it worked perfect. Have tested just the behavior/Functionality and NOT checked the code.

👍

Thanks!

+++ b/core/modules/editor/editor.module
@@ -185,14 +186,20 @@ function editor_form_filter_format_form_alter(&$form, FormStateInterface $form_s
+  if (!$editor && !$format->isNew()) {
+    $editor = editor_load($format->id());
+    $form_state->set('editor', $editor);

This could probably use an inline comment explaining why/when it's necessary.

While writing the docs I made a single cleanup to the code.
Also, removed an accidental addition to the test.

Attaching the patch helps

Triggering a fatal through the UI is a major, so promoting to major. This is quite the fun Easter egg for the site builder. :P

I also manually tested to confirm that the WSOD is resolved and that there don't seem to be any other side effects.

This also seems backportable; changing the internal form state/values seems very minor in comparison to the bug it's fixing, and I confirmed that it works fine with no cache clear once I apply the patch, whether the editor was already set to "None" or not.

Committed and pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

This test seemed to have failed randomly here: https://www.drupal.org/pift-ci-job/794008

Yep, this one is a different random fail from https://www.drupal.org/pift-ci-job/794678, also in the added coverage for this issue. So reverted this one too.

#2912399: Extend the CKEditorIntegrationTest for DrupalImage was also reverted now which caused a different failure in a different CKEditor test when committed around the same time.

1. +++ b/core/modules/editor/tests/src/FunctionalJavascript/EditorFilterFormConfigurationTest.php
   @@ -0,0 +1,59 @@
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->elementExists('css', '#ckeditor-button-configuration');
   

   I think instead of waiting for an ajax request to complete we should just wait for the element to existi.

   $this->assertNotEmpty($this->assertSession()->waitForElement('css', '#ckeditor-button-configuration'));

2. +++ b/core/modules/editor/tests/src/FunctionalJavascript/EditorFilterFormConfigurationTest.php
   @@ -0,0 +1,59 @@
   +    $page->pressButton('Save configuration');
   +    $assert_session->pageTextContains('The text format Plain text has been updated.');
   

   This is where the random fail happened.

   I think because there is no wait here after the button is press for the text to appear when the testbot is running too slow it will fail.

#32: since you already did the analysis, would you like to roll a patch? Happy to re-RTBC.

Here is a patch that addresses #32.2 since that is where the test failure actually happened.

I also included 2 patches that run the new test 60 times with and without my suggested changes. This is to see if we can get a random failure.

I am actually less confident in my assessment in #32. I tried to test it with the method I used here #2902191-6: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks which basically to add a usleep(250000); call to index.php which consistently caused the random test to fail but that is not happening with this test.

Maybe I am missing something or maybe the 60x patches will cause the random failure.

Whoops copy/paste error with run-tests.sh in the 60x patches in #34

Today is not my day, did not actually fix 2701393-34_60x_try2.patch
ironically if this happened yesterday I won't have noticed because #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it(🎉) wasn't committed and so my c/p error would have gone unnoticed.

also 2701393-23_60x_try2.patch passed so we didn't prove that the current patch in #23 will cause a random failure. uping to 110

JS tests are now finally less brittle. Several new CKEditor functional JS tests were committed in the past few weeks: #2912399: Extend the CKEditorIntegrationTest for DrupalImage + #2916589: Extend the CKEditorIntegrationTest for DrupalImageCaption. Let's get this going again too!

Re-rolled

I found this bug and accidentally created a duplicate: #3084699: Fatal error when deselecting/reselecting an editor

I'm moving my patch over here. I'll review #45 and compare the approaches.

Here's why I think we should go with my new approach:
1) Less changes. I think we can fix this without adding a new '_none' value, which is better for BC.
2) My test is already converted over to FunctionalJavascript, so it tests Ajax submission. Other than that, it's very similar to the one added by @tim.plunkett. 👍

Thanks, @oknate!

Oh, this needs coding standards fixes. 🤓

This should kill the two coding standard warnings with one stone.

Nice catch! Also GREAT that we have a base now from which to expand test coverage for CKEditor config!

Committed and pushed to 8.8.x. Thanks!

Thanks!

Still encountering this in d10.2.5. Same scenario as originally posted above: Editing basic_html (admin/config/content/formats/manage/basic_html), toggle "Text Editor" from ckeditor5 to none then back again. Hit submit and same error:

Drupal\Core\Entity\EntityStorageException: 'editor' entity with ID 'basic_html' already exists. in Drupal\Core\Entity\EntityStorageBase->doPreSave() (line 519 of /app/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php).