Problem/Motivation

Removing CKEditor5PluginContextualInterface

Quoting #3215506-11: Plugins should be enableable based on toolbar configuration

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 advocated against

  1. 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:

ckeditor5.media
would get
conditions:
  filter: media_embed
ckeditor5.mediaLibrary
would get
conditions:
  filter: media_embed
  toolbarItem: drupalMedia
ckeditor5.image
would get
conditions:
  toolbarItem: uploadImage
  imageUploadStatus: true
ckeditor5.imageUpload
would get
conditions:
  toolbarItem: uploadImage
  imageUploadStatus: true
Consequence
All CKEditor5PluginContextualInterface implementations would disappear!

Removing CKEditor5PluginContextualValidationInterface

Better still, this would allow us to remove CKEditor5PluginContextualValidationInterface as well! The only use for that so far is \Drupal\ckeditor5\Plugin\CKEditor5Plugin\MediaLibrary::validateFilterForm(), which explicitly checks whether the drupalMedia filter is enabled. That requirement/validation logic can easily be inferred from that metadata.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork ckeditor5-3216244

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Title: Remove CKEditor5PluginContextualInterface in favor of annotation » Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation
Issue summary: View changes
Related issues: +#3201641: Improve the HTML filter configuration UX

Better still 🤓

Looks like this could end up helping #3201641: Improve the HTML filter configuration UX too!

wim leers’s picture

Issue summary: View changes
wim leers’s picture

Issue summary: View changes

wim leers’s picture

Status: Active » Needs review
wim leers’s picture

How would this work for potential contrib use cases? What if they need to specify their own conditions that are not defined here?

I'm saying that at least 90% of the validation we'll need is going to be covered by the things in this proposal.

Rather than each reimplementing the same thing many times but slightly differently and slightly broken, this is far more robust.

And the best part: if the need for custom validation reoccurs, we can reintroduce this functionality 🤓😊

wim leers’s picture

This conflicted heavily with:

  1. #3216013: Move validation logic out of form into validation constraints ⇒ use config validation for form, audit command, upgrade path, et cetera
  2. #3201641: Improve the HTML filter configuration UX
  3. #3215506: Plugins should be enableable based on toolbar configuration
  4. #3215597: CKEditor 5 plugin settings form available always, even when the plugin is not enabled

Rebased.

And beautifully, because it is now metadata instead of inexaminable logic, the code in \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::shouldHaveVisiblePluginSettingsForm() added by #3215597: CKEditor 5 plugin settings form available always, even when the plugin is not enabled now has no need for guesswork anymore!

See https://git.drupalcode.org/project/ckeditor5/-/merge_requests/43/diffs?c... for the simplifications I was able to make after I rebased. 😊

End result:

 ckeditor5.ckeditor5.yml                                     | 11 ++++-
 ckeditor5.module                                            | 12 ++---
 src/AdminUi.php                                             | 67 ++++++++++++++++++++++++++++
 src/Annotation/CKEditor5Plugin.php                          |  7 +++
 src/Plugin/CKEditor5Plugin/Essentials.php                   | 23 ----------
 src/Plugin/CKEditor5Plugin/Image.php                        | 25 -----------
 src/Plugin/CKEditor5Plugin/ImageUpload.php                  | 39 +++-------------
 src/Plugin/CKEditor5Plugin/Media.php                        | 13 +-----
 src/Plugin/CKEditor5Plugin/MediaLibrary.php                 | 49 +-------------------
 src/Plugin/CKEditor5PluginContextualInterface.php           | 25 -----------
 src/Plugin/CKEditor5PluginContextualValidationInterface.php | 20 ---------
 src/Plugin/CKEditor5PluginManager.php                       | 79 ++++++++++++++++++++++-----------
 12 files changed, 148 insertions(+), 222 deletions(-)

lauriii made their first commit to this issue’s fork.

lauriii’s picture

Status: Needs review » Fixed

It seems fine to move forward with this. If any plugins ever needed this functionality, it could be brought back. Some generic use cases could be also added as conditions to core, without adding the more advanced functionality to allow handling more complex rules for enabling plugins or validating the editor form.

  • lauriii committed 4c1aa56 on 1.0.x authored by Wim Leers
    Issue #3216244 by Wim Leers: Remove CKEditor5PluginContextualInterface...
wim leers’s picture

🥳

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.