Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
29.93 KB
quicksketch’s picture

This looks great Wim! My only minor complaint is that ckeditor.theme.inc should probably live at includes/ckeditor.theme.inc (although ckeditor.admin.inc is empty so far, I can understand why you might consider eliminating the includes directory entirely). Alternatively I suppose we could also use ckeditor.admin.inc for these preprocess/theme functions. The use of x.theme.inc files might be reduced a bit by Twig because it will eliminate theme functions.

quicksketch’s picture

What do you think about hook_ckeditor_plugins()? Is that something that could (or should) be converted to D8's plugin system also? It's a bit different because if each CKEditor plugin were required to be a Drupal plugin class, we'd end up with ~20 classes that would all be empty. At first I thought because most *_info() hooks are going away in D8 (and hook_ckeditor_plugins() is still essentially an *_info() hook, even though it doesn't have "info" in its name), that using the plugin system would make sense. However on further thought, I'm not sure it's a good fit. Perhaps if each plugin provided a settings callback too, the plugin system would make sense. Right now the module is simply assuming the plugin will form_alter() the CKEditor configuration form to expose its options.

quicksketch’s picture

Status: Needs review » Fixed

I move the theme functions into ckeditor.admin.inc and updated hook_theme(). Otherwise committed as is. Thanks Wim! Let's followup if needed on hook_ckeditor_plugins() being converted to the Drupal plugin system.

Wim Leers’s picture

Well, if the end goal is to get CKEditor in core, then you really should use ckeditor.theme.inc. In Drupal core, an "includes" directory is bad practice, what I did is considered best practice. (I also learned this the hard way through the Edit module.)

I haven't put a lot of thought into hook_ckeditor_plugins() yet, but I agree that that is something we need to start looking into. I created #1874712: Analyze/improve how other modules can provide CKEditor plug-ins to discuss that.

quicksketch’s picture

Hm, yeah it seems includes directories are indeed uncommon in core. There are 29 *.admin.inc files and only 2 *.theme.inc files (both in Views, which also is the only module with an "includes" directory). I think we just need to move ckeditor.admin.inc to the root.

Wim Leers’s picture

Yep, then we'd be fine too :)

quicksketch’s picture

Okay ckeditor.admin.inc is now moved to the root.

Wim Leers’s picture

Title: Convert to use the plug-in system » Convert to use the plugin system
Status: Fixed » Needs review
FileSize
3.75 KB
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
FileSize
9.38 KB

Another reroll, as of #1833716-139: WYSIWYG: Introduce "Text editors" as part of filter format configuration. Also, the one in #9 probably wouldn't have applied because it was not relative to HEAD, but relative to something similar to HEAD on my local repo. Hence no interdiff.

Wim Leers’s picture

FileSize
12.42 KB

Patch in #10 excluded changes to js/ckeditor.admin.js. Most of the changes there are for JS strict mode compatibility.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Fixed

quicksketch has made me a maintainer and has approved this patch to be committed, so: committed!

http://drupalcode.org/project/wysiwyg_ckeditor.git/commit/33f1c07

quicksketch’s picture

Thanks Wim. Although I would prefer to keep the CKEditor settings from being unnecessarily nested (i.e. $editor->settings['format_list'] instead of $editor->settings['editor']['format_list'], the new structure that makes the settings array match the form array exactly doesn't bother me much and it does make things a bit more consistent.

One thing I would like to change though is to avoid $editor->settings['toolbar']['toolbar'], yuk. I changed the sub element to be "buttons" instead to avoid this double-nesting of the same key. Pushed up this code to 8.x-1.x.

Wim Leers’s picture

Heh, that's all fine with me.

What I did boils down to one simple thing: make all the code match the settings form structure that you had. The only reason you didn't have to keep it all consistent before is because you were using #parents to get the CKEditor's settings subform values more easily, but that's now handled by the Editor module already, so that's why I was able to make it more consistent (i.e. as if it were a normal form). If you'd stop using a "toolbar" fieldset to put everything in, then we could also get rid of the "top level" of CKEditor's settings. But since you put it in a "toolbar" fieldset, I figured you might want to add other fieldsets in the future (though I'm not sure what those would be).

I made it a bit more verbose, but also more consistent. Your changes in #14 to make that a bit more elegant make sense, of course.

quicksketch’s picture

make all the code match the settings form structure that you had. The only reason you didn't have to keep it all consistent before is because you were using #parents to get the CKEditor's settings subform values

Yep, understood. Though if we get the default settings merging properly as I described in #1833716-148: WYSIWYG: Introduce "Text editors" as part of filter format configuration, we may need to move these settings back to the root of $editor->settings so that they can be more easily merged with defaults.

Wim Leers’s picture

I don't see why the settings would need to live at the root. If we sue the same merging mechanism that we use for merging JS settings (improved in D8), then it'll work just fine for nested settings as well? In any case merging of nested settings should also work correctly, because that is what will be done by other modules.
Finally, I of course agree that it makes sense to make the settings array as simple as possible. Preferably though, it'll retain the same structure as the form, no?

quicksketch’s picture

If we sue the same merging mechanism that we use for merging JS settings (improved in D8), then it'll work just fine for nested settings as well?

This might be okay. In the first versions of the patch, the addition was only done with one layer (a single array merge). This would cause problems if you had a setting that was an array, and you wanted to replace that setting with a different array (not merging them).

i.e. if the default toolbar was this:

$settings['toolbar'] = array('Bold', 'Italics', 'Image');

And you wanted to change it to this:

$settings['toolbar'] = array('Bold', 'Italics', 'Underline');

A recursive merging of the settings would end up with:

$settings['toolbar'] = array('Bold', 'Italics', 'Image', 'Underline');

But that's not what you wanted. You wanted Bold, Italics, Underline and NOT Image. So merging recursively (even with the improved method in D8) could yield unexpected results. *However*, that might be a moot point since we now have not only hook_editor_default_settings() but also hook_editor_default_settings_alter(), so you could use the latter hook to modify the toolbar rather than the first.

So maybe it's nothing to worry about. However if we go with a recursive merge (and keep the current CKEditor setting structure), we'll need to reroll the Editor module patch.

Wim Leers’s picture

#18: that is true in d7, and used to be true in D8, but not anymore in D8 as of #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings). I've made the setting of JS settings idempotent (which is necessary in light of ESI). See #1875632-27: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings) for more details, #1875632-31: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings) for a follow-up patch that will clarify and document this explicitly in the codebase.

quicksketch’s picture

Hey Wim, so I read through that issue but I don't understand how that change is supposed to be made. I don't see how logically what you're saying is possible: how does Drupal know when to merge in a new value instead of removing an existing value? I think you might be interpreting my scenario as something different.

In any case, I do think you're right that the merging can be done recursively, though explicit removal of nested settings will still need to be done through hook_editor_settings_alter(). Right now the settings merging is done via a simple += operator, which obviously isn't going to have the better merging behavior. However @effulgensia already marked the patch RTBC. I'd really like to make it so that we don't have to check for NULL values all over the place, so we'll need to either adjust editor.module (to add the recursive merging) or ckeditor.module (to remove the nesting). I personally would be okay with modifying ckeditor, since I think less nesting is good. But from an ease of use and for other editors, the recursive merging certainly seems more flexible.

Wim Leers’s picture

#20: Sorry for not explaining it more clearly.

The "new D8" recursive settings merging with your example in #18 would work as follows:

STEP 1

Set the defaults.
$settings['toolbar'] = array('Bold', 'Italics', 'Image');

STEP 2

Override with user-specified settings.
$settings['toolbar'] = array('Bold', 'Italics', 'Underline');

END RESULT
$settings['toolbar'] = array('Bold', 'Italics', 'Underline');

i.e. in the aforelinked (in #19) issue, we removed the "auto append to the end of the array if it's not yet in the array" behavior.


For full detail about the how & why, please again see the parts of the issue I linked to from #19, but I'll try to summarize it a bit for you here.

The original reason for me starting #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings) is because of your CKEditor (well, wysiwyg_ckeditor) module. On e.g. the node/1 page, comments were enabled, hence there was a comment form with CKEditor (i.e. a CKEditor config for filtered_html), but the Edit module was also enabled, with support for CKEditor, and node 1 happened to be written using the Filtered HTML format, thus the filtered_html CKEditor config was added again.

Now, this is nothing special, and all should continue to work well. In the future, when Drupal 8 will be fully ESI-powered (also internally, for rendering any of its own pages), it will become very common to have the same settings added multiple times. Imagine that there are several comment forms on the same page. Each of those comment forms is in a separate block. Each of those blocks are rendered independently and the CSS, JS and JS settings that they need has been #attached to them. Now it becomes obvious that it's absolutely essential that Drupal's JS is able to not fail when the same JS settings are added multiple times.

Yet Drupal was failing to handle the JS settings for CKEditor properly! As you know (since you wrote it), the CKEditor "active toolbar" or "buttons" configuration is an array that is essentially an ordered sequence of CKEditor button identifiers, so that CKEditor knows how we want its toolbar to be rendered. Well, precisely because it was an array, it was causing problems.

So if we look at the code flow, this is what happened before the patch (simplified, but equivalent):

  1. The same setting is added twice:
    drupal_add_js(array('ckeditor' => array('toolbar' => array('button A', 'button B'))), 'setting');
    drupal_add_js(array('ckeditor' => array('toolbar' => array('button A', 'button B'))), 'setting');
    
  2. The result is
    {ckeditor: {toolbar: ['button A', 'button B', 'button A', 'button B']}}
    

And this is what happened after the patch (simplified, but equivalent):

  1. The same setting is added twice:
    drupal_add_js(array('ckeditor' => array('toolbar' => array('button A', 'button B'))), 'setting');
    drupal_add_js(array('ckeditor' => array('toolbar' => array('button A', 'button B'))), 'setting');
    
  2. The result is
    {ckeditor: {toolbar: ['button A', 'button B']}}
    

We essentially made it so that whenever a value is set for given key, that the value always overrides the previous value. There's no magical appending anymore. This is essential to make it reliable in the context of Drupal 8. In Drupal 8, we cannot rely on global variables to check whether a setting was already added or not.

I hope it's clear now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.