Ensure Editor config entities using CKEditor 4 only store plugins settings for actually enabled plugins

If this update is going to be a necessary pre-requisite to updating to ckeditor5, then we will need to do two things:

1. Backport the update to 9.4.x
2. Remove the update from 10.0.x and implement hook_update_last_removed() for it.

We'll then need to special case ckeditor5 in system_requirements() so that people trying to update from 9.4.0 or earlier see a message telling them their version of core is too old. We had to do the same thing for workspaces in 9.0.x

Crosslinking #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps and #3290808: Remove workspaces special casing from system_requirements() and fix versions/docs.

If it's not a necessary prerequisite and just an improvement, we might not need to do the above - but it'd be good to decide either way.

I think given:

This could result in plugins unexpectedly being enabled.

...We do probably need to do this. We should also double-check whether there are other issues affecting the data model.

This should address the failing tests + #8.

This will need a change record, and maybe a release note given that we'll probably be backporting it to 9.4.x when usually this would be a minor-only change.

1. +++ b/core/modules/ckeditor/ckeditor.module
   @@ -122,6 +125,58 @@ function _ckeditor_theme_css($theme = NULL) {
   +    // Enable this plugin if it provides a button that has been enabled.
   ...
   +    // Otherwise enable this plugin if it declares itself as enabled.
   

   Minor nit, but this code isn't actually enabling anything, right? It's just retrieving a list of plugins that have been considered enabled.

2. +++ b/core/modules/ckeditor/ckeditor.post_update.php
   @@ -0,0 +1,41 @@
   +function ckeditor_post_update_omit_settings_for_disabled_plugins(&$sandbox = []) {
   +  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
   +
   +  $callback = function (Editor $editor) {
   

   I'm guessing we're not checking for whether the Editor module is enabled because CKE4 can't be enabled without it?

   What happens if Editor is installed and one tries to delete all the editor config? 🤔 I guess it'd be an empty foreach loop for the update hook, but if it's possible for a site to exist in such a state, the update hook should return early if there are no configured editors.

3. +++ b/core/modules/ckeditor/ckeditor.post_update.php
   @@ -0,0 +1,41 @@
   +  $config_entity_updater->update($sandbox, 'editor', $callback);
   

   I had to look up how this works; ConfigEntityUpdater::update() is some magic. But at least it's pre-existing magic.

4. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -295,6 +295,24 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
   +    // Ensure only plugin settings are saved for plugins that are actually
   +    // enabled.
   

   I think this is meant to say:

   Ensure plugin settings are only saved for plugins that are actually enabled.

5. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -295,6 +295,24 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
   +    $about_to_be_saved_editor = Editor::create([
   +      'editor' => 'ckeditor',
   +      'settings' => [
   +        'toolbar' => $form_state->getValue('toolbar'),
   +        'plugins' => $form_state->getValue('plugins'),
   +      ],
   +    ]);
   +    $enabled_plugins = _ckeditor_get_enabled_plugins($about_to_be_saved_editor);
   +    $plugin_settings = $form_state->getValue('plugins', []);
   +    foreach (array_keys($plugin_settings) as $plugin_id) {
   +      if (!in_array($plugin_id, $enabled_plugins, TRUE)) {
   +        unset($plugin_settings[$plugin_id]);
   +      }
   +    }
   +    $form_state->setValue('plugins', $plugin_settings);
   

   As far as I can see, we're missing explicit test coverage for the runtime behavior here. (The upgrade path is covered.) There's maybe implicit test coverage given the changes to some tests related to stylescombo, but an explicit test would probably be good? It might be just one additional assertion in CKEditorAdminTest plus an line comment.

+++ b/core/modules/ckeditor/ckeditor.post_update.php
@@ -0,0 +1,41 @@
+
+/**
+ * Updates Text Editors using CKEditor 4 to omit settings for disabled plugins.
+ */
+function ckeditor_post_update_omit_settings_for_disabled_plugins(&$sandbox = []) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+
+  $callback = function (Editor $editor) {
+    // Only try to update editors using CKEditor 4.
+    if ($editor->getEditor() !== 'ckeditor') {
+      return FALSE;
+    }
+
+    $enabled_plugins = _ckeditor_get_enabled_plugins($editor);
+
+    // Only update if the editor has plugin settings for disabled plugins.
+    $needs_update = FALSE;
+    $settings = $editor->getSettings();
+    foreach (array_keys($settings['plugins']) as $plugin_id) {
+      if (!in_array($plugin_id, $enabled_plugins, TRUE)) {
+        unset($settings['plugins'][$plugin_id]);
+        $needs_update = TRUE;
+      }
+    }
+    if ($needs_update) {
+      $editor->setSettings($settings);
+    }
+    return $needs_update;
+  };
+
+  $config_entity_updater->update($sandbox, 'editor', $callback);
+}

The actual logic here should happen in a hook_entity_presave() implementation - that way any default configuration in modules or install profiles will get the same treatment.

For an example see ViewsConfigUpdater and views_view_presave() (especially in 9.4.x/9.5.x - a lot has been removed in 10.x).

The update then more or less just loads and saves the config entities.

#12.1: Clarified the comments 👍
#12.2: This indeed leads into an empty foreach loop – I don't think there's anything needed here.
#12.4: Indeed! Updated 👍
#12.5: We already basically have explicit test coverage for this in \Drupal\Tests\ckeditor\Functional\CKEditorAdminTest::testExistingFormat. It tests both; that plugin settings are not saved for disabled plugins and that plugin settings are saved for enabled plugins.

#13: Of course 🤦‍♂️ I have done this before but I haven't done this in years and I had forgotten about this approach already.

+++ b/core/modules/ckeditor/ckeditor.module
@@ -258,3 +259,29 @@ function ckeditor_filter_format_edit_form_validate($form, FormStateInterface $fo
+function ckeditor_editor_presave(EditorInterface $editor) {

Last question here is whether this should be removed in Drupal 10 along with the new update (assuming we do indeed remove it and force people to update to >= 9.4.2 before going to Drupal 10). Or in Drupal 11 on the chance that some bad ckeditor4 default configuration exists in a Drupal 10 module/install profile. This seems a bit more unlikely than other default config like views given everything should shift over to ckeditor5 anyway. But... if it does need to stay into Drupal 11 just in case, we should add a @todo to remove it then.

edit: realised this is a non-issue, because this code is in ckeditor(4) module, which will be removed from core in Drupal 10 anyway, and probably go unsupported end of 2023 when ckeditor4 does - i.e. long before we'd want to tidy up this code.

Fixed custom command fail issue in patch #14.

This actually I think, if we land this before next week's patch window.

CR looks OK too. I added some more info to both it and the release note. Thanks @lauriii!

Interdiff in #14 looks good; however, the test fail in #18 also looks relevant to the issue here:

1) Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testConfig
Exception: editor.editor.basic_html: Drupal\Component\Diff\Engine\DiffOpChange::__set_state(array(
   'type' => 'change',
   'orig' => 
  array (
    0 => '  plugins:',
    1 => '    language:',
    2 => '      language_list: un',
    3 => '    stylescombo:',
    4 => '      styles: \'\'',
  ),
   'closing' => 
  array (
    0 => '  plugins: {  }',
  ),
))

/var/www/html/core/tests/Drupal/KernelTests/AssertConfigTrait.php:39
/var/www/html/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php:79
/var/www/html/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php:56
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

ERRORS!
Tests: 6, Assertions: 48, Errors: 1.

This should address the remaining test failures. Removed a test case that was explicitly testing invalid editor configuration addressed by this issue which is no longer testable at least in a straight forward way.

Fixing trailing whitespace. :)

Went through the entire code and spotted one thing.

+++ b/core/modules/ckeditor/ckeditor.module
@@ -122,6 +125,59 @@ function _ckeditor_theme_css($theme = NULL) {
+    if ($enabled) {
+      $enabled_plugins[] = $plugin_id;
+      // Check if this plugin has dependencies that should be considered
+      // enabled.
+      $additional_plugins = array_merge($additional_plugins, array_diff($plugin->getDependencies($editor), $additional_plugins));
+    }
+  }
+
+  // Add the list of dependent plugins.
+  foreach ($additional_plugins as $plugin_id) {
+    $enabled_plugins[$plugin_id] = $plugin_id;
+  }

On line 167 $enabled_plugins it treated as an indexed array and on 176 it is associative. The latter use is more consistent with the method this was taken from, but the former seems simpler overall and consistent with the docs. It may not even have an impact, but having 176 be $enabled_plugins[] = $plugin_id may spare us future confusion.

***************

I also went through https://git.drupalcode.org/search to see if there were any examples of contrib assuming a CKEditor plugin would have settings regardless of enabled stats, since language and stylescombo currently have config by default. I did not see evidence of this, though I'll acknowledge I may have missed something due to this Gitlab searches having different syntax requirements than the .ru site I'd previously used.

Good catch @bnjmnm! I kept the array as an associative array to remove duplicates from the list. It doesn't matter in practice but it's better that way.

Missed the window today, so updating tag.

Committed/pushed to 10.1.x, 10.0.x and 9.5.x, this needs a 9.4.x-specific patch due to conflicts in ckeditor.module.

Trivial re-roll so setting back to RTBC pending a green test run.

Actually let's get a review so I can commit this if it's OK.

Do we want to commit the post_update to a patch release? I was thinking that it should be committed only to the next minor.

@lauriii if it wasn't blocking the ckeditor5 update then usually yes, but see #3291744-17: Ensure Editor config entities using CKEditor 4 only store plugins settings for actually enabled plugins. If the ckeditor5 update relies on this having run, we need to make sure every site runs it before attempting to update to ckeditor5, which means before they update to 10.0.0 at the latest, which in turn means implementing hook_update_last_removed() in ckeditor module. We want to require that sites update to at least 9.4.x before going to 10.0.0, but not require 9.5.x, which means getting this into a patch release.

edit: also, if all the ckeditor5 blockers are committed to 9.4.x, then ckeditor5 will be effectively stable in 9.4.something, which could see more sites trying to update to it in 9.4.

Makes sense! The reroll in #29 looks good. 👍

Since I only fixed a couple of trivial merge conflicts I think it's OK to commit the backport too, so committed/pushed to 9.4.x thanks!

We should also have a version of the release note snippet for D10 saying sites must update to 9.4.4 prior to updating to 10.0.0.

@xjm I'd assume we'd add that in #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps instead? It's not the case yet.

After upgrading from 9.4.3 to 9.4.4 or 9.4.5 I get the error below. If I (before update) empty the file "web\core\modules\ckeditor\ckeditor.post_update.php" there is no error after update.

The website encountered an unexpected error. Please try again later.

TypeError: Argument 1 passed to Drupal\Core\Plugin\Context\Context::__construct() must implement interface Drupal\Core\Plugin\Context\ContextDefinitionInterface, null given, called in /var/www/my_local_site/web/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php on line 85 in Drupal\Core\Plugin\Context\Context->__construct() (Line 50 in /var/www/my_local_site/web/core/lib/Drupal/Core/Plugin/Context/Context.php)
#0 /var/www/my_local_site/web/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php(85): Drupal\Core\Plugin\Context\Context->__construct()
#1 /var/www/my_local_site/web/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php(116): Drupal\Core\Executable\ExecutablePluginBase->getContext()
#2 /var/www/my_local_site/web/core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php(91): Drupal\Core\Executable\ExecutablePluginBase->getContextValue()
#3 /var/www/my_local_site/web/core/lib/Drupal/Core/Condition/ConditionManager.php(77): Drupal\Core\Entity\Plugin\Condition\EntityBundle->evaluate()
#4 /var/www/my_local_site/web/core/lib/Drupal/Core/Condition/ConditionPluginBase.php(84): Drupal\Core\Condition\ConditionManager->execute()
#5 /var/www/my_local_site/web/modules/contrib/view_mode_page/src/Entity/ViewmodepagePattern.php(411): Drupal\Core\Condition\ConditionPluginBase->execute()
#6 /var/www/my_local_site/web/modules/contrib/view_mode_page/src/PathProcessor/DynamicPathProcessor.php(107): Drupal\view_mode_page\Entity\ViewmodepagePattern->applies()
#7 /var/www/my_local_site/web/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.php(70): Drupal\view_mode_page\PathProcessor\DynamicPathProcessor->processInbound()
#8 /var/www/my_local_site/web/modules/contrib/redirect/src/EventSubscriber/RedirectRequestSubscriber.php(138): Drupal\Core\PathProcessor\PathProcessorManager->processInbound()
#9 [internal function]: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->onKernelRequestCheckRedirect()
#10 /var/www/my_local_site/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func()
#11 /var/www/my_local_site/vendor/symfony/http-kernel/HttpKernel.php(134): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch()
#12 /var/www/my_local_site/vendor/symfony/http-kernel/HttpKernel.php(80): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#13 /var/www/my_local_site/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle()
#14 /var/www/my_local_site/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#15 /var/www/my_local_site/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#16 /var/www/my_local_site/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass()
#17 /var/www/my_local_site/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle()
#18 /var/www/my_local_site/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\ban\BanMiddleware->handle()
#19 /var/www/my_local_site/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#20 /var/www/my_local_site/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#21 /var/www/my_local_site/web/core/lib/Drupal/Core/DrupalKernel.php(709): Stack\StackedHttpKernel->handle()
#22 /var/www/my_local_site/web/index.php(19): Drupal\Core\DrupalKernel->handle()
#23 {main}

I ran into a problem updating core from 9.4.3 to 9.4.5 that I think is related to this.
rushupdb
---------- ----------------- ------------- ---------------------------------
Module Update ID Type Description
---------- ----------------- ------------- ---------------------------------
ckeditor omit_settings_f post-update Updates Text Editors using
or_disabled_plu CKEditor 4 to omit settings for
gins disabled plugins.
---------- ----------------- ------------- ---------------------------------

In ProcessBase.php line 171:

Unable to decode output into JSON: Syntax error

Error: Call to a member function filters() on null in Drupal\ckeditor\Plugin\CKEditorPlugin\DrupalImageCaption->isEnabled() (line 111 of
/home/me
Hoo/public_html/web/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImageCaption.php).

This throws a php error:
Error: Call to a member function getConfigDependencyName() on null in
Drupal\editor\Entity\Editor->calculateDependencies() (line 110 of
/home/me/public_html/web/core/modules/editor/s

How do I fix this?