The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
The bug that #3248177: Language toolbar item cannot be removed from the toolbar uncovered and fixed revealed that some textPartLanguage-specific validation logic should be generalized.
Right now it's possible to have for example heading plugin configuration without having the heading
plugin enabled!
Steps to reproduce
Enable Heading
plugin by activating its button. Configure it. Now remove the Heading
button. No error, but the exported configuration entity still contains the configuration!
Proposed resolution
Generalize textPartLanguage's validation: always trigger this validation error for any plugins that have both toolbar items ($definition->hasToolbarItems() === TRUE) and configuration ($definition->isConfigurable() === TRUE).
Remaining tasks
- Remove
CKEditor5ToolbarItemDependencyConstraintfromckeditor5.plugin.ckeditor5_languageinckeditor5.schema.yml. - Move
CKEditor5ToolbarItemDependencyConstrainttoeditor.settings.ckeditor5'spluginschild key. - Update the logic in
ToolbarItemDependencyConstraintValidatorto iterate over each of the received plugin keys, get the plugin definition, check if it has buttons (otherwise continue), then run the current validation logic.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3259795-6-alt.patch | 6.33 KB | wim leers |
Issue fork drupal-3259795
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:
- 3259795-generalize-textpartlanguages-validation
changes, plain diff MR !1747
Comments
Comment #3
marcvangendI've been looking at the proposed solution and the existing code. Please correct me if I'm wrong, but I think I understand the following.
Basically, this generalized constraint validator will be a lot like EnabledConfigurablePluginsConstraintValidator, albeit the other way around. While EnabledConfigurablePluginsConstraintValidator checks if each enabled configurable plugin ("toolbar item") has corresponding plugin settings, we now want to check if each set of plugin settings corresponds to an enabled configurable plugin. Right?
If the above is correct, wouldn't it make sense to scrap CKEditor5ToolbarItemDependencyConstraint completely and enhance EnabledConfigurablePluginsConstraintValidator to perform the validation in both directions?
Comment #5
marcvangendJust pushed a first attempt at a more generalized constraint validator for plugin settings. In the end I didn't exactly follow the proposed resolution from the issue summary.
I chose not to merge it into
EnabledConfigurablePluginsConstraintValidator, as I briefly considered in #3, because it's probably better to have two separate, more simple constraints.The result is a constraint called
ConfigurablePluginSettingsConstraint. It does for each item in the plugin settings whatToolbarItemDependencyConstraintdid for only the specified one: check if the corresponding plugin is enabled on the toolbar. The code to make this work was mostly inspired byEnabledConfigurablePluginsConstraintValidator.Note that this is just a validator - removing configurable plugins from the toolbar now throws errors at you. If I understand correctly, this is not a new problem, we're just exposing it.
Todo's:
ToolbarItemDependencyConstraint|ToolbarItemDependencyConstraintValidatorComment #6
wim leersThese steps to reproduce no longer work since #3248177: Language toolbar item cannot be removed from the toolbar.
The only way to create such nonsensical configuration (i.e. containing plugin settings for a plugin that is not enabled) is by manually modifying configuration.
That means this is now hardening for a pretty remote edge case. So I think we could change this MR to do this instead: remove
CKEditor5ToolbarItemDependencyConstraintaltogether. Of course, we could continue to do what this MR does to protect against manual config changes. That'd be the safer choice. I'm personally still in favor of doing this MR, I just wanted to lay the options on the table.