Locale uses a huge regular expression to look for calls to Drupal.t and Drupal.formatPlural, in order know what translations to load on page-view.
I am converting them to extended form.

Files: 
CommentFileSizeAuthor
#22 locale-regex-extended-1281932-11.patch6.18 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 36,641 pass(es).
[ View ]
#17 locale-regex-extended-1281932-17.patch1.06 KBbfroehle
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-regex-extended-1281932-17.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#11 locale-regex-extended-1281932-11.patch6.18 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 36,628 pass(es).
[ View ]
#4 locale-regex-extended-1281932-4.patch6.58 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 33,020 pass(es).
[ View ]
#1 locale-regex-extended-1281932-1.patch3.08 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 32,940 pass(es).
[ View ]

Comments

Assigned:loganfsmyth» Unassigned
Status:Active» Needs review
StatusFileSize
new3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 32,940 pass(es).
[ View ]

This patch converts the regexes to extended form and adds tons of comments so you can actually tell what's going on.

This also applies to 7.x.

Issue tags:+D8MI

As I've said in person, this looks so much better. It is way cleaner to understand what is going on here. Do tests cover this already? If that is the case, this looks like just ready to go :)

StatusFileSize
new6.58 KB
PASSED: [[SimpleTest]]: [MySQL] 33,020 pass(es).
[ View ]

I couldn't find any tests and totally breaking the regex didn't cause any failed tests, so I don't think there are any.

Here is a new patch with the same regex, and some new tests to make sure parsing is working.

Superb, looking forward to testing results.

Status:Needs review» Reviewed & tested by the community

Looks good, test looks pretty comprehensive and passes :) Thanks for your work.

The time increase in what takes PCRE to parse the string compared to parsing the string and throwing away whitespace and comments is barely measurable compared to actually running the PCRE state machine. Edit: I microbenchmarked this and on my machine 10 000 runs adds 0.02s time to the execution of the PCRE so every run we are talking of two microseconds. We can tolerate a 2us slowdown for readability.

Status:Reviewed & tested by the community» Needs work

+    file_put_contents($filename, $contents);

I don't see any reason to create the file in the test rather than just ship it inside locale/tests - also not 100% but does simpletest even mess with the file directory during test runs or is this something which would have to be cleared up manually (which isn't done by the test if so).

If just throwing a random file in the tests directory is acceptable, then I can do that. I mostly wasn't sure if that was a good way to do it, or if I should keep the test more self-contained like this.

I used the file_put_contents method because it seemed to work, and I assumed that it would get cleaned up because this method is also used in this: http://api.drupal.org/api/drupal/modules--simpletest--simpletest.module/...

@loganfsmyth: the direct backportable solution IMHO would be to put a locale.test.js or locale_test.js or somesuch in modules/locale/. Not sure what's the pattern, locale.test.js or locale_test.js though. Probably other tests have js test files, for aggregation testing for example to look for naming patterns.

Status:Needs work» Needs review
StatusFileSize
new6.18 KB
PASSED: [[SimpleTest]]: [MySQL] 36,628 pass(es).
[ View ]

I moved the JavaScript to a file called 'locale_test.js' in the locale/tests folder.
I also added one last test to make sure the total number of source strings found matches the expected value.

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D7

Looks good to me. Also, this should be backported to D7 (should apply direct). It is just about making the JS parser code more understandable and adding a heap of tests to verify.

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

Committed to 8.x. Moving to 7.x for Angie to consider.

Title:Convert Drupal.t and Drupal.formatPlural regular expressions to extended form.[8.x broken] Convert Drupal.t and Drupal.formatPlural regular expressions to extended form.
Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs work

It looks to me like this broke 8.x; most likely there were missing files that didn't get committed.

http://qa.drupal.org/8.x-status

file_get_contents(modules/locale/tests/locale_test.js): failed to open stream: No such file or directory

It looks like the patch wasn't properly applied.
You are right Randy, locale_test.js was in my patch, but wasn't committed.

Priority:Normal» Critical
Status:Needs work» Reviewed & tested by the community

It is critical if its broke. We also have the file to commit, so RTBC.

StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch locale-regex-extended-1281932-17.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

I've attached the modules/locale/tests/locale_test.js hunk from the patch in #11 for ease.

Title:[8.x broken] Convert Drupal.t and Drupal.formatPlural regular expressions to extended form.Convert Drupal.t and Drupal.formatPlural regular expressions to extended form.
Version:8.x-dev» 7.x-dev
Category:bug» task
Priority:Critical» Normal

All right, @Dries added the file in. The D7 patch is in #11. :)
Edit: commit link at http://drupalcode.org/project/drupal.git/commitdiff/3c536d68db761d7a9e60...

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D7, -D8MI

The last submitted patch, locale-regex-extended-1281932-17.patch, failed testing.

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

Status:Needs review» Reviewed & tested by the community

Testing success on D7. Setting RTBC again as Gábor had it.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new6.18 KB
PASSED: [[SimpleTest]]: [MySQL] 36,641 pass(es).
[ View ]

Uploading again to make it clear this patch is for D7. Should be RTBC if tests pass, just like D8.

Status:Needs review» Reviewed & tested by the community

Should be good to go (not my patch, I just reposted it above :).

Status:Reviewed & tested by the community» Fixed

Seems like this is no functional change, just makes everything a hell of a lot easier to understand and also adds more tests which is always a good thing. :)

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)

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

Issue tags:+language-ui

Tagging for UI translation.