Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Currently, Drupal\ckeditor\Plugin\Editor\CKEditor::getJSSettings() uses a += operator to combine the configuration arrays provided by plugins. In the event there are identical keys, the first instance is honored, not the last. Drupal's Internal plugin, which loads first, sets some settings by default:
/**
* {@inheritdoc}
*/
public function getConfig(Editor $editor) {
// Reasonable defaults that provide expected basic behavior.
$config = [
// Don't load CKEditor's config.js file.
'customConfig' => '',
'pasteFromWordPromptCleanup' => TRUE,
'resize_dir' => 'vertical',
'justifyClasses' => ['text-align-left', 'text-align-center', 'text-align-right', 'text-align-justify'],
'entities' => FALSE,
'disableNativeSpellChecker' => FALSE,
];
...
return $config;
}
This means that another plugin cannot override these settings without implementing hook_editor_js_settings_alter().
This is being filed as a bug, because usinghook_editor_js_settings_alter() is a workaround. Per Drupal\ckeditor\Plugin\CKEditorPlugin\Internal::getConfig():
// Reasonable defaults that provide expected basic behavior.
The language shows a clear intent to provide defaults, not to hard-code these values.
Proposed resolution
Use array_merge() to combine the arrays instead.
Remaining tasks
Submit patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
In versions of Drupal prior to 8.8.0, it was not possible to override CKEditor default configuration provided by the Internal plugin. In the event multiple plugins provided configuration with the same key, the first instance was honored. Beginning in Drupal 8.8.0, the last instance is now honored.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | core-ckeditor_allow_default_config_override-3052938-2.patch | 769 bytes | Chris Burge |
Comments
Comment #2
Chris Burge CreditAttribution: Chris Burge at University of Nebraska commentedPatch attached.
Comment #3
Chris Burge CreditAttribution: Chris Burge commentedComment #5
Chris Burge CreditAttribution: Chris Burge commentedTests still pass on 8.9. Leaving status at Needs Review.
Comment #6
TwoDI do not actually agree with this approach for two reasons.
1, I'm not sure we can handle merging of settings correctly as there may be a need for for deep merges for complex cases. Unless only a single plugin wants to completely override an editor setting there's no generally "correct" way of merging them. Plugins usually only include settings which they have defined themselves.
2, The CKEditor custom config module could instead of [or in addition to] implementing its functionality as a CKEditorPlugin plugin, it could implement
hook_editor_js_settings_alter()and modify any part of the settings tree.It's not adding any buttons or actual CKEditor plugin as such so it seems to only use the plugin to inject itself into the settings form, which could also be accomplished with a form alteration (yes, storing the settings would be more complicated, so perhaps keep the plugin for that reason).
Comment #7
Chris Burge CreditAttribution: Chris Burge commentedThis issue does not seek to merge settings.
The patch simply allows for settings for
pasteFromWordPromptCleanup,justifyClasses, etc. to be modified like all other settings. Core provides certain "defaults" that cannot be modified. This patch corrects that.Agreed; however, that's not relevant to this issue. The patch simply allows
pasteFromWordPromptCleanup,justifyClasses, etc. to be modified like all other settings. A module may desire to override settings or to modify/merge settings. That makes no difference here, however.Usually, but not necessarily. By design, CKEditor config modifies all settings. It's a utility module that allows privileged users to manage CKEditor settings via a UI.
Modifying granular parts of the settings tree is out of scope for CKEditor config. See #3062284-12: Move CKEditor Custom Config to a JSON approach...: "The second use case is merging configuration objects/arrays. That's outside the scope of this module as that is best accomplished programmatically (because we don't know how to resolve merge conflicts - it may vary from usage to usage)."
In conclusion, Merging settings is out of scope and is not suggested by this issue. Modifying any part of the settings tree is out of scope and is not suggested by this issue.
The clear intent from the existing code in core's CKEditor module is that the "default" settings are provided are defaults, not absolutes. The scope and intent of this issue to allow them to be modified like all other settings. That's it.
Comment #8
TwoDThe patch does open up for that possibility and potential side-effects
array_merge()would introduce.The description for
::getConfig()statesReplacing an existing setting is an override, not an addition. So the
+operator works as intended, allowing you to make additions to the default settings, allowing the to be modified/alter at a later stage (as in the alter hook).The method is not intended to modify existing config, only extend it. The core defaults can be modified in
hook_editor_js_settings_alter().I meant native CKEditor plugins here, not Drupal modules. Drupal modules wrapping CKEditor plugins should likewise not touch settings for other plugins than those it implements, that can lead to all kinds of confusion. The CKEditor custom config module does not wrap any CKEditor plugin, and would be more suited to implement
hook_editor_js_settings_alter()to add or override any editor/plugin with its custom config.Yeah, I get that, but I'm also trying to tell you that implementing
Drupal\ckeditor\CKEditorPluginInterfaceis not the expected way to modify or override any setting at all,hook_editor_js_settings_alter()is that way.Good, agreed. Then let's not use
array_merge()on the return value of::getConfig()and save us from a lot of future headaches. ;)Is
hook_editor_js_settings_alter()insufficient for this purpose?Comment #9
Chris Burge CreditAttribution: Chris Burge commentedI've dug through the issue queue and Git history.
In the case of CKEditor config, I agree that implementing settings overrides with
hook_editor_js_settings_alter()is preferable to implementing them with::getConfig(). Primarily, we get settings after all other plugins have added their settings. The current method predates my co-maintainership with the CKEditor config module. Accordingly, I've opened an issue: #3090970: Implement settings with hook_editor_js_settings_alter()That said, these changes to the CKEditor config module are tangential to this issue.
The central issue remains - CKEditor [Drupal] plugins are unable to set certain settings without implementing
hook_editor_js_settings_alter().Example: A developer writes a CKEditor Justify module that allows site builders to set the classes added to justified elements, which requires the
justifyClassessetting. The correct method would be to set it with::getConfig()in the plugin; however, because 1) the internal plugin always loads first and 2)+=is used instead ofarray_merge()- this is impossible because the first instance of a key is used instead of the last.As such, I'm leaving this issue open.
Comment #10
TwoDThat is by design. Plugins provide defaults, alter hooks do alterations as needed.
The intended use case is; unless you are actually presenting a completely separate native CKEditor plugin to the Drupal CKEditor module, you should not implement
Drupal\ckeditor\CKEditorPluginInterfaceor use the@CKEditorPlugin()annotation. CKEditor plugins are not meant to mess with each other's settings in this way, and if they need to do so, they can use the CKEditor JS API directly. I don't see that module using a CKEditor plugin and thus should not need a Drupal plugin. (As mentioned earlier, I do agree it's a bit easier to present the settings form that way vs a separate form alter hook, just don't return anything from::getConfig().)1) When CKEditor is configured in standalone mode its plugins provide default settings which you can override for your use case when passing the settings object to a new instance. In Drupal the
CKEditorPluginshould present overrides of those settings which makes it work better with Drupal in the general case. TheInternalplugin does this for the plugins bundles with the CKEditor build used by core, including thejustifyplugin. Plugins are not supposed to be able to override each other's settings this way, doing so could cause conflicts you can not solve without also implementing a later alter hook and overriding conflicting values anyway.2) Then you are free to implement
hook_editor_js_settings_alter()as the primary method of overriding any of the defaults - perhaps using a helper module like CKEditor custom config or in a custom per-site module. Here you can do whatever you want withjustifyClassesor any other setting as very few modules should need to use this hook (at least in conflicting ways).Comment #11
Chris Burge CreditAttribution: Chris Burge commentedMost of #6 through #10 is off-topic (although, thank you for your suggestion to improve the architecture of the CKEditor config module). This issue is about the order in which CKEditor Drupal plugin settings are honored.
> unless you are actually presenting a completely separate native CKEditor plugin to the Drupal CKEditor module, you should not implement Drupal\ckeditor\CKEditorPluginInterface or use the @CKEditorPlugin() annotation.
Feel free to open a new issue to address this assertion. I can't find anything (documentation, code comments, comments in issues, Git commit messages) supporting this assertion.
Returning to the scope of this issue:
I have seen no evidence, either here or elsewhere, that the default settings provided by the Internal plugin should be within the absolute domain of the Internal plugin within Drupal's plugin architecture. Again, this issue is about the order in which CKEditor Drupal plugin settings are honored. Feel free to open new issues (and link back) for issues tangential to this.
Comment #12
TwoDWhat I've said so far is very relevant because the CKEditor custom config module is trying to coerce an API into doing something it's not meant to do while there is already a more suited separate API. I'm trying to explain to you why the plugin API is (mostly) the wrong choice for CKEditor custom config module and that the alter hook would perfectly fit what it's trying to accomplish, without any Core changes being necessary.
Invocation order is not honored because it should be irrelevant. What is returned from
::getConfig()should be unique to a single plugin because we have no way of correctly resolving any potentially conflicting defaults. If each plugin sticks to only setting defaults for itself, the outcome is deterministic, reliable and a source of any values can be easily traced.I've already quoted part of the method description from the interface method you are implementing; Here it is again with the relevant part highlighted:
The description of the interface
Drupal\ckeditor\CKEditorPluginInterfaceitself also makes a point about it being centered around CKEditor plugins:The CKEditor custom config module does not use a plugin, which are isolated pieces of functionality unless they have explicit dependencies on some other plugins. Even in that case they avoid changing defaults as those are normally defined inside the plugins itself. In the case of Drupal plugins wrapping CKEditor plugins, we provide a convenience method for making common tweaks to those internal defaults to better work with Drupal instead of having to always implement the alter hook. The method can do that precisely because the alter hook always has the last say in what gets passed on to the editor.
The CKEditor custom config module implements a plugin wrapper for a plugin that does not exist just to be able to override the default settings for other plugins, something which there is already a specific and supported hook for, which runs after all plugins have been processed.
Opening up for merging of default settings would lead to plugins stepping on each others toes because the order is by design irrelevant. CKEditor plugins try hard to avoid naming their settings the same because having to settings with the same name is not supported, and I don't see how it can be for all the cases that would come up.
What the CKEditor custom config module currently does is the equivalent of hacking the plugin files to modify default settings, instead of overriding them just before instantiating the editor instance, via the alter hook.
Yes, the
internalplugin runs first in Drupal, but that's just an implementation detail and not something which should be relied on to change it's default settings at that point in the code flow. The defaults are defaults, not something that needs to change at that point in the config setup.Comment #13
Chris Burge CreditAttribution: Chris Burge commentedContinued comments regarding CKEditor config are irrelevant and out of scope. Please move this discussion elsewhere.
Comment #14
TwoDThe CKEditor Custom config module is relevant because it is the only case I know of that has tried to use the Editor plugin API (or the equivalent preceeding Wysiwyg module API) in the way the posted patch would allow.
If this module can demonstrate a use case where this change is required (vs implementing
hook_editor_js_settings_alter()it would mean we have found something that was not considered during the design phase.If so, I would like to bring this to the attention of the other module co-maintainers so we can all have a deeper discussion about the implications and consequences - such as how to handle multiple modules wanting to change the defaults of multiple other modules in a deterministic and explainable way.
Comment #20
quietone CreditAttribution: quietone at PreviousNext commentedCKEditor has been removed from core, CKEditor 4 is removed from Drupal Core in 10.0.0
Comment #21
apaderno