Problem/Motivation

  • JS translation files are never added to Drupal pages => strings in JS are never translated.
  • When parsing the JS files for source strings, singular and plural strings are stored separately. This is wrong, since Drupal 8 now stores these concatenated as one source string for easier lookup.
  • The Drupal.formatPlural() logic then needs to be updated to look up the ETX character concatenated string (add an equivalent of LOCALE_PLURAL_DELIMITER in JS too).
  • We now need to replace multiple occurrences of the placeholder, since @count will appear multiple times. If a placeholder appears more than once in a string then only the first occurrence gets replaced.

The #1273968: remove eval from locale.module issue is cannot be solved without this fix.

Proposed resolution

  • Add the JS to the page properly. That is a one line fix, the state name is wrong.
  • Parse the JS file properly for singular/plural string pairs, and save them as one string like the .po parser / format_plural backend.
  • Update the logic in Drupal.formatPlural() to use the single concatenated string lookup to find translations and split for the right index.
  • Support replacing multiple instances of the same placeholder, so multiple instances of @count as with these strings will work.

Remaining tasks

Review.

User interface changes

JS translations will work again :)

API changes

None. This brings the JS parser and JS file generation in line with the rest of the locale system. If the structure of Drupal.locale.strings is to be considered an API, now singular+plural string pairs and their translations will appear as one string pair, not multiple string pairs. We don't believe this is an API though.

Files: 
CommentFileSizeAuthor
#59 locale-js-plural-2182265-59-D6.patch3.47 KBSweetchuck
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-js-plural-2182265-59-D6.patch. Unable to apply patch. See the log in the details link for more information. View
#54 1230986-54-drupal-t-placeholders.patch2.42 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 41,167 pass(es). View
#51 1230986-14-drupal-t-placeholders.patch2.43 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 40,733 pass(es). View
#45 locale-js-plural-2182265-45-D8.patch492 bytesandrejsmuzikovs
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-js-plural-2182265-45-D8.patch. Unable to apply patch. See the log in the details link for more information. View
#37 locale-js-plural-2182265-37-D8.patch15.42 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 63,610 pass(es). View
#31 locale-js-plural-2182265-31-D8.patch14.54 KBSweetchuck
PASSED: [[SimpleTest]]: [MySQL] 63,318 pass(es). View
#27 core-locale-js-plural-2182265-27.patch9.34 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 63,319 pass(es), 45 fail(s), and 0 exception(s). View

Comments

Sweetchuck’s picture

Status: Needs work » Needs review
FileSize
7.09 KB
FAILED: [[SimpleTest]]: [MySQL] 63,042 pass(es), 45 fail(s), and 0 exception(s). View
Sweetchuck’s picture

Issue tags: +#SprintWeekend2014
Sweetchuck’s picture

FileSize
7 KB
FAILED: [[SimpleTest]]: [MySQL] 63,146 pass(es), 45 fail(s), and 0 exception(s). View
nod_’s picture

Status: Needs review » Needs work
  • Drupal.stringReplace = function (str, args, keys) { instead of Drupal.stringReplace = function(str, args, keys) { (the space before the function opening parenthesis).
  • !Array.isArray(keys) instead of keys instanceof Array !== true
  •   var key = keys.pop();
      var fragments = str.split(key);

    instead of

      var
        key = keys.pop(),
        fragments = str.split(key);
  • I'd be tempted to call Drupal.pluralDelimiter, Drupal.PLURAL_DELIMITER to make sure people don't mess around with it. Or better yet, add the value to drupalSettings from the PHP so that it can use the PHP constant directly.
  • since Drupal.locale.pluralFormula is used a bunch of times in Drupal.formatPlural, could you put that in a variable?

Haven't looked into the logic too much, to late for that on my end :)

Sweetchuck’s picture

The "pluralFormula" is not parsed.

Cannot use 'in' operator to search for '1' in (($n==1)?(0):((((($n%10)>=2)&&(($n%10)<=4))&&((($n%100)<10)||(($n%100)>=20)))?(1):2))

nod_’s picture

What I meant was:

var pluralFormula = Drupal.locale.pluralFormula;
// …
index = count in pluralFormula ? pluralFormula[count] : pluralFormula['default'];
// …
nod_’s picture

Umm looks like this patch needs the other one to work properly too. Sounds like you'd want to put the changes here in the other patch no? and solve everything in one patch?

Sweetchuck’s picture

At the beginning I started to work on the "eval" issue, but I ran into more and more problems.
I fixed them together with Goba, but a I made some mistake when I split the modifications to two separate patch.

Sweetchuck’s picture

We use the Drupal.pluralDelimiter only together with Drupal.locale.pluralFormula, which is come from the <langcode>_<hash>.js file. So we can also put the pluralDelimiter character into the same file. Like you suggest.

Sweetchuck’s picture

Still does not works, but a little bit better.
(I go to sleep)

The last submitted patch, 1: locale-js-plural-2182265-1-8.patch, failed testing.

The last submitted patch, 3: locale-js-plural-2182265-3-D8.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -#SprintWeekend2014 +SprintWeekend2014, +sprint, +language-ui

@nod: Yeah there are *numerous* problems with JS translation in Drupal core right now, the primary thing is it does not work at all since the JS translation file is *never* added to the page. That is kind of a bid deal :D So lumping this in with removing eval() may be a mistake. Therefore the separate issue.

The fix here should:

1. Add the JS to the page properly. That is a one line fix, the state name is wrong :)
2. Parse the JS file properly for singular/plural string pairs, and save them as one string like the .po parser / format_plural backend.
3. Update the logic in Drupal.formatPlural() to use the single concatenated string lookup to find translations and split for the right index.

This patch is not meant to change looking up the index. It fixes all bugs that exist in Drupal core *already* so the eval() removal issue can work off of a working state and get to a different working state instead of starting with a totally bugos state :)

Updated issue summary with further details.

Gábor Hojtsy’s picture

Title: Plural translations on the client side. » Javascript file translations are several bugs away from working at all

Retitling for the scope too :)

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue summary: View changes
webchick’s picture

Priority: Major » Critical

Not being able to translate something at all sounds critical.

Sweetchuck’s picture

The Drupal.formatPlural() works on the client side.
Now I am starting to test it with missing translations. (with partially translated strings)

nod_’s picture

Nothing to complain about on the JS side :) Thanks!

Sweetchuck’s picture

Thanks :-)

But there are still several problems.
I use the Polish languge to testing, because it have 7 different plural formula.
The problem is that if the translation is missing then \Drupal::translation()->formatPlural() returns an empty string instead of the original English string.
The JS Drupal.formatPlural() works on the same wrong way. :-(

Example:
- If the singular is translated and one of the plural version is translated then formatPlural() returns an empty string when @count points to an untranslated index. (fail)
- If the singular is not translated, but one of the plural version is translated then formatPlural() returns an empty string when @count points to an untranslated index or @count = 1. (fail)
- Only works well if everything is translated or everything is not translated.

Wim Leers’s picture

  1. +++ b/core/misc/drupal.js
    @@ -223,10 +223,61 @@ Drupal.formatString = function(str, args) {
    + * @param str
    ...
    + * @param args
    ...
    + * @param keys
    

    Typehints missing.

  2. +++ b/core/modules/locale/locale.module
    @@ -1145,6 +1145,18 @@ function _locale_refresh_configuration(array $langcodes, array $lids) {
    + * @param string $string
    + *
    

    Incomplete parameter docs. Silly, I know, but it's the standard…

  3. +++ b/core/modules/locale/locale.module
    @@ -1327,14 +1327,18 @@ function _locale_rebuild_js($langcode = NULL) {
    +      'pluralDelimiter: ' . Drupal\Component\Utility\Json::encode(LOCALE_PLURAL_DELIMITER),
    ...
    +    $data[] = 'strings: ' . Drupal\Component\Utility\Json::encode($translations);
    

    Two invocations on the same class; let's use that class so that you don't need to specify the fully qualified class name twice.

Gábor Hojtsy’s picture

@Sweetchuck: is it realistic to support partial translations? Not sure how we could avoid that though. Eg. we should validate the submission of plural translations in form submission / .po import? Anyway, sounds like that is not part of this issue anymore (and not JS dependent), so should be its own issue :)

Sweetchuck’s picture

I am in a trouble with the \Drupal\Core\StringTranslation\TranslationManager::formatPlural()
The problem is that any index can contain an empty string in the $translated_array.
And if so - have to use the original $singular or $plural strings?
In other words:
If $translated_array[$index] is empty and the $options['langcode'] is not 'en' then call the $this->translate() again with different arguments?

Sweetchuck’s picture

@Gábor Hojtsy - Yes. You can submit the "User interface translation" form even if one of the translation is missing (singular or any plural).

Gábor Hojtsy’s picture

@Sweetchuck: Once again I don't think this should be in the scope for this patch. Don't consider that scenario in the test. This is not related to the JS problems.

Sweetchuck’s picture

Sweetchuck’s picture

When I run the tests of the locale module I get the same result as the patch in #3 comment.

JAVASCRIPT TRANSLATION
Tests parsing js files for translatable strings
21 passes, 45 fails, and 0 exceptions

nod_’s picture

FileSize
9.34 KB
FAILED: [[SimpleTest]]: [MySQL] 63,319 pass(es), 45 fail(s), and 0 exception(s). View

This is a reroll because the indentation standard changed for JavaScript files. No need for commit credit because of this reroll.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Let's get testbot results :)

Status: Needs review » Needs work

The last submitted patch, 27: core-locale-js-plural-2182265-27.patch, failed testing.

Gábor Hojtsy’s picture

Part of the fails seems to be due to the singular/plural strings asserted as separate source strings found. That is obviously not good anymore, should be fixed. There are some other (string context) related fails, which I'm a bit puzzled about. We are not supposed to modify the preg pattern to find strings here :)

Sweetchuck’s picture

Status: Needs work » Needs review
FileSize
14.54 KB
PASSED: [[SimpleTest]]: [MySQL] 63,318 pass(es). View
Sweetchuck’s picture

I think the D7 is also affected by the problem of the JS String.replace(), that is only the first occurrence get replaced.

Drupal.t("Lorem ipsum @foo sid amet @foo", { "@foo": "bar" }, {});

Sweetchuck’s picture

There are no test cases for the string concatenations where the quotation marks are mixed. Like this:
Drupal.t("Double" + 'simple')

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Great job, mostly English languages fixes needed.

I would refrain from doing any more expansions of scope of this patch. IMHO its totally fine to fix the following and get this in. Its a critical bugfix after all, we don't need to fix all kinds of side things while we are at it :)

  1. +++ b/core/misc/drupal.js
    @@ -223,10 +226,61 @@ if (window.jQuery) {
    +   * The longest keys will be tried first.
    +   * Once a substring has been replaced, its new value will not be searched again.
    

    Don't do the newline after the first sentence, just continue with the second. Also the second line was longer than 80 chars, but this will resolve itself with the above tip :)

  2. +++ b/core/misc/drupal.js
    @@ -223,10 +226,61 @@ if (window.jQuery) {
    +   *   Array of keys from the "args".
    +   *   Internal use only.
    

    Merge onto 1 line.

  3. +++ b/core/misc/drupal.js
    @@ -223,10 +226,61 @@ if (window.jQuery) {
    +      // Order the keys by the character length. The shortest one is the first.
    +      keys.sort(function (a, b) { return a.length - b.length; });
    ...
    +    var key = keys.pop();
    

    Maybe document right above the pop() that

    // Take next longest one from the end.

    or something along those lines. The other comment makes it look like the short ones will be processed first, while that is not true :)

  4. +++ b/core/modules/locale/locale.module
    @@ -1168,6 +1169,19 @@ function _locale_refresh_configuration(array $langcodes, array $lids) {
    + * Remove the quotes and string concatenations from the string.
    

    Removes

  5. +++ b/core/modules/locale/locale.module
    @@ -1168,6 +1169,19 @@ function _locale_refresh_configuration(array $langcodes, array $lids) {
    + * @param string $string
    + *   Single or double quoted strings concatenated by plus (+) sign.
    

    optionally concatenated, no?

  6. +++ b/core/modules/locale/locale.module
    @@ -1168,6 +1169,19 @@ function _locale_refresh_configuration(array $langcodes, array $lids) {
    + * @return string
    + *   Remove leading and trailing quotes.
    

    String with leading and trailing quotes removed.

  7. +++ b/core/modules/locale/locale.module
    @@ -1241,43 +1255,28 @@ function _locale_parse_js_file($filepath) {
    -      $source = \Drupal::service('locale.storage')->createString(array(
    -        'source'    => $string,
    -        'context'   => $context,
    -      ));
    +      $source = \Drupal::service('locale.storage')->createString($match);
    

    Nice simplification :)

  8. +++ b/core/modules/locale/locale.module
    @@ -1322,6 +1321,9 @@ function _locale_invalidate_js($langcode = NULL) {
    + *   Returns TRUE if translation file is exists FALSE otherwise.
    

    TRUE if translation file exists, FALSE otherwise.

  9. +++ b/core/modules/locale/locale.module
    @@ -1350,14 +1352,18 @@ function _locale_rebuild_js($langcode = NULL) {
    +    $data = 'Drupal.locale = { ' . implode(', ', $data) . ' };';
    ...
    +
    ...
    +    $data[] = 'strings: ' . Json::encode($translations);
    

    Would it make sense to use the Json library to generate the whole file (from an array) instead of doing parts of it with Json and parts of it manually?

Sweetchuck’s picture

@Gábor Hojtsy "... Would it make sense to use the Json library to generate the whole file (from an array) instead of doing parts of it with Json and parts of it manually? ..."

The "pluralFormula" is a function, so we can't use the json_encode() directly on the $data. The "eval" patch will optimize this.

Gábor Hojtsy’s picture

@Sweetchuck: that makes sense :) Looking forward to the updates for the rest of the points.

Sweetchuck’s picture

FileSize
15.42 KB
PASSED: [[SimpleTest]]: [MySQL] 63,610 pass(es). View
Sweetchuck’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing those. Fixes look good, now JS translations work again :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, thanks!

Sweetchuck’s picture

@nod_ In the comment #4 you suggest :
"I'd be tempted to call Drupal.pluralDelimiter, Drupal.PLURAL_DELIMITER to make sure people don't mess around with it. Or better yet, add the value to drupalSettings from the PHP so that it can use the PHP constant directly."

The problem is that we need the LOCAL_PLURAL_DELIMITER in order to Drupal.t() and Drupal.formatPlural() to works well even if the current language is English, but the files/languages/<lang_code>_<hash>.js file is only generated for the non-english languages.

Can we add the delimiter charachter to the base settings?

function _drupal_add_js() {
  // ...
  if (!isset($javascript['settings'])) {
    // Somewhere here.
  }
  // ...
}
nod_’s picture

I understood wrong how the whole thing was working.

Can you add the delimiter to Drupal.local.localPluralDelimiter when the file gets generated? Would that work?

Ok too dirstracted now, I'll have a look later.

Gábor Hojtsy’s picture

@Sweetchuck: IMHO add it to the base settings or just hardcode the same value in drupal.js as well. I don't think its a big issue to have the same char defined at two places, the logic for the plurals, etc. is redefined in PHP and JS anyway.

andrejsmuzikovs’s picture

FileSize
492 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-js-plural-2182265-45-D8.patch. Unable to apply patch. See the log in the details link for more information. View

I believe that Drupal.stringReplace, which was commited in, doesn't properly replace all the placeholders. Noticed this when "edit" module wasn't loading WYSIWYG by clicking quick-edit. Can quickly reproduce this by running Drupal.stringReplace("/x/!aa/!b", {"!aa":1, "!b":2}, null);.

Expected result: "/x/1/2"
Actual result: "/x/1/!b"

I wasn't sure if I should open a new issue for this, it seems to be related to this one. Added a patch that is running OK locally.

/Andrejs

Sweetchuck’s picture

I can confirm the bug (described in #45) is exists and the patch is solve the problem.

@Andrejs I am not a JS expert, so can you explain me why need to clone the keys array, please?

catch’s picture

@ andrejsmuzikovs - please open a new issue for that patch.

andrejsmuzikovs’s picture

@catch Here it is https://drupal.org/node/2192809

I also explained it a bit more there.

/Andrejs

Status: Fixed » Closed (fixed)

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

droplet’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Sweetchuck » Unassigned
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to 7.x
Related issues: +#1230986: Drupal.t() placeholder replacement is wonky.
Sweetchuck’s picture

FileSize
2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 40,733 pass(es). View

Drupal.formatString() and Drupal.stringReplace() ported. (technically copied)

Sweetchuck’s picture

The other issue was created on " July 27, 2011". Why that is marked as duplicated?

droplet’s picture

Status: Patch (to be ported) » Needs work
Issue tags: +Needs backport to 6.x

@Sweetchuck,
That issue reported same bugs.

+++ b/misc/drupal.js
@@ -168,23 +168,76 @@ Drupal.checkPlain = function (str) {
+  if (!Array.isArray(keys)) {

The only stuff I've seen that we can't use ES5 Array.isArray() in D7 and below. $.isArray() will does the job :)

Thanks!!

Sweetchuck’s picture

FileSize
2.42 KB
PASSED: [[SimpleTest]]: [MySQL] 41,167 pass(es). View

Patch for D7 with if (!$.isArray(keys)) {

droplet’s picture

Status: Needs work » Reviewed & tested by the community

The last submitted patch, 45: locale-js-plural-2182265-45-D8.patch, failed testing.

Sweetchuck’s picture

Now I am working on the D6 backport patch and the jQuery version is too old. There is no $.isArray()
Can I use if (!(keys instanceof Array)) { instead?

http://stackoverflow.com/questions/767486/how-do-you-check-if-a-variable...

There is another suggestion toString.call(obj) === "[object Array]"

droplet’s picture

instanceof may failed at some case.

The another suggestion used in jQuery now.

toString.call(obj) === "[object Array]"
Sweetchuck’s picture

FileSize
3.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-js-plural-2182265-59-D6.patch. Unable to apply patch. See the log in the details link for more information. View

Backport to D6

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 59: locale-js-plural-2182265-59-D6.patch, failed testing.

droplet’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC, Patch #54 for D7.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Has there been any manual testing of this patch (cross-browser, etc)? The code looks good, but we need to be pretty careful with these kind of JavaScript patches that change fundamental parts of the code...

Also, is it correct that the issue title here is invalid for Drupal 7; for Drupal 7 we just are fixing #1230986: Drupal.t() placeholder replacement is wonky. and that's it?

mgifford’s picture

What's the best way to test this? With "JS translation files are never added to Drupal pages => strings in JS are never translated." it should be everywhere, right?

The summary states "The #1273968: remove eval from locale.module issue is cannot be solved without this fix.", but that's obviously not the case as it's been fixed.

Note, last patch still applies nicely.

Gábor Hojtsy’s picture

@mgifford: I don't think the D8 title and the D8 dependencies of this issue apply to it now that it is on Drupal 7.

droplet’s picture

@mgifford,

on D7 patch, it only need to fix the problem of #1230986: Drupal.t() placeholder replacement is wonky.. Copy the code on the summary into the browsers console. If it giving expected results, then it worked.

** as requests #62, mainly i think we have to test it on IE6,7,8. other browsers i did some tests already. (if it does not working, we should go back to D8 first)

Thanks.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

What happened to if (args) { in Drupal.formatString? Patch no longer applies.

droplet’s picture

Issue tags: -Needs reroll

#54 still applies

mgifford’s picture

arg - I was looking at the D6 version in #59. Sorry.

xjm’s picture

Sweetchuck’s picture

Priority: Critical » Major
Status: Needs work » Needs review
Issue tags: +Needs manual testing

The patch #54 is still applicable to the latest 7.x and the patch #59 is still applicable to the latest 6.x

The original issue for 8.x was critical because the client side translation was not worked at all.
In D7 and D6 you can reproduce the bug only if you are using the same place holder twice or more in a string.
I think this bug is not critical any more.

Drupal.t('@foo @foo', { '@foo': 'bar' } );

Without the patch the code above output: bar @foo instead of bar bar.

My test code is:

(function ($) {
  'use strict';
  Drupal.behaviors.i2182265 = function (context, settings) {
    console.log(Drupal.t(
      'One = @one, Two = @two, Three = @three - One = @one, Two = @two, Three = @three',
      {
        '@one': '@two',
        '@two': '@three',
        '@three': '@four'
      },
      {}
    ));
  };
}(jQuery));

David_Rothstein’s picture

Title: Javascript file translations are several bugs away from working at all » Drupal.t() placeholder substitution doesn't always work correctly (backport part of the Javascript file translation fixes from Drupal 8)
Related issues: +#2508812: t() function not working the same way in PHP and JS

I marked #2508812: t() function not working the same way in PHP and JS as duplicate which suggests a simpler fix (although I think it doesn't necessarily solve all the cases this patch does, i.e. everything mentioned in #1230986: Drupal.t() placeholder replacement is wonky.).

Also giving this a better title for the backport.

David_Rothstein’s picture

Component: locale.module » javascript

  • catch committed b3e112f on 8.3.x
    Issue #2182265 by Sweetchuck, nod_: Javascript file translations are...

  • catch committed b3e112f on 8.3.x
    Issue #2182265 by Sweetchuck, nod_: Javascript file translations are...

  • catch committed b3e112f on 8.4.x
    Issue #2182265 by Sweetchuck, nod_: Javascript file translations are...

  • catch committed b3e112f on 8.4.x
    Issue #2182265 by Sweetchuck, nod_: Javascript file translations are...