Problem/Motivation
It was added in #3216244: Remove CKEditor5PluginContextualInterface and CKEditor5PluginContextualValidationInterface in favor of annotation, days after the validation constraint refactor landed. It added form-level validation.
But it only worked at the time thanks to a bug: $plugins = $this->pluginManager->getEnabledDefinitions($submitted_editor); did not actually return all enabled definitions, but …
It never worked, because it starts from
$plugins = $this->pluginManager->getEnabledDefinitions($submitted_editor);
… and then it goes on to check the reasons why that plugin was NOT enabled. But … it already is … and so … it never showed a validation error!
We just never noticed because this was introduced in a turbulent time: lots of changes were in flight, and progress was more important >2 months ago. Crucially, there was no test coverage 🤔😨
Steps to reproduce
The reason it makes sense for this be a form-level validation constraint only is because it actually was designed to ensure that all items that the user put in the toolbar will actually work based on their conditions.
- For example: placing the image button required the image upload status to be enabled, otherwise there's no point to having that button. But it would still result in a functional CKEditor 5 instance: it just won't have the image button there.
- For example: placing the media library button required the media filter to be enabled, otherwise there's no point to having that button. But it would still result in a functional CKEditor 5 instance: it just won't have the media library button there.
The problem with both is that the admin UI shows something different than the resulting CKEditor 5 instance.
Proposed resolution
Convert these to a validation constraint that validates conditions for plugins loaded by active toolbar items: if the toolbar item is active, it SHOULD show up.
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork ckeditor5-3231220
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
Comment #3
wim leersIt's green. It's adding test coverage. It found a flaw/oversight in our existing functional tests. It's fixing something we introduced months ago.
@bnjmnm is the other person who's been reviewing the validation constraints. But because we'd like to open the core merge request ASAP — preferably today — I'm going to go ahead and self-RTBC and merge this request.
Comment #5
wim leers