CKEditor 5 plugin module dependency not added to text format configuration

Woah, great find! 👌 Thanks for reporting it so very clearly!

Of course I completely agree with the proposed resolution:

Add automatically a dependency on modules providing enabled CKEditor plugins.

I'd love to review a patch that fixes this!

Excellent work @Grimreaper. Patch looks good to me. Waiting for a review from Drupal core committer.

Adding a new method to an interface unfortunately is a BC break: https://www.drupal.org/core/d8-bc-policy#interfaces

Interfaces tagged @api can be safely used as type hints and implemented. No methods will be added, removed or changed in a breaking way.

Could you create a new EditorPluginWithCalculatableDependenciesInterface interface instead? That interface should be marked @deprecated from the start, and should have a @todo Move into \Drupal\editor\Plugin\EditorPluginInterface in Drupal 9.

BTW: the patch looks great! 👌 Well done :)

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -105,6 +105,10 @@ public function calculateDependencies() {
+    // Allow the editor to add its dependencies.
+    $this->addDependencies($this->editorPluginManager()->createInstance($this->editor)->calculateEditorDependencies($this));

This means it's a change in editor.module, not ckeditor.module.

Patch review:

1. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -72,4 +72,37 @@ public function getJSSettings(Editor $editor);
   +   * Similar to
   +   * \Drupal\Component\Plugin\DependentPluginInterface::calculateDependencies(),
   +   * but we need to pass the editor as parameter to know which plugins are
   +   * enabled.
   

   👍

   Just one nit: "pass the editor" -> "pass the text editor config entity"

2. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -72,4 +72,37 @@ public function getJSSettings(Editor $editor);
   +   * determine configuration synchronization order. For example, if the plugin
   

   s/plugin/text editor plugin/

3. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -72,4 +72,37 @@ public function getJSSettings(Editor $editor);
   +   * dependencies listing the specified roles.
   

   s/roles/role config entities/

4. +++ b/core/modules/editor/src/Plugin/EditorPluginInterface.php
   @@ -72,4 +72,37 @@ public function getJSSettings(Editor $editor);
   +   *     'config' => array('user.role.anonymous', 'user.role.authenticated'),
   

   Nit: use short array notation.

+++ b/core/modules/editor/src/Plugin/EditorBase.php
@@ -78,4 +78,11 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
+  public function calculateEditorDependencies(Editor $editor) {
+    return [];
+  }

Hm … on second thought, this actually may make the interface change okay. Because the handful of \Drupal\editor\Plugin\EditorPluginInterface implementations likely all use this base class, which means disruption is super minimal.

So … ignore #8!

Once the nits in #11 are addressed, I'll RTBC this, and we'll see what a core committer has to say.

@Wim Leers - all suggested changes addressed mentioned in comment #11 & also added an interdiff.

This should be reviewed by @alexpott, who is both core committer and maintainer of the configuration API. If he's okay with this, I'm okay with this.

A CR is still needed, but perhaps it makes more sense for Alex to +1 or -1 the direction before creating the CR.

I think the plugin should implement DependentPluginInterface no?

1. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -429,4 +429,27 @@ public function buildContentsCssJSSetting(Editor $editor) {
   +  public function calculateEditorDependencies(Editor $editor) {
   +    $dependencies = [];
   +
   +    // Add the module provider of each enabled CKEditor plugins.
   +    $enabled_plugins = array_keys($this->ckeditorPluginManager->getEnabledPluginFiles($editor));
   +    if (!empty($enabled_plugins)) {
   +      $dependencies['module'] = [];
   +
   +      foreach ($enabled_plugins as $plugin_id) {
   +        $ckeditor_plugin_definition = $this->ckeditorPluginManager->getDefinition($plugin_id);
   +        $provider = $ckeditor_plugin_definition['provider'];
   +        if (!in_array($provider, $dependencies['module'])) {
   +          $dependencies['module'][] = $provider;
   +        }
   +      }
   +    }
   +
   +    return $dependencies;
   +  }
   

   I'm a bit concerned that this method doesn't really have much to do with the CkEditor plugin itself and seems to be reaching out for things it itself has not configured.

2. +++ b/core/modules/editor/src/Entity/Editor.php
   @@ -105,6 +105,10 @@ public function calculateDependencies() {
   +    // Allow the editor to add its dependencies.
   +    $this->addDependencies($this->editorPluginManager()->createInstance($this->editor)->calculateEditorDependencies($this));
   

   This all feels quite convoluted :(

   The approach seems okay but it's an API break. We should add calculateEditorDependencies to a new interface and make CkEditor implement that. And check for the interface in Editor::calculateDependenices. This way other editors won't break and get to opt in to the new functionality.

3. We need tests of this actually working

Thanks for providing clear direction, @alexpott! 🙏

+++ b/core/modules/editor/src/Entity/Editor.php
@@ -101,10 +102,17 @@ public function calculateDependencies() {
+      $this->addDependencies($editor_plugin_instance->calculateEditorDependencies($this));

This is still looking really odd to me. Passing an instance of the configuration entity to the plugin is kinda the wrong direction. Plugins really should have no knowledge of a configuration entity. Looking at the code in CkEditor::calculateDependencies it is hard to reason why this goes there. However the Editor configuration entity is being passed all over the place through the editor and ckeditor plugin system so it's hard to roll that back now.

In an ideal world the Editor configuration entity is used to configure the Editor and other plugins but the plugins don't know anything about the Editor config entity this separation of concerns means that the plugins can stand on their own feet with the config system.

Atm I can't see any other way of doing this without considerable refactoring and API breaks :(

Because this is a problem/oversight in the Editor config entity, the same problem still exists for CKEditor 5 too. Discovered this while responding to #3245720-50: [drupalMedia] Support choosing a view mode for <drupal-media>.

When I started implementing this independently, I ended up doing something very similar to @Grimreaper because AFAICT there's no other way.

This is still looking really odd to me. Passing an instance of the configuration entity to the plugin is kinda the wrong direction. Plugins really should have no knowledge of a configuration entity.

Each text editor config entity has its own configuration: which buttons are enabled, which settings are applied, and so on. So I don't see how this could work any other way? 🤔

In an ideal world the Editor configuration entity is used to configure the Editor and other plugins but the plugins don't know anything about the Editor config entity this separation of concerns means that the plugins can stand on their own feet with the config system.

I don't think this is possible for text editor config entities because the set of plugins to choose from is itself dependent on the text editor plugin being used!

IOW: there's two layers of config dependencies here:

1. \Drupal\editor\Entity\Editor has a \Drupal\editor\Plugin\EditorPluginInterface plugin configured — this is stored in the top-level editor key in the Editor config entity. In core right now this usually means the ckeditor plugin (\Drupal\ckeditor\Plugin\Editor\CKEditor) is configured. In the future, it'll mean that usually the ckeditor5 plugin (\Drupal\ckeditor5\Plugin\Editor\CKEditor5) is configured.
2. Each of those two can have plugins configured for them too: for ckeditor it's @CKEditorPlugin plugins that implement \Drupal\ckeditor\CKEditorPluginInterface; for ckeditor5 it's @CKEditor5Plugin plugins that implement \Drupal\ckeditor5\Plugin\CKEditor5PluginInterface.

   Plugins for one text editor plugin do not work for another.

I don't think anything else (in Drupal core) has this multi-level plugin architecture.

I propose we generalize @Grimreaper's DependentEditorPluginInterface proposal into something that isn't Editor-specific. It's conceivable there are other things that need this.

This is also why this comment that was added in 2014 in #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall is wrong:

    // @todo use EntityWithPluginCollectionInterface so configuration between
    //   config entity and dependency on provider is managed automatically.

The Editor entity does not have a plugin collection directly. It has a single plugin (\Drupal\editor\Plugin\EditorPluginInterface) which may have a plugin collection. (Not every text editor plugin is required to support plugins — a simple one with far less configurability and extensibility would not need it.)

IMHO this is clearly a Major bug.

Postponed the sibling CKEditor 5 issue on this: #3269101-11: [PP-1] Ensure enabled CKEditor 5 plugins remain available by calculating corresponding dependencies.

Related: #3231354: [PP-1] [META] Discuss: merge the Editor config entity into the FilterFormat config entity.

This caused problems once again — this time a detailed report by @gordon at #3335155-8: CKEditor 5 toolbar configuration: no admin UI visible, only JSON in a <textarea>.

CKEditor 4 is gone. But this problem is not.