Posted by Davy Van Den Bremt on October 13, 2008 at 5:20pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | locale.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | location performance improvements, Performance |
Issue Summary
... or somewhere else where $language hasn't been set yet.
I've noticed this on in Drupal 5 but I'm pretty sure it still exists in Drupal 7.
When a module calls the t() function when $locale hasn't been set yet (like in a define()), the locale cache gets rebuilt every page request. This can have a huge performance impact on a site with a lot of translated labels.
The function locale now does something like:
function locale($string = NULL, $langcode = NULL, $reset = FALSE) {
global $language;
static $locale_t;
if ($reset) {
// Reset in-memory cache.
$locale_t = NULL;
}
if (!isset($string)) {
// Return all cached strings if no string was specified
return $locale_t;
}
$langcode = isset($langcode) ? $langcode : $language->language;
// Store database cached translations in a static var.
if (!isset($locale_t[$langcode])) {I think it would be safer to check if $language is set in that latest if:
<code>
function locale($string = NULL, $langcode = NULL, $reset = FALSE) {
global $language;
static $locale_t;
if ($reset) {
// Reset in-memory cache.
$locale_t = NULL;
}
if (!isset($string)) {
// Return all cached strings if no string was specified
return $locale_t;
}
$langcode = isset($langcode) ? $langcode : $language->language;
// Store database cached translations in a static var.
if (!isset($locale_t[$langcode]) && isset($language)) {Okay, I admit every module should take care of calling t() only when locale is initialized, but this way we can make this a bit safer. Only the buggy t() call gets not translated and all the rest is translated well.
Comments
#1
BTW. You can find an example of this at http://drupal.org/node/320682.
This was a bug which was VERY hard to find.
#2
Well, the true answer is: *don't use t() in a global context*. Gabor lists this as one of the most common mistakes in his cheat sheet.
Requalifying as a documentation problem, the document of t() should be extended to include that in bold.
#3
Why not put the extra check there? It solves the performance issue gracefully.
#4
can we catch this with the coder module?
#5
To be honest. The impact on performance is so big, we need to do both imho. There are so many contributed modules available and if some novice users downloads them, he'll never notice this issue.
#6
Tagging
#7
If that is the patch, this is not a documentation issue.
#8
Title should reflect that as well. Is this accurate?
#9
Yes sir!
#10
+1
Patch applies nicely.
Don't see any need to rebuild locale cache when $language is not set
#11
This gets a +1 from me too. The change in the logic makes sense and the code change is clean and simple. Tests are passing as well, so I dare RTBC this.
Even though this could be seen as baby-sitting broken code, I don't think it is. Sanity checks are quite normal, and as this one prevents a pretty nasty slow-down, it's a good idea.
#12
The check needs to be on $langcode, not $language.
I suggest to make this check on is own, and return in this case.
#13
It's definitely on $language. That's not initialized yet. It's not a check on input parameters.
#14
The last submitted patch failed testing.
#15
#16
head broke -- resetting status
#17
Re-test of 320686-global-t.patch from comment #5 was requested by Arancaytar.
#18
Last test is over a month old. Otherwise, this is probably RTBC.
#19
The last submitted patch, 320686-global-t.patch, failed testing.
#20
Re-test of 320686-global-t.patch from comment #5 was requested by Arancaytar.
#21
I cannot reproduce the fatal error on DatabaseTaggingTestCase. Let's have another go, then investigate if it happens again.
#22
This is passing now, must have been broken test bot. I don't see any reason not to add the extra check here, IMO this doesn't fall into "dont' babysit broken code" - it makes sense in itself.
However can we get a one-line comment which explains what we're doing and why?
#23
Here's a comment.
#24
On second thought, that comment is silly. This one is hopefully a bit better.
#25
Very nice.
#26
Instead of 'var', let's use 'variable'. Should be quick re-roll.
#27
Done, and patched.
#28
Yep.
#29
Committed to HEAD!
#30
Automatically closed -- issue fixed for 2 weeks with no activity.