As of #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, Drupal ships with its own "image" and "link" plugins for CKEditor. What we haven't figured out yet, is how to deal with translations.

CKEditor has its own translation system, as does Drupal: Drupal.t().

Drupal.t() only works if any JS it is used in is loaded via PHP: Drupal dynamically adds the translation JS file for a given JS file (see locale.module's hook_js_alter() implementation).

But … Drupal-specific CKEditor plugins are still CKEditor plugins, hence CKEditor is what loads them, not Drupal. Which means the translation files won't be loaded automatically.

So, the question is: how do we deal with this? Pinging Drupal domain experts Gábor Hojtsy & nod_, as well as the CKEditor developers.


Note: non-Drupal-specific CKEditor plugins should of course use CKEditor's translation system.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

I don't really have any great ideas from the top of my head... We may be able to get a list of their strings and inject the translations in a format ck would need to the page? Or just rely on ck translations pre-existing.

FredCK’s picture

Options:

1. Have the translation files in the plugin folder... the CKEditor way... and have translators maintaing those files directly.
2. Feed CKEditor with translation entries at run time. Translations maintained in the Drupal way.

If 2 is the way to go, let us know and we'll figure out the best way to code it.

FredCK’s picture

Some more info about "2"...

Editor instances fire the "langLoaded" event, which could be used to add new language entries to editor.lang.

Wim Leers’s picture

#2: Option 1 is less than ideal because it'd make our existing massive translation systems useless for code that is essentially Drupal code: http://localize.drupal.org/

#3: now that sounds like a step in the right direction! :) I'm wondering if we can do something like locale_js_alter() in JS only, or potentially call back to the server, execute locale_js_alter() or something similar, and cache the results on the client side, so that we now what file to request.

Now the ball is in the Drupal camp: how can we leverage #3? Does what I said above make any sense at all? :)

Wim Leers’s picture

webchick’s picture

Priority: Normal » Major

It seems like this is at least major, no?

Wim Leers’s picture

You're probably right!

Gábor Hojtsy’s picture

@Wim: Is any Drupal library loading happening for those JS files or anything that would let Drupal know they exist? If not then JS needs to manage this purely. That sounds like may be better done with some rearchitecture of the JS translation system then? I mean we can pre-generate translation JS files for each JS file per language. Then those can be loaded by CK. That would mean we know where to look for those JS files even if they are not added via a JS library. So maybe the same problem again :D We don't want to parse CK's or other 3rd party files though.

Wim Leers’s picture

#8:

  • No, no Drupal library loading happens; CKEditor triggers the downloading of plugins' .js files.
  • I'm not sure what you're saying. Are you saying that we can extend Drupal's handling of JS translations to also find Drupal-specific CKEditor plugins and then somehow, during cron probably, pregenerate translation .js files for them?
Gábor Hojtsy’s picture

Yeah I've been talking about pre-generation. I think if we do that though, we probably want to do that for all of the JS files, and then just pick up the pre-generated translation JS files for the JS files added to the page in the normal setup too. It may be a num of request disaster for those not using JS aggregation, but for them JS requests are a disaster anyway :D

Gábor Hojtsy’s picture

One very easy option is the following: if you define a ckeditor plugin, you can register attached JS that is loaded if CK is loaded. This JS would contain only the strings from the plugin. Granted, this is some overhead on the page if the plugin does not get loaded at the end, but this is likely very minimal (it is hard to imagine the plugins would contain lots of strings, and the string JS would only result in at most twice the amount of strings added (which is normal practice for the JS translation system). So the only case would be when the module to add the plugin is enabled but the plugin itself is not loaded and the JS overhead would be very minimal.

Wim Leers’s picture

Haha, very clever work-around :)

So would that mean that the "strings only" JS file would look like this:

(function (Drupal) {

Drupal.CKEditorUnicornPluginStrings = {
  'dialog_title': Drupal.t('Insert pony'),
  'dialog_button_confirmation': Drupal.t('Yes, sparkles please!'),
  'some_description': Drupal.t('Unicorns are awesome, but where do they live?')
};

})(Drupal);

and then the CKEditor plugin would get the text they want by doing e.g. buttonName = Drupal.CKEditorUnicornPluginStrings.dialog_button_confirmation?

Isn't that … awful? I see two alternatives.


A. How CKEditor plugin loading works

(Having written this, I think my answer in #9 was pretty poor. Sorry :()

Step 0
Each CKEditor plugin defines its file:
  /**
   * {@inheritdoc}
   */
  public function getFile() {
    return drupal_get_path('module', 'ckeditor') . '/js/plugins/drupalimage/plugin.js';
  }
Step 1
CKEditor::getJSSettings() generates the URL for that CKEditor will use to load any plugin that isn't part of Drupal's CKEditor build:
    $settings += array(
      'drupalExternalPlugins' => array_map('file_create_url', $external_plugin_files),
    );
Step 2
Our "glue ckeditor.js" takes the above information and loads the files it's instructed to load (one file per plugin):
  _loadExternalPlugins: function (format) {
    var externalPlugins = format.editorSettings.drupalExternalPlugins;
    // Register and load additional CKEditor plugins as necessary.
    if (externalPlugins) {
      for (var pluginName in externalPlugins) {
        if (externalPlugins.hasOwnProperty(pluginName)) {
          CKEDITOR.plugins.addExternal(pluginName, externalPlugins[pluginName], '');
        }
      }
      delete format.editorSettings.drupalExternalPlugins;
    }
  },
Conclusion
Besides CKEditor's langLoaded event that FredCK mentioned in #2, we control the two points of control where can dynamically change things!

B. Approach 1: Pregenerate the translation file on the server side

(This is probably similar to what you were thinking in #8/#10 before I gave a crappy answer in #9.)

  1. Apply logic similar to that in locale_js_alter() to each plugin file that is added by CKEditor::getJSSettings() to the drupalExternalPlugins setting. Then, instead of adding *one* JS file ({'drupalImage: '/path/to/plugin.js'}), we add two: one with the actual logic, one with the translation for the current page's language ({'drupalImage': '/path/to/plugin.js', 'drupalImage-locale-nl': '/path/to/nl_HASH.js'}.
  2. _loadExternalPlugins() will just load the translation files as well.
  3. Done!

C. Approach 2: Have a route on the server that redirects to a specific JS file's translation

This approach could also be very helpful when using Drupal as the underlying framework/content store for websites whose front-end is written in e.g. Backbone, which could then use Drupal.t() for translations for both the front-end (Backbone) and back-end (Drupal).

  1. No changes to CKEditor::getJSSettings(), but ensure that there is something like drupalSettings.currentLangcode.
  2. Modify _loadExternalPlugins() to check if the current language is not English, and if so, make a request to http://example.com/locale/js/LANGCODE/path/to/plugin.js, which would then perform a redirect to the actual translation file. (It would have to be a HTTP 302 temporary redirect to prevent browsers from caching the redirect, because otherwise it's possible that when the JS file is updated, the new translation won't be fetched.)
  3. Done!

Conclusion

Both approaches are better than #11 because there doesn't need to be a split anymore.
There's a clear front-end performance penalty for the second approach: redirects are bad.
So approach 1 seems the best approach to me.

Thoughts? Did I make mistakes?

Gábor Hojtsy’s picture

I don't think the JS strings file idea is awful especially given that Jesse is arguing for having a string list separate from code as a best practice in #2058843: Be consistent with respect to which strings within JS code should be overriddable via drupalSettings as well. In fact that gave me the idea :)

As for #12/2, I think redirects are indeed bad, and those will never participate in JS aggregation for the page. Seems like #12/1 would not participate either due to how the CK plugins and languages would be individually loaded. I think thats a problem, but CK plugins have that problem already. Sad.

#12/1 would also need a special flower implementation for JS file generation for CK plugin files (we currently generate JS translation files for each added JS file as processing on the server side). Because server side processing would not work in the standard way, we would need to reimplement some / most of https://api.drupal.org/api/drupal/core%21modules%21locale%21locale.modul... in a CK specific way. I'm not sure how _loadExternalPlugins() would tell the hash value?

Wim Leers’s picture

#13:

RE: strings separate: I think it's not wise to move in that direction because it is counter to what we do and recommend in PHP.

RE: #12/2: okay, that's out then :)

RE: #12/1: No, locale_js_alter() wouldn't need to change in a CKE-specific way at all. It'd just need to be refactored, so that the core logic could be applied to a single file if desired. Or is that a wrong assumption?


Maybe an even better solution would be to be able to do this: DON'T let CKEditor dictate when CKEditor plugins are loaded, but just always let Drupal load them already.

There is one downside if we do it this way: we force all users to download all plugins for all text formats they have access to. But then again, the majority of users will only have access to 1 text format, so the difference would be negligible for most users. (This is essentially the only "conditional"/"JIT" asset loading that we have in Drupal 8, and it's not even Drupal 8's :P).

There is also one (pretty big) problem: the fact that they look something like this:

(function ($, Drupal, drupalSettings, CKEDITOR) {

"use strict";

CKEDITOR.plugins.add('drupalimage', {
  init: function (editor) {
…

CKEDITOR must already be defined (i.e. loaded) in order for CKEditor plugins to load! Maybe the CKEditor guys know a way around this? I'll ping them.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
4.02 KB

I think its sad that none of the JS in CK participates in our JS aggregation because once you start to dress up your editor, it will be lots of extra HTTP requests. Granted those maybe concentrating more on the backend site and would only appear on the frontend when you click into an in-place editing field that uses CK (in which case actually loading them later instead of lumping it up for the page may in fact be better). Hum.

Is this the refactoring that you are looking for?

- factor out the file parsing and translation JS generation from the alter function, so we can use this without the specific JS file list array
- use this from CK plugin to add JS files from CK

This should help expand JS source text and update translation files even though the JS files are not "added" to the page. The language translation files for Drupal in general are one piece per language, so all CK plugin source text will be added to all pages which may have JS files.

In case of the CK plugin, we don't use the return value (translation JS file name), so this can go back to the alter function, but I think it may be better if it stays the same.

Two notes:

1. I found local.settings as reference to the JS dir in the alter function, wondering how this ever worked(?) - I assume D8 has a regression right now where no JS translations appear. Heh.
2. I used the moduleHandler in the patch in CK, but that is not yet available there. That is being added in #2050097: Map CKEditor languages to Drupal languages. I did not want to repeat the same boilerplate here too. I think the suggested code change is reviewable as-is (but it will not pass).

Status: Needs review » Needs work

The last submitted patch, ck-plugin-language.patch, failed testing.

Wim Leers’s picture

We should have a test similar to LocaleTranslationUiTest::testJavaScriptTranslation() in CKEditorLoadingTest. But I don't know how to configure locale module without performing the whole series of HTTP requests that LocaleTranslationUiTest::testJavaScriptTranslation() is performing, even though we should just test whether a CKE plugin's Drupal.t() call is picked up and ends up.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Great updates! Thanks! Indeed, we can extend the test cases. Do we already have CK plugin text that needs to be translated?

Wim Leers’s picture

Yes: core/modules/ckeditor/js/plugins/(drupalimage|drupallink)/plugin.js -> Drupal.t('Image') etc.

Gábor Hojtsy’s picture

That sounds pretty good! :)

Wim Leers’s picture

#2050097: Map CKEditor languages to Drupal languages got committed, which unblocks this issue, so a simple re-uploading of #17, with the -do-not-test suffix removed.

Gábor Hojtsy’s picture

Added test coverage. Need to enable locale module, install some key tables, so the string discovery works and then retrieve the JS settings. The strings are discovered, so we can look for a specific string from the CKEditor image plugin, proving that this works. Would not really call this a unit test, but its a not full webtest at least :)

Gábor Hojtsy’s picture

Issue tags: -Needs tests

Remove tag.

Wim Leers’s picture

Assigned: Wim Leers » Gábor Hojtsy
Status: Needs review » Reviewed & tested by the community

Excellent! And such a concise test! :)

With that done, this is RTBC :)

Great job, Gábor!

Dries’s picture

Assigned: Gábor Hojtsy » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Wim Leers’s picture

Issue tags: -sprint

Yay :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.