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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

loganfsmyth’s picture

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

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.

loganfsmyth’s picture

Issue tags: +D8MI
Gábor Hojtsy’s picture

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

loganfsmyth’s picture

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.

Gábor Hojtsy’s picture

Superb, looking forward to testing results.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

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.

catch’s picture

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

loganfsmyth’s picture

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

Gábor Hojtsy’s picture

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

loganfsmyth’s picture

Status: Needs work » Needs review
FileSize
6.18 KB

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.

Gábor Hojtsy’s picture

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.

Dries’s picture

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

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

rfay’s picture

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
loganfsmyth’s picture

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

Gábor Hojtsy’s picture

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.

bfroehle’s picture

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

Gábor Hojtsy’s picture

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.

loganfsmyth’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7, +D8MI
loganfsmyth’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.18 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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!

Gábor Hojtsy’s picture

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.