CKEditor 5 plugins needing more complex conditions

High-level thoughts

Great point! Those are edge cases we had not yet thought about.

Note that this current limited set of possible conditions was intentional. drupal.conditions is intentionally hardcoded to support only the most common scenarios right now; see #3216244: Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation for the history.

The idea is that it's better to make the common kinds failproof, avoid duplicate-but-subtly-different-and-broken logic (which is a problem we actually had). We can continue adding more kinds of conditions in the future.

Challenge in the proposal

When you require >1 filter or toolbar item that implies AND. But what if for some case it's OR?

Implementation notes

If the challenge is not a concern, and we think we should move forward with AND by simply accepting arrays of strings instead of a single string for both kinds of conditions, then I'm fine with that. What do we need to do for that?

1. We'd need to expand \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemConditionsMetConstraintValidator::validate(), which has explicit test coverage since #3231220: Follow-up for #3216244: AdminUi::validateConditions() does not work, convert to validation constraint.
2. We should also add this to \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition::validateDrupalAspects(), much like we already did recently in #3231327: Plugin definition DX: validate ckeditor5.drupal.elements items and #3228505: Plugin definition DX: automatically check for plugin definitions whether their ::getDefaultSettings() matches the config schema

add new properties, like […]

Exactly!

We could then handle the OR case with yet a different name

Indeed :)

Alternatively, we could overload the existing filter and toolbarItem properties to be either strings or arrays.

That would be more complicated; we can't really express such optional different data shapes in config schema…

Also, if we punt on this, let's add a @todo linking to this issue in ckeditor5.api.php where these properties are documented.

+1

My experience

As another data point: https://www.drupal.org/project/linkit (on whose CKE5 support we already worked: #3232190: CKEditor 5 readiness) has an optional filter, \Drupal\linkit\Plugin\Filter\LinkitFilter.

From everything I've seen in the filter system (which is a lot because I also worked a lot on D7 → D9 migrations) and CKEditor 4 plugins (which I've seen a good share of), I think it is very rare for text editor functionality to depend on multiple filters.

My opinion

That's why I personally would prefer the first option suggested in #3: add more kinds of conditions in the future (filters, filtersOneOf, etc.), and add a @todo linking to this issue from ckeditor5.api.php. Does that mean we should keep this issue open as a meta issue then? Or do we add that @todo in this issue and close this, just point to this as the conversation where this happened? What do you propose?

#3248188: Plugin definition DX: validate drupal.conditions will help prevent WTFs, and will also help identify the need for more conditions.

EDIT: that will do what I wrote in #2:

2. We should also add this to \Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition::validateDrupalAspects(), much like we already did recently in #3231327: Plugin definition DX: validate ckeditor5.drupal.elements items and #3228505: Plugin definition DX: automatically check for plugin definitions whether their ::getDefaultSettings() matches the config schema

Back to @effulgentsia for review.

The one place where the need for >1 filter condition came up so far was #3261061: Allow filter condition to depend on multiple filters, and that actually did end up not needing it. Better yet: it didn't only not need it, there was a better alternative solution: #3261061-3: Allow filter condition to depend on multiple filters.

P.S.: #3248188: Plugin definition DX: validate drupal.conditions landed in the meantime!

That totally works :)

Is the plan to keep this issue open after committing so that people can report their use cases? Just wondering what are we expecting to happen when folks land on this issue.

+1

Wouldn’t that prevent non-privileged users from acting on this since they wouldn’t be able to re-open the issue?

Moving to Needs review to get #17 addressed.

I propose to ask them to create a child issue of this issue. Updated issue summary.

Referencing closed issues in the API docs is generally against coding standards. Also, children of a closed issue are not really discoverable.

Could we instead add a section to the CKE5 developer handbook page with instructions, link to the relevant header (with fragment), and tell them what to do in the handbook section? Then, convert this to a plan issue and leave it Active. That way we can change what the handbook section says after CKEditor 5 is stable and in Standard, to instead tell them to do the standard thing ("Major" and "Contributed project blocker" issue in the core queue), since I assume that we are not going to be checking back to this issue after it's closed for eternity.

Could we instead add a section to the CKE5 developer handbook page with instructions, link to the relevant header (with fragment)

This is impossible to guarantee link reliability for, especially with the fragment. The fragment can be broken by anybody who can modify docs, and drupal.org has been known to break fragments in the past with its own infrastructure changes.

Then, convert this to a plan issue and leave it Active.

… but that would mean that this issue remains open even after the commit?

So, actually … why are we doing all these weird things? For everything else in Drupal core we also expect people to just create new feature requests and use git blame to figure out what the appropriate issues are? So why not do the same here?

So, actually … why are we doing all these weird things? For everything else in Drupal core we also expect people to just create new feature requests and use git blame to figure out what the appropriate issues are? So why not do the same here?

Yep, agreed with this in general and for this issue. However, I still think putting in a link to handbook docs is a good idea. Yes, link fragments get broken, but if we hardcode an ID it's less likely, and URL redirects still exist forever so they will always end up on the right page. It is a good idea to have a developer CTA.

Done. That was a whole ordeal.

I had to create https://www.drupal.org/docs/drupal-apis/ckeditor-5-api to be the sibling of https://www.drupal.org/docs/drupal-apis/ckeditor-api. But I can't get it to be listed in https://www.drupal.org/docs/drupal-apis 🤷‍♂️ Even though everything looks exactly the same.

Fortunately, I was able to create https://www.drupal.org/docs/drupal-apis/ckeditor-5-api/overview (as the successor of https://www.drupal.org/docs/8/api/ckeditor-api/overview) just fine.

There, I added https://www.drupal.org/docs/drupal-apis/ckeditor-5-api/overview#conditions to be able to do what @xjm requested here.

Update title to match the new direction.

I think the IS is also out of date.

I'm not sure why we didn't just leave this open as a feature request or similar, but committed/pushed to 10.1.x and cherry-picked back through to 9.4.x.