Update Editor + CKEditor 5 module to not use CKEditor 4 in tests

Here's a kickstart for this issue, thanks to a planning meeting during which I was partially idle 🤓

This updates all of CKEditor5AllowedTagsTest's test methods (3 of the 8 affected) to not rely on CKEditor 4.

+++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
@@ -80,31 +82,16 @@ public function testEnablingToVersion5Validation() {
   /**
-   * Tests that when image uploads are enabled in CKEditor 4, they remain in 5.
+   * Tests that when image uploads were enabled, they remain enabled.
    */
   public function testImageUploadsRemainEnabled(): void {

That's funny: this test now fails.

Because the CKEditor 5 module only took the liberty of fixing #3245351: Should switching text editors retain image upload settings? for the CKE 4 → 5 upgrade path, not for switching from some other text editor.

So if we want this to pass, we also need to broaden that fix, but it's still isolated to CKEditor 5, that upstream bug should still be fixed, and fixing it is out of scope here.

Still applies. Re-testing.

Patch no longer applies.

Starting from #6 again, which was the last green test run.

Did not run test suite locally because once again dealing with chromedriver hell. Hopefully it passes 🤞

There are still plenty of remaining tasks listed in the issue summary though.

#21: Hm, I'd swear I've seen that same crash months ago and that it was fixed. Maybe I misremember. A quick search revealed that indeed exactly this was fixed and even has explicit test coverage (see #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers). I wonder what I'm missing. Can you put a breakpoint on that line and check what exact input is causing this crash? 🙏

Semi-good news: getting the current patch back to green is only part of what needs to happen here! See the issue summary for the other tests that need to be updated to not use CKEditor 4 anymore 🤓 So while we figure out what's going on in #21, we can already proceed with the other parts. 👍

This is now hard-blocking #3304326: Deprecate CKEditor 4 module in 9.5.

Wow, you found a "sub edge case" that we missed in #3274648! 👏 😅 Nice detective work & great comment in #24!

#25 looks great! But now we need the other remaining tasks to still be addressed — any chance you could take that on? 🤞

Here's a review in the meantime:

1. +++ b/core/modules/ckeditor5/src/SmartDefaultSettings.php
   @@ -124,11 +124,14 @@ public function computeSmartDefaultSettings(?EditorInterface $text_editor, Filte
   +    // @todo Remove in https://www.drupal.org/project/drupal/issues/3245351
   +    if ($old_editor) {
   +      $editor->setImageUploadSettings($old_editor->getImageUploadSettings());
   +    }
        if ($old_editor && $old_editor->getEditor() === 'ckeditor') {
          $enabled_cke4_plugins = $this->getEnabledCkeditor4Plugins($old_editor);
          [$upgraded_settings, $messages] = $this->createSettingsFromCKEditor4($old_editor->getSettings(), $enabled_cke4_plugins, HTMLRestrictions::fromTextFormat($old_editor->getFilterFormat()));
          $editor->setSettings($upgraded_settings);
   -      $editor->setImageUploadSettings($old_editor->getImageUploadSettings());
   

   The point of this was to retain image upload settings, regardless of which text editor is the original one: CKEditor 4 or any other.

   (#3245351: Should switching text editors retain image upload settings? will fix this in the Text Editor module.)

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -120,14 +107,15 @@ public function testImageUploadsRemainEnabled(): void {
   -    // Assert that image uploads are enabled initially.
   -    $this->drupalGet('admin/config/content/formats/manage/cke4_image_uploads');
   -    $this->assertTrue($page->hasCheckedField('Enable image uploads'));
   

   It makes sense that we cannot do this: the Text Editor plugin itself is responsible for exposing image upload settings in some way. But because we switch from the CKEditor 4 text editor to the Unicorn text editor, there is no form element anymore to enable/disable image uploads.

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php
   @@ -120,14 +107,15 @@ public function testImageUploadsRemainEnabled(): void {
   +    // Enable the image upload toolbar item.
   +    $this->triggerKeyUp('.ckeditor5-toolbar-item-uploadImage', 'ArrowDown');
   +    $assert_session->assertWaitOnAjaxRequest();
   

   … which brings me to my point: we should NOT need this! The first hunk I commented on should make this happen automatically when switching to CKEditor 5.

   But I think the fact that there is no form element is what is causing all this weirdness!

   P.S.: I wrote this 🙈

… I think all of this could be solved (read: omitted from the patch) by adding this:

diff --git a/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php b/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php
index 0317e12adf..f6a35434a4 100644
--- a/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php
+++ b/core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php
@@ -39,9 +39,25 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
       '#type' => 'checkbox',
       '#default_value' => TRUE,
     ];
+    $form_state->loadInclude('editor', 'admin.inc');
+    $form['image_upload'] = editor_image_upload_settings_form($form_state->get('editor'));
+    $form['image_upload']['#element_validate'][] = [$this, 'validateImageUploadSettings'];
     return $form;
   }
 
+  /**
+   * #element_validate handler for "image_upload" in buildConfigurationForm().
+   *
+   * Moves the text editor's image upload settings into $editor->image_upload.
+   *
+   * @see editor_image_upload_settings_form()
+   */
+  public function validateImageUploadSettings(array $element, FormStateInterface $form_state) {
+    $settings = &$form_state->getValue(['editor', 'settings', 'image_upload']);
+    $form_state->get('editor')->setImageUploadSettings($settings);
+    $form_state->unsetValue(['editor', 'settings', 'image_upload']);
+  }
+
   /**
    * {@inheritdoc}
    */

Functional JS tests are once again not working because of Chrome + chromedriver shenanigans. No time to mess with that tonight. Here's hoping I get EditorAdminTest changes right the first time 🤓

Been trying all day to get to this, but it's now past 19:00 and my day has been sunk into #3222756: Allow using images from external source — assigning this to myself for tomorrow 🤓

#37: That problem is solved in #3304736-14: Provide a good UX to ensure the CKEditor 4 to 5 update is always done before upgrading to Drupal 10 unless the contrib module is installed now! 👍🤓

#38: 🤣👏

Yay, that got us past it! So this should be a viable path forward with far less code! 🤞

But now the failures are in a different spot:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "ckeditor" plugin does not exist. Valid plugin IDs for Drupal\editor\Plugin\EditorManager are: ckeditor5

/var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:53
/var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php:25
/var/www/html/core/lib/Drupal/Core/Plugin/Factory/ContainerFactory.php:16
/var/www/html/core/lib/Drupal/Component/Plugin/PluginManagerBase.php:83
/var/www/html/core/modules/editor/src/Entity/Editor.php:121
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:320
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:253
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:608
/var/www/html/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php:91
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Line 121 of Editor.php is in here:

  public function calculateDependencies() {
    parent::calculateDependencies();
    // Create a dependency on the associated FilterFormat.
    $this->addDependency('config', $this->getFilterFormat()->getConfigDependencyName());
    // @todo use EntityWithPluginCollectionInterface so configuration between
    //   config entity and dependency on provider is managed automatically.
    $definition = $this->editorPluginManager()->createInstance($this->editor)->getPluginDefinition();
    $this->addDependency('module', $definition['provider']);
    return $this;
  }

… we're literally just crashing now on RE-calculating dependencies for Text Editor config entities that should not be recalculated because we're talking about a test fixture here!

And that's only because the test is doing things like:

    Editor::create(
      Yaml::parseFile('core/profiles/standard/config/install/editor.editor.basic_html.yml')
    )->save();

… even though that really should be created that way. Let me figure out the optimal way to install configuration! 🤓

Yay, got past that too, next:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for editor.editor.full_html with the following errors: editor.editor.full_html:settings missing schema

/var/www/html/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:94
/var/www/html/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:108
/var/www/html/core/lib/Drupal/Core/Config/Config.php:229
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:274
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:486
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:253
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:608
/var/www/html/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php:91
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

https://git.drupalcode.org/project/drupal/-/merge_requests/2665/diffs?co... made that green 👍

Reviewed. This is almost RTBC. 3 questions remain 😊

So my 3 remaining concerns were all about test coverage: I want to make sure we do not lose test coverage.

Looks like I trolled myself, because all 3 were introduced by myself in #5 🙈

✅ testImageUploadsRemainEnabled(): the // Enable the image upload toolbar item. addition

Confirmed that this is necessary and fine, but it's unclear why. It does NOT enable image uploads: it triggers the image upload settings form to become visible, to allow the image upload status to be checked. We should add a comment.

❌ testSwitchToVersion5(): the $this->assertSession()->pageTextContains('The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Image (<img src alt data-entity-uuid data-entity-type>)'); removal

In #5, I modified \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::$defaultElementsWhenUpdatingNotCkeditor5. Previously, it contained markup typical for CKEditor 4, i.e. including <img src alt data-entity-type data-entity-uuid>. #5 removed that. Why? Because to test the behavior of switching to CKEditor 5, I created a fresh text format + editor … that used CKEditor 4. The DrupalImage button is placed in the CKEditor 4 toolbar by default (see \Drupal\ckeditor\Plugin\Editor\CKEditor::getDefaultSettings(). This explains the presence of the <img> tag.

Switching to the Unicorn editor does not trigger that anymore.

Now, that's fine, except that this causes the The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Image (<img src alt data-entity-uuid data-entity-type>) message to no longer appear, and that's something that is explicitly asserted in HEAD.

Apparently April-Wim thought that it was fine to lose this test coverage. August-Wim is not so sure!

So I dug into this and … found that \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::testAllowedTags() explicitly tests this in the UI! 👍 (And it's tested at the unit level in many scenarios in \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::testPair().) That means it's fine to simplify testSwitchToVersion5() and remove that assertion, because as the name of the test method indicates: that's not the intent of that particular test; it was actually just a distraction in that test 👍

❌ testSwitchToVersion5(): the // Return to the confirm form and enable the image upload toolbar item. addition

Much like the above, turns out this was only necessary because of the DrupalImage button being in the CKEditor 4 toolbar by default: the original test was also distracted with the fact that CKEditor 4 could be saved with image uploads being disabled, but CKEditor 5 does not allow that (at least not in HEAD, it will soon: #3222756: Allow using images from external source). So the proper fix is to just delete all that completely, resulting in a much easier to understand test! 🥳

That was green!

No more remarks. Let's do this! 🚢

(Yes, I wrote the very beginning of this patch. But then @longwave took it to the finish line. And as #45 shows, I've very strongly disagreed with my own initial patch in #5 🙈😅 All I did here was simplify clean up test fixtures — see #39 — and thoroughly verify that we do not lose any test coverage — see #45.)

@lauriii pointed out in chat that \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5CKEditor4Compatibility should be moved into core/modules/ckeditor too, otherwise we'd still have a test relying on CKE4 after removing core/modules/ckeditor in #3270438: Remove CKEditor 4 from core.

Trivial change (pushed it already), so keeping at RTBC.

Yay, this unblocked #3304326: Deprecate CKEditor 4 module in 9.5! 🎉

We should still get this committed to 9.4.x.

The difference here is that we should not update the editor module's tests in 9.4.x. Only the ckeditor5 module. And because #3271097: Replace CKEditor 4 with CKEditor 5 in the Standard profile and StandardTest obviously won't be backported to 9.4.x either, we need to change the tests slightly.

I need at least the core/modules/editor/tests/modules/src/Plugin/Editor/UnicornEditor.php hunk from the 9.5.x commit if this wants to be able to pass tests.

Green! 👍

Thanks!

Restoring previous issue state.