Problem/Motivation

Currently, CKEditor5PluginElementsSubsetInterface extends CKEditor5PluginConfigurableInterface. I think the intention of this is to convey that returning an element subset only makes sense if the plugin is configurable.

However, to me that seems like an aspect of implementation. From the perspective of an interface, I'm not imagining a case where a caller of the interface or something that typehints to the interface would need to rely on anything in CKEditor5PluginConfigurableInterface.

Steps to reproduce

Proposed resolution

I'm leaning towards thinking that the interface shouldn't extend, but I'm open to arguments on why it should.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

effulgentsia created an issue. See original summary.

wim leers’s picture

Status: Active » Postponed (maintainer needs more info)

If it's not configurable, how could it possibly determine what the relevant subset is if the signature is

public function getElementsSubset(): array;

?

Based on what input would it determine what the relevant element subset is? There's no input to the method, so it'd have to be hardcoded. And if it's hardcoded, then … the plugin definition never should have claimed a bigger set of elements is supported if it's hardcoding a subset of those elements?

effulgentsia’s picture

Status: Postponed (maintainer needs more info) » Active

I agree with #2, but that's the perspective of the implementation. An implementor of CKEditor5PluginElementsSubsetInterface should also implement CKEditor5PluginConfigurableInterface. However, the Interface segregation principle is about taking the perspective of the clients of the interface. Do the callers of getElementsSubset() care about what other methods the object needs in order to implement that method in a sensible way?

wim leers’s picture

I see your point.

I don't see the harm in the current approach.

I guess we could remove that extends, but it's just mean nonsensical implementations are possible. That'd be worse for DX I think? I agree with the principle of composable interfaces. But when certain compositions do not make sense, what's the value in supporting them just for purist sake?

IOW: I don't see the value in the proposed approach. Not yet, that is 🤓


Also, what are reasonable clients of this interface (aka callers)? The only sensible client is the CKEditor 5 module itself, isn't it?

bnjmnm’s picture

I'm currently +! with @Wim Leers. I acknowledge the motivation behind wanting to make the interfaces independent, but I think the practical benefits of keeping the extends outweigh the conceptual benefits of removing it.

There's a steep learning curve in developing for CKEditor 5, and I support leveraging any opportunity where a bit of opinionated code guides the developer towards steps that will ultimately be necessary. CKEditor5PluginElementsSubsetInterface extending CKEditor5PluginConfigurableInterface saliently enforces (via errors/IDE) the addition of methods that will inevitably need to be there anyway. Not doing this removes the benefits of immediacy and turns this into something a that adds to the already strained cognitive load of a developer figuring out how to extend CKEditor 5 in Drupal.

There's also plenty of classes extending FormInterface that have empty submitForm methods, and I think that's fairly uncontroversial. I certainly appreciated it when I was learning the poorly documented Drupal 8 API.

I don't want anyone ordering soup to have to explicitly ask for a spoon. It adds inconvenience (and potential frustration) to request an item that is almost certainly necessary. While it's theoretically possible the customer might want to drink from the soup bowl, etc. those customers aren't offended by the presence of the spoon. However, customers being told they needed to request the spoon will almost always think less of the restaurant.

wim leers’s picture

Status: Active » Needs review

I'd almost RTBC this based on @bnjmnm's hilarious and surprisingly fitting metaphors alone 😆

All silliness aside, curious what you think, @effulgentsia! 😊

wim leers’s picture

Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 10.0.x-dev
Component: Code » ckeditor5.module
wim leers’s picture

Version: 10.0.x-dev » 9.3.x-dev
wim leers’s picture

Assigned: Unassigned » effulgentsia

Back to @effulgentsia for review.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs review » Closed (works as designed)

I'm not sure I fully agree with, but I can accept, the premise that if there is no sensible way to implement CKEditor5PluginElementsSubsetInterface without also implementing CKEditor5PluginConfigurableInterface, that there is some conceptual benefit to, and no functional drawback to, having the former extend the latter. Therefore, closing this as works as designed. Thanks for considering it!