Plugins should be enableable based on toolbar configuration

YESSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS this has confused and bothered me since day 1 🤪😬

To be clear: this totally makes sense in an MVP sense. But it's definitely something we need to fix at some point. I suspect that with #3209613: Add "Table" plugin having been merged, this has become super obvious 🤓

We basically need to expand the logic in \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getEnabledDefinitions() (and its helper \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::isPluginDisabled()) to match \Drupal\ckeditor\CKEditorPluginManager::getEnabledPluginFiles()'s logic. In CKEditor 4 we had \Drupal\ckeditor\CKEditorPluginButtonsInterface to make this very explicit, but I think that @zrpnr would like to avoid that. Besides, with the metadata now living in *.ckeditor5.yml under a toolbar_items key, that would not even be necessary I think.

Ironically,

Create an API that specifies a plugin is available as a toolbar item. This is because it is possible for there to be plugins that don't have toolbar buttons, so those can't be enabled/disabled via toolbar.

from the issue summary's proposed resolution sounds almost exactly like CKEditorPluginButtonsInterface in Drupal 8|9 core for CKEditor 4.

I am absolutely not intent on reusing the exact same interface composition architecture though. But I am really curious what you had in mind exactly.

In fact, I think that @zrpnr would want a sibling method for ::getEnabledDefinitions() and ::getEnabledLibraries(). So that'd look like the attached patch basically.

Looking forward to thoughts from both of you here! 😊

For

For plugins configured as having toolbar buttons: the html filter should only support the tags specified in that plugin's config IF that plugin is enabled in the text format's CK5 toolbar.

We just need to modify \Drupal\ckeditor5\AdminUi::validateAllowedElements()'s

    $enabled_plugins = array_keys($this->pluginManager->getEnabledDefinitions($submitted_editor));
    $provided = $this->pluginManager->getProvidedElements($enabled_plugins);

… except that already takes into account enabled plugins! So the problem is that ::getEnabledPlugins() considers all plugins enabled by default unless it implements CKEditor5PluginContextualInterface and returns FALSE for its ::isEnabled() implementation.

Tests should start failing now.

OMG d.o you're drunk 😬

Alright, those failures look like they are expected!

Assigning to @zrpnr for review :)

This blocks #3215597: CKEditor 5 plugin settings form available always, even when the plugin is not enabled.

Note that #4 is failing as expected/hoped:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5FormatValidationTest::testHtmlFilterValidation
Behat\Mink\Exception\ResponseTextException: The text "                            " was not found anywhere in the text of the current page.

(Never mind DrupalCI's broken escaping…)

… because instead of requiring all HTML elements of all available CKEditor 5 plugins to be allowed, the change in #4 causes only the HTML to be required that the actually enabled plugins need!

+++ b/src/Plugin/CKEditor5PluginManager.php
@@ -177,7 +192,7 @@ class CKEditor5PluginManager extends DefaultPluginManager {
+    $allowed_toolbar_items = $this->getAllowedToolbarItems($editor_settings['toolbar']['items'] ?? [], $this->getEnabledToolbarItems($editor));

It is more clear to have this method, but there's no change in the result since $definitions already was filtered by the changes in isPluginDisabled which is called by getEnabledDefinitions a few lines up.

+++ b/src/Plugin/CKEditor5PluginManager.php
@@ -418,6 +433,15 @@ class CKEditor5PluginManager extends DefaultPluginManager {
+      return empty(array_intersect(array_keys($toolbar_items_for_plugin), $editor->getSettings()['toolbar']['items']));

so simple! I love this, and it is such a nice way to let non toolbar providing plugins pass but disables any toolbar providing plugins IF they don't have any items in the current toolbar.

I'm really amazed at how little code is needed to make this work, manually tested and it seems to be exactly right. Image is allowed, since it does not provide a toolbar item, but any plugins like bold are removed entirely if their toolbar item is missing. With either bullet or numbered lists the List plugin is correctly enabled.

This will be greatly improved by #3201641: Improve the HTML filter configuration UX where the allowed elements for the html filter get automatically set.

It is more clear to have this method, but there's no change in the result since $definitions already was filtered by the changes in isPluginDisabled which is called by getEnabledDefinitions a few lines up.

Correct, that was very much the intention! That's why I wrote this in #2:

In fact, I think that @zrpnr would want a sibling method for ::getEnabledDefinitions() and ::getEnabledLibraries(). So that'd look like the attached patch basically.

🤓

so simple! I love this, and it is such a nice way to let non toolbar providing plugins pass but disables any toolbar providing plugins IF they don't have any items in the current toolbar.

I think that means you're in favor of this 😁

Soooo … let's push this to completion!

Thanks to this, one problem become apparent: it is impossible to disable the ckeditor5.image plugin. Because right now \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::isPluginDisabled() assumes everything is enabled, unless:

1. they implement CKEditor5PluginContextualInterface and the implementation of ::isEnabled() returns FALSE
2. the CKEditor 5 plugin has >=1 toolbar items and none of them are placed

Currently, only ckeditor5.image hits this edge case: it's the only plugin that does not implement CKEditor5PluginContextualInterface and has no buttons.

The consequence is that the smallest set of HTML that you can enable for a CKEditor 5 instance is:

<p> <br> <img src alt data-entity-uuid data-entity-type>

That's a regression compared to CKEditor 4: it should absolutely be possible to have imageless text formats.

In CKEditor 4, the logic is inverted. Quoting the docs of \Drupal\ckeditor\CKEditorPluginManager::getEnabledPluginFiles():

   * For CKEditor plugins that implement:
   *  - CKEditorPluginButtonsInterface, not CKEditorPluginContextualInterface,
   *     a plugin is enabled if at least one of its buttons is in the toolbar;
   *  - CKEditorPluginContextualInterface, not CKEditorPluginButtonsInterface,
   *     a plugin is enabled if its isEnabled() method returns TRUE
   *  - both of these interfaces, a plugin is enabled if either is the case.

#4 proves we do not actually need a CKEditor 5 equivalent of CKEditorPluginButtonsInterface. Yay! 🥳

But it does mean that we need to figure out a solution for ckeditor5.image. #3195965: Register CKEditor 5 plugins via YAML definitions instead of class annotations split it up in an unconditional ckeditor5.image and a conditional ckeditor5.imageUpload (see https://git.drupalcode.org/project/ckeditor5/-/merge_requests/6/diffs?co...). That was only possible because until this very issue, this module would always require the HTML for all CKEditor 5 plugins, regardless of which ones would be used. Specifically see #3195965-4: Register CKEditor 5 plugins via YAML definitions instead of class annotations.

We can't just conditionally enable ckeditor5.image though, because if there's no PHP code, one cannot implement CKEditor5PluginContextualInterface either. So I see at least 4 solutions:

1. The simplest possible approach here AFAICT is to move the elements annotation from ckeditor5.image to ckeditor5.imageUpload.
2. Merge ckeditor5.image and ckeditor5.imageUpload into a single plugin definition again. This is also still very simple. It would just mean one fewer CKEditor 5 plugin definition defined in YAML.
3. Add class to ckeditor5.image and add a tiny amount of code to implement CKEditor5PluginContextualInterface.
4. Add infrastructure to conditionally enable a particular CKEditor plugin from YAML only. Something like enableIfToolbarItemIsPresent: foobar. This is an API addition though. It feels a bit logic-y. Which is bad for an annotation.

I think option #3 most closely aligns with the current architecture and intent. So went ahead and implemented that. See https://git.drupalcode.org/project/ckeditor5/-/merge_requests/42/diffs?c....

I just advocated against

4. Add infrastructure to conditionally enable a particular CKEditor plugin from YAML only. Something like enableIfToolbarItemIsPresent: foobar. This is an API addition though. It feels a bit logic-y. Which is bad for an annotation.

… but I actually do think this could be a good idea. Because it will actually allow writing less PHP code. Don't want to derail this issue though. So created #3216244: Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation.

Actually, there's no reason this needs to be postponed… 😅

I cannot debug any of the remaining test failures because FunctionalJavascript tests refuse to run on my machine currently. Do not want to redo my development environment just for this issue on a Friday 😬🙈

I see @bnjmnm removed

Postponed on #3201641: Improve the HTML filter configuration UX -- while this isn't fully blocked by this, waiting on that reduces the chances of having to discard work.

from the issue summary in #16.

I agree with that, because without this being committed, #3201641: Improve the HTML filter configuration UX will not actually be able to test the full breadth of what it needs to do. Right now, #3201641: Improve the HTML filter configuration UX will always generate one and the same configuration. But we actually need it to change whenever the filter settings change (that's the critical bug I just found and reported at #3201641-18: Improve the HTML filter configuration UX). Once this issue lands, we will also need it to change whenever the set of toolbar items in CKEditor 5 toolbar changes. It'd be better to tackle all that simultaneously.

So I'd love to land this ASAP.

@lauriii confirmed to me that that last nit of his was the only remaining blocker. Marking RTBC in anticipation of tests coming back green.