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.
Comment | File | Size | Author |
---|---|---|---|
#22 | locale-regex-extended-1281932-11.patch | 6.18 KB | Gábor Hojtsy |
#17 | locale-regex-extended-1281932-17.patch | 1.06 KB | bfroehle |
#11 | locale-regex-extended-1281932-11.patch | 6.18 KB | loganfsmyth |
#4 | locale-regex-extended-1281932-4.patch | 6.58 KB | loganfsmyth |
#1 | locale-regex-extended-1281932-1.patch | 3.08 KB | loganfsmyth |
Comments
Comment #1
loganfsmyth CreditAttribution: loganfsmyth commentedThis 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.
Comment #2
loganfsmyth CreditAttribution: loganfsmyth commentedComment #3
Gábor HojtsyAs 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 :)
Comment #4
loganfsmyth CreditAttribution: loganfsmyth commentedI 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.
Comment #5
Gábor HojtsySuperb, looking forward to testing results.
Comment #6
Gábor HojtsyLooks good, test looks pretty comprehensive and passes :) Thanks for your work.
Comment #7
chx CreditAttribution: chx commentedThe 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.
Comment #8
catchI 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).
Comment #9
loganfsmyth CreditAttribution: loganfsmyth commentedIf 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/...
Comment #10
Gábor Hojtsy@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.
Comment #11
loganfsmyth CreditAttribution: loganfsmyth commentedI 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.
Comment #12
Gábor HojtsyLooks 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.
Comment #13
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 7.x for Angie to consider.
Comment #14
rfayIt 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
Comment #15
loganfsmyth CreditAttribution: loganfsmyth commentedIt looks like the patch wasn't properly applied.
You are right Randy, locale_test.js was in my patch, but wasn't committed.
Comment #16
Gábor HojtsyIt is critical if its broke. We also have the file to commit, so RTBC.
Comment #17
bfroehle CreditAttribution: bfroehle commentedI've attached the modules/locale/tests/locale_test.js hunk from the patch in #11 for ease.
Comment #18
Gábor HojtsyAll right, @Dries added the file in. The D7 patch is in #11. :)
Edit: commit link at http://drupalcode.org/project/drupal.git/commitdiff/3c536d68db761d7a9e60...
Comment #20
loganfsmyth CreditAttribution: loganfsmyth commented#11: locale-regex-extended-1281932-11.patch queued for re-testing.
Comment #21
loganfsmyth CreditAttribution: loganfsmyth commentedTesting success on D7. Setting RTBC again as Gábor had it.
Comment #22
Gábor HojtsyUploading again to make it clear this patch is for D7. Should be RTBC if tests pass, just like D8.
Comment #23
Gábor HojtsyShould be good to go (not my patch, I just reposted it above :).
Comment #24
webchickSeems 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!
Comment #25
Gábor HojtsySuperb, thanks, now onwards to #488496: Implement missing msgctx context in JS translation for feature parity with t() :)
Comment #27
Gábor HojtsyTagging for UI translation.