CKEditor 5 admin UI's AJAX logic and form alterations are too complex and brittle

I was testing this with:

diff --git a/core/modules/editor/editor.module b/core/modules/editor/editor.module
index 2dd2ed9760..ac3708b36f 100644
--- a/core/modules/editor/editor.module
+++ b/core/modules/editor/editor.module
@@ -222,6 +222,9 @@ function editor_form_filter_admin_format_validate($form, FormStateInterface $for
     $editor_plugin->validateConfigurationForm($form['editor']['settings']['subform'], $subform_state);
   }
 
+  $form_state->setErrorByName('filters', 'filters general');
+  $form_state->setErrorByName('filters[filter_html][settings][allowed_html]', 'filters specific');
+
   // This validate handler is not applicable when using the 'Configure' button.
   if ($form_state->getTriggeringElement()['#name'] === 'editor_configure') {
     return;

I think #3219076: When switching from CKEditor 4 to 5, the allowed HTML tags are not yet immediately auto-computed could help simplify this, especially if we make that update $form_state->get('editor') 🤓

#3219076: When switching from CKEditor 4 to 5, the allowed HTML tags are not yet immediately auto-computed was clearly misguided, sorry for the noise.

I was tryint to get to the point where we only run this validation logic if and only if the resulting selected text editor plugin is CKEditor 5, at which point all of this can indeed just be removed.

I can confirm this is still a problem after #3201637: Figure out how to prevent data loss during upgrade/migration path. Patch attached to make testing this simpler.

I think this is the consensus.

Actually, I'm struggling to reproduce this now? 😅

Seems like this is a related @todo:

function ckeditor5_form_filter_admin_format_validate(array $form, FormStateInterface $form_state) {
  // @todo do we need to run after editor_form_filter_admin_format_validate()?

Great, thanks for digging deeper! Marking "needs work" since it's not fully done yet, but well on its way obviously :)

In digging into this, I actually think that we might be able to get rid of most AJAX complexity? 🤞

Alright, going from ccc6fb3b0e0c35338d1190dbb63875461e289ecc yesterday to 496045c0ad088cbfe7b55577686a5c0b2c662aff today, I at least got Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest from

EFEE....

to

..EE....

That's progress.

I still have no idea how c654ec7576cc3ccf7a2db8b74c208474c6a08a3b managed to increase the number of failures though, because nothing is calling it… :O

From 496045c0ad088cbfe7b55577686a5c0b2c662aff to d62cf32ca703f53d706ce1fa3fd1752ee927fad3 makes CKEditor5Test go from

.EEEE.E   

to

..EEE.E 

but made CKEditor5AllowedTagsTest go from

..EE.... 

to

..EEE...

Argh! Making CKEditor5Test::testHeadingsPlugin() pass made CKEditor5AllowedTagsTest::testAllowedTags() fail. At least that's a useful hint!

Paired with @lauriii. He cannot reproduce the failures locally! He also tested with PHP 8.0, but with SQLite instead of MySQL (should not make a difference) and a newer version of Chrome+chromedriver.

I just pushed a commit which adds strict config schema checking only when PHP assertions are enabled; that should help narrow it down. And that is also a net improvement to DX for CKEditor 5 contrib developers.

Looks like that is triggering net new failures! That's great! Even if this does not get us to green, it helps tracking things down then 👍

But … I just realized that the PHP log entries that commit will generate will not show up, because this branch was started before #3228355: PHP warning when saving text format with CKE5: $form_state->getTriggeringElement() unexpectedly returning NULL — we need to merge in all 1.0.x changes before it'll start generating useful output. So … doing that next. 🤞

I'm now 90% certain that this is indeed caused by strict config schema checking only running on DrupalCI somehow. In the past few commits, I added it explicitly to \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validateConfigurationForm() and despite DrupalCI still only showing

RuntimeException: Unable to complete AJAX request.

… I can see that the exact same steps to reproduce are now locally resulting in AJAX responses like

 <em class="placeholder">AssertionError</em>: Schema errors: Array
 (
     [editor.editor.cke5:settings.plugins.ckeditor5_imageUpload] =&gt; missing schema
 )
  in <em class="placeholder">assert()</em> (line <em class="placeholder">482</em> of <em class="placeholder">/Users/wim.leers/Work/d8/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php</em>).

(Fixed that one in 9e56f6712b2aeceb9e9928547c6b2eaae3ca5fb3.)

What's weird is that \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5TestBase::tearDown() which was added in #3228355: PHP warning when saving text format with CKE5: $form_state->getTriggeringElement() unexpectedly returning NULL should be causing this to be visible, but it's … just not. It seems that the tearDown() is perhaps not being called in the case of Functional JS tests failing early? Yet ::tearDown() is supposed to be called always… I could decorate $assert_session->assertWaitOnAjaxRequest(); in CKE5 tests just so we get better error response checking… using the same technique as I added in \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::getLastPreviewRequestTransferSize().

What's weird is that \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5TestBase::tearDown() which was added in #3228355: PHP warning when saving text format with CKE5: $form_state->getTriggeringElement() unexpectedly returning NULL should be causing this to be visible, but it's … just not. It seems that the tearDown() is perhaps not being called in the case of Functional JS tests failing early? Yet ::tearDown() is supposed to be called always…

Actually … I think I figured out why:

00:06:37.487 FATAL Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Test: test runner returned a non-zero error code (2).

This is because \Drupal\FunctionalJavascriptTests\JSWebAssert::assertWaitOnAjaxRequest() triggers a \RuntimeException that the test runner won't handle. So it instantly aborts the test, without running ::tearDown()!

Related core issue: #3061852: [META] Deprecate assertWaitOnAjaxRequest() and make the JsWebAssert::waitFor*() methods behave like real assertions. It's been that way since it was introduced in #2648956: Editing a view leaves old keys hanging, results in invalid schema.

Discovered a core bug while debugging those last few failures: #3230829: editor_form_filter_format_form_alter() does not remove "editor_plugin" from form state when needed.

And @tim.plunkett opened an issue for #21: #3230726: JsWebAssert::assertWaitOnAjaxRequest() should use Behat\Mink\Exception\ExpectationException not RuntimeException!

The last remaining call to AdminUi::getSubmittedEditorAndFilterFormat() was happening in AdminUi::validateConditions(). In trying to figure out how to get rid of that, I noticed … that it never worked. It was lacking test coverage too. So … refactoring it into validation constraints over at #3231220: Follow-up for #3216244: AdminUi::validateConditions() does not work, convert to validation constraint.

That's another good catch prior to opening the core merge request! 🤞

Retitling because this issue was rescoped.

#3218985: CKEditor 5 admin UI's AJAX logic and form alterations are too complex and brittle landed, merged in that commit…

Reviewing

This is still a very tricky bit of code, but it is much easier to follow the flow now, has helpful assert() calls, and it is well documented. Farewell AdminUi!
Merged!