Drupal.t() placeholder substitution doesn't always work correctly (backport part of the Javascript file translation fixes from Drupal 8)

• 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 :)

What I meant was:

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

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?

@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.

Retitling for the scope too :)

Not being able to translate something at all sounds critical.

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

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.

@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: 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.

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

Let's get testbot results :)

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 :)

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: that makes sense :) Looking forward to the updates for the rest of the points.

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

Committed/pushed to 8.x, thanks!

Woot, thanks!

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.

@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.

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

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

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

I also explained it a bit more there.

/Andrejs

@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!!

instanceof may failed at some case.

The another suggestion used in jQuery now.

toString.call(obj) === "[object Array]"

Back to RTBC, Patch #54 for D7.

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?

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.

@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.

@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.

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

#54 still applies

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

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.

Passes manual testing in these browsers:

• Chrome
• Firefox
• Safari
• IE11

Test javascript code

alert(Drupal.t('See my email: @emailaddr and again @emailaddr', {'@emailaddr': 'drupal@7pro.ca'}));

before patch:

with patch 54 (79 is same as 54, just added 79 to allow testbot to run):

This looks very good. I did some additional testing (including some of the scenarios from #1230986: Drupal.t() placeholder replacement is wonky.) and everything seems to work. I am adding issue credit for mr.baileys (who worked on that issue) also.

Committed to 7.x - thanks!

Already in latest version of drupal 7 only , but drupal.t placeholder value is not shown in translate interface. please clarify.

@sri@re Did you find anything else on this? I am having the same problem with placeholders in multiple places in Drupal 8.