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();
}
}
}
}
Comment | File | Size | Author |
---|---|---|---|
#39 | js-t-context-488496-39.patch | 15.15 KB | loganfsmyth |
#33 | js-t-context-488496-33.patch | 14.68 KB | loganfsmyth |
#28 | js-t-context-488496-28.patch | 14.66 KB | loganfsmyth |
#26 | js-t-context-488496-26.patch | 14.43 KB | loganfsmyth |
#25 | js-t-context-488496-25.patch | 14.43 KB | loganfsmyth |
Comments
Comment #1
andypostTagged to summarize all contexts
Comment #2
tstoecklerSubscribe.
Need this for the d7 module upgrade.
Comment #3
RobLoachWow, _locale_parse_js_file does look like a pile of gross.
Comment #4
andypostSuppose this issue should be critical because now this functionality is different from core's
Comment #5
cwgordon7 CreditAttribution: cwgordon7 commentedI can probably do this.
Comment #6
cwgordon7 CreditAttribution: cwgordon7 commentedOk, 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.
Comment #7
andypost@cwgordon7 what for you trying to add JS-objects? I think it could be just another param for Drupal.t() Please, provide some example
Comment #8
cwgordon7 CreditAttribution: cwgordon7 commented@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:
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.
Comment #9
cha0s CreditAttribution: cha0s commentedSub
Comment #10
littleviking001 CreditAttribution: littleviking001 commentedA 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.
Comment #11
pwolanin CreditAttribution: pwolanin commentedis this really critical?
Comment #12
ksenzeeI'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.
Comment #13
ksenzeeOops.
Comment #14
andypostSuppose this major and maybe critical for release because it's functionality exists in D6 but broken in D7
Actually this needs a regexp guru
Comment #15
Gábor HojtsyWell, 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.
Comment #16
Gábor HojtsyWell, Drupal 7 will not be consistent in context support, we can try in Drupal 8.
Comment #17
tstoecklerIs 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.
Comment #18
Gábor HojtsyTag for Drupal 8 Multilingual Initiative.
Comment #19
loganfsmyth CreditAttribution: loganfsmyth commentedComment #20
loganfsmyth CreditAttribution: loganfsmyth commentedHere 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.
Comment #22
loganfsmyth CreditAttribution: loganfsmyth commentedHere is a version that should pass testing :P Regular expressions are COMPLICATED.
Comment #23
loganfsmyth CreditAttribution: loganfsmyth commentedThis 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.
Comment #25
loganfsmyth CreditAttribution: loganfsmyth commentedHere 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.
Comment #26
loganfsmyth CreditAttribution: loganfsmyth commentedRerolled to match the new addition of locale_test.js in #1281932: Convert Drupal.t and Drupal.formatPlural regular expressions to extended form..
Should pass on 8.x.
Still waiting on #1281932: Convert Drupal.t and Drupal.formatPlural regular expressions to extended form. for 7.x.
Comment #27
Gábor HojtsyWow, 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.
Comment #28
loganfsmyth CreditAttribution: loganfsmyth commentedFixed those comments and added 3 more testing strings, with substitution arguments.
Comment #29
Gábor HojtsyLooks fabulous! I think if this passes tests, it should be good to go :)
Comment #30
tstoecklerThis should be backported, right?
Comment #31
Gábor HojtsyIt 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.
Comment #32
catchfor.... what?
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.
Comment #33
loganfsmyth CreditAttribution: loganfsmyth commentedGood 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.
Comment #34
Gábor HojtsyPasses again, fixed issues called out by @catch and otherwise universally liked, so should be good to go :)
Comment #35
catchCommitted and pushed to 8.x, thanks! Moving to 7.x for consideration.
Comment #36
loganfsmyth CreditAttribution: loganfsmyth commented#33: js-t-context-488496-33.patch queued for re-testing.
Comment #38
Gábor HojtsyAdded this change notice http://drupal.org/node/1323152, thanks!
Comment #39
loganfsmyth CreditAttribution: loganfsmyth commentedRerolled against D7. Only changes from D8 version were adding back in textgroup='default' conditions.
Comment #40
Gábor HojtsyLooks good to me. Plugs in functionality we did not get to with Drupal 7.0. Expands tests nicely, so looks ready to go.
Comment #41
webchickHm. 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!
Comment #43
Gábor HojtsyTagging for UI translation leg of D8MI.