While #334283: Add msgctxt-type context to t() added context to t(), format_plural() and friends in PHP, it did not add it to their counterparts in JavaScript. Since we tried to keep the APIs in sync, and JS code in Drupal modules and themes is on the increase still, this looks like a bad omission. To add JS support, we should modify the code in _locale_parse_js_file(). But first, we need to define the API for it. While we worked hard to keep the API the same as in PHP (minus the $langcode) parameter, the new conversion of $langcode to $options which can also contain a context key dictates that we add the similar options object parameter here, which might not be easy to parse via regular expressions. Maybe we need to grab the optional options object and convert it as JSON to the PHP data structure.

JS parsing currently happens this way:

/**
 * Parses a JavaScript file, extracts strings wrapped in Drupal.t() and
 * Drupal.formatPlural() and inserts them into the database.
 */
function _locale_parse_js_file($filepath) {
  global $language;

  // Load the JavaScript file.
  $file = file_get_contents($filepath);

  // Match all calls to Drupal.t() in an array.
  // Note: \s also matches newlines with the 's' modifier.
  preg_match_all('~[^\w]Drupal\s*\.\s*t\s*\(\s*(' . LOCALE_JS_STRING . ')\s*[,\)]~s', $file, $t_matches);

  // Match all Drupal.formatPlural() calls in another array.
  preg_match_all('~[^\w]Drupal\s*\.\s*formatPlural\s*\(\s*.+?\s*,\s*(' . LOCALE_JS_STRING . ')\s*,\s*((?:(?:\'(?:\\\\\'|[^\'])*@count(?:\\\\\'|[^\'])*\'|"(?:\\\\"|[^"])*@count(?:\\\\"|[^"])*")(?:\s*\+\s*)?)+)\s*[,\)]~s', $file, $plural_matches);

  // Loop through all matches and process them.
  $all_matches = array_merge($plural_matches[1], $t_matches[1]);
  foreach ($all_matches as $key => $string) {
    $strings = array($string);

    // If there is also a plural version of this string, add it to the strings array.
    if (isset($plural_matches[2][$key])) {
      $strings[] = $plural_matches[2][$key];
    }

    foreach ($strings as $key => $string) {
      // Remove the quotes and string concatenations from the string.
      $string = implode('', preg_split('~(?<!\\\\)[\'"]\s*\+\s*[\'"]~s', substr($string, 1, -1)));

      $source = db_query("SELECT lid, location FROM {locales_source} WHERE source = :source AND textgroup = 'default'", array(':source' => $string))->fetchObject();
      if ($source) {
        // We already have this source string and now have to add the location
        // to the location column, if this file is not yet present in there.
        $locations = preg_split('~\s*;\s*~', $source->location);

        if (!in_array($filepath, $locations)) {
          $locations[] = $filepath;
          $locations = implode('; ', $locations);

          // Save the new locations string to the database.
          db_update('locales_source')
            ->fields(array(
              'location' => $locations,
            ))
            ->condition('lid', $source->lid)
            ->execute();
        }
      }
      else {
        // We don't have the source string yet, thus we insert it into the database.
        db_insert('locales_source')
          ->fields(array(
            'location' => $filepath,
            'source' => $string,
            'context' => '',
            'textgroup' => 'default',
          ))
          ->execute();
      }
    }
  }
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +msgctx

Tagged to summarize all contexts

tstoeckler’s picture

Subscribe.
Need this for the d7 module upgrade.

RobLoach’s picture

Wow, _locale_parse_js_file does look like a pile of gross.

andypost’s picture

Component: base system » javascript
Priority: Normal » Critical

Suppose this issue should be critical because now this functionality is different from core's

cwgordon7’s picture

Assigned: Unassigned » cwgordon7

I can probably do this.

cwgordon7’s picture

Assigned: cwgordon7 » Unassigned

Ok, this is way more complicated than I thought it would be. Parsing JavaScript through a giant regular expression to find strings to translate is fine, but when I have to modify that regular expression to additionally match a JavaScript object, I'm stuck. If someone can write me a regular expression to match the JavaScript object, I can finish this up.

andypost’s picture

@cwgordon7 what for you trying to add JS-objects? I think it could be just another param for Drupal.t() Please, provide some example

cwgordon7’s picture

@andypost: The additional parameter for Drupal.t would be the JavaScript object. Note that in JavaScript, an array with string keys is considered an object. It's just beyond me how to match a JS-object in a PCRE. Consider the current code:

  // Match all calls to Drupal.t() in an array.
  // Note: \s also matches newlines with the 's' modifier.
  preg_match_all('~[^\w]Drupal\s*\.\s*t\s*\(\s*(' . LOCALE_JS_STRING . ')\s*[,\)]~s', $file, $t_matches);

  // Match all Drupal.formatPlural() calls in another array.
  preg_match_all('~[^\w]Drupal\s*\.\s*formatPlural\s*\(\s*.+?\s*,\s*(' . LOCALE_JS_STRING . ')\s*,\s*((?:(?:\'(?:\\\\\'|[^\'])*@count(?:\\\\\'|[^\'])*\'|"(?:\\\\"|[^"])*@count(?:\\\\"|[^"])*")(?:\s*\+\s*)?)+)\s*[,\)]~s', $file, $plural_matches);

I have no idea how to change that to additionally match and store the value of a JavaScript object's (var name options) 'context' key.

cha0s’s picture

Sub

littleviking001’s picture

A JavaScript object should be valid JSON, so if it makes it easier, you can match the full object and pass it through drupal_json_decode to get a PHP array - rather than trying to parse it in with regex.

pwolanin’s picture

Priority: Critical » Normal

is this really critical?

ksenzee’s picture

Priority: Normal » Critical

I'm not familiar with the JS object that's being parsed here, but it's worth noting that JSON is only a subset of valid JS object notation - you can have perfectly valid JS defining a perfectly valid JS object, and JSON parsers will choke on it. There are full Javascript parsers out there, such as http://web.2point1.com/2009/11/14/jparser-and-jtokenizer-released/ - I would assume that's overkill here, but I'm throwing it out there just in case.

ksenzee’s picture

Priority: Critical » Normal

Oops.

andypost’s picture

Priority: Normal » Major

Suppose this major and maybe critical for release because it's functionality exists in D6 but broken in D7

Actually this needs a regexp guru

Gábor Hojtsy’s picture

Well, its not at all functionality that exists in Drupal 6, but it is indeed missing functionality that exists in PHP in Drupal 7 but not in JS in Drupal 7. As I said in the issue starter and as others pointed out, we can grab a whole object and parse it as JSON, so we can grab the context.

Gábor Hojtsy’s picture

Version: 7.x-dev » 8.x-dev

Well, Drupal 7 will not be consistent in context support, we can try in Drupal 8.

tstoeckler’s picture

Is there a reason why this cannot be fixed in 7.1?
While you might technically call this an API change, I don't see any BC-break here.

Gábor Hojtsy’s picture

Issue tags: +D8MI

Tag for Drupal 8 Multilingual Initiative.

loganfsmyth’s picture

Assigned: Unassigned » loganfsmyth
loganfsmyth’s picture

Assigned: loganfsmyth » Unassigned
Status: Active » Needs review
FileSize
9.29 KB

Here is a first attempt at this.

I have added an 'options' argument as the last argument of Drupal.t() and Drupal.formatPlural(), and updated the JS and PHP to make Drupal.locale.strings be nested by context, then source string, instead of just source string. This should all be similar to the way the core t() function works.

As for the complicated part, I have changed the regular expressions to match the context argument out of the options object. I did not use json_decode in the end because I decided that it would be too difficult to process the object literal as JSON, since something like {context:"mycontext"} will fail. I also thought it would be too confusing for developers to always have to make sure that particular argument was JSON compliant.

This seems to work overall, and the only matching problems that I can see that may need to be discussed as the following:
* 'args' has to be matched as object literal since anything else would probably be too strong. This means that if a user did Drupal.t('Some string', undefined, {context: 'kjdfg'}); it would not match.
* I don't think this will come up, but it's important to note that any nested objects in those literals could break parsing if they are in the right place.

Status: Needs review » Needs work

The last submitted patch, javascript-t-context-support-488496-20.patch, failed testing.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
9.3 KB

Here is a version that should pass testing :P Regular expressions are COMPLICATED.

loganfsmyth’s picture

This new patch relies on #1281932: Convert Drupal.t and Drupal.formatPlural regular expressions to extended form..

It uses extended regexes to make life easier for everyone.

Status: Needs review » Needs work

The last submitted patch, js-t-context-488496-23.patch, failed testing.

loganfsmyth’s picture

FileSize
14.43 KB

Here is a new patch that adds some tests to go along with my newest patch in #1281932: Convert Drupal.t and Drupal.formatPlural regular expressions to extended form., and removes some unneeded whitespace checks from the context regex.

loganfsmyth’s picture

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

Status: Needs review » Needs work

Wow, thanks for keeping this up! This looks *very* good. Just two comments:

- phpdoc comment blocks should have one line summary (noticed for the two define()'s you put in) and then the details after an empty line
- it would be great to add test coverage where arguments (replacements for the string) are provided before context, so we can ensure that works too

Otherwise it all looks good to me.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
14.66 KB

Fixed those comments and added 3 more testing strings, with substitution arguments.

Gábor Hojtsy’s picture

Looks fabulous! I think if this passes tests, it should be good to go :)

tstoeckler’s picture

Issue tags: +Needs backport to D7

This should be backported, right?

Gábor Hojtsy’s picture

Title: Implement msgctx context in JS translation » Implement missing msgctx context in JS translation for feature parity with t()
Status: Needs review » Reviewed & tested by the community

It is definitely good for D8, I think it would be great for D7 and the tests should prove the old (limited) parsing behavior still works too, so it should not be an issue hopefully to get into D7 too.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/locale.incundefined
@@ -43,6 +43,37 @@ define('LOCALE_LANGUAGE_NEGOTIATION_SESSION', 'locale-session');
+ * This pattern matches a basic JS object, but will fail on an object with
+ * nested objects. Used in JS file parsing for.

for.... what?

+++ b/includes/locale.incundefined
@@ -43,6 +43,37 @@ define('LOCALE_LANGUAGE_NEGOTIATION_SESSION', 'locale-session');
+ * Pattern to match a JS object containing a 'context key'
+ * with a string value, which is captured. Will fail if there

Very minor but these lines are way under 80 chars, would likely fit on two lines if they used the space better.

Otherwise looks good to me and thanks for the extensive tests!
-19 days to next Drupal core point release.

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
14.68 KB

Good call. That's what I get for working on a patch on a plane.

I made it actually say what that pattern is for, and made the other comment wrap at 80 so it is 2 lines instead of 3.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Passes again, fixed issues called out by @catch and otherwise universally liked, so should be good to go :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

Committed and pushed to 8.x, thanks! Moving to 7.x for consideration.

loganfsmyth’s picture

Issue tags: -msgctx, -Needs backport to D7, -D8MI

#33: js-t-context-488496-33.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +msgctx, +Needs backport to D7, +D8MI

The last submitted patch, js-t-context-488496-33.patch, failed testing.

Gábor Hojtsy’s picture

Added this change notice http://drupal.org/node/1323152, thanks!

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
15.15 KB

Rerolled against D7. Only changes from D8 version were adding back in textgroup='default' conditions.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Plugs in functionality we did not get to with Drupal 7.0. Expands tests nicely, so looks ready to go.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. This patch looks slightly terrifying. :) However, looking at the change notice, this is only adding new functionality to Drupal.t(), not changing any APIs, so should be good to go.

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-ui

Tagging for UI translation leg of D8MI.