Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.t() currently uses function_exists('locale') to determine if the locale module is enabled. This is not robust enough: it breaks simpletest is some edge cases, for example when trying to run tests from a parent site with the locale module active *and* a non-English default language.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 407256-we-want-robust-t.patch | 874 bytes | Damien Tournoud |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHopefully, it is easy enough to fix.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedWrong order in my patch stack.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedDear Damien, please be more careful with your merges.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commented:(
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedHarmless for sure. And if you say it helps testing, then in it goes.
Comment #9
sunThe reason for using function_exists() is that the localization system is pluggable. Locale module is just one of many possible implementations.
This patch at the very least needs Gábor's approval.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis way of making pluggable things is deprecated and incompatible with the registry. Even if there is no replacement for now (waiting for something like the handler patch), I believe we can already get rid of that, especially because I believe no contrib module actually uses this "feature".
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedagreed.
Comment #12
sunI'd say it should be drupal_function_exists() instead.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo. Using
module_exists()here is designed to make the whole stuff more robust to the testing framework, in which a function can be loaded (because locale.module was loaded at one point) without the module being actually enabled.Comment #14
webchickHm. This patch makes http://drupal.org/project/stringoverrides module impossible, and represents a regression in functionality.
I would prefer to fix this in SimpleTest module than remove this. What exactly is the issue?
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedstringoverrides is nothing more then an interface around locale_custom_strings_* and locale_disabled_strings_*. It doesn't use the broken feature that is removed here. As I stated in #10, I know no contribution module that uses that.
Comment #16
Dries CreditAttribution: Dries commentedPer #9, I've asked Gabor to comment on this issue, and will pull the trigger if he gives me a 'yay'.
Comment #17
Gábor HojtsyThe reason we had function_exists() there was that we did not necessarily tie this to locale module but to any module implementing the locale() function. This patch basically removes that flexibility. I don't know of any module which would use this feature however, so it could be safe to do. Otherwise, it would be good to document why are we doing a module_exists(). We usually use drupal_function_exists() as sun pointed out, and others will point this out and/or try to "fix" this if this is not documented right. We should say that this is dependent on the module being turned on and the locale() function is used from there.
Comment #18
Dries CreditAttribution: Dries commentedLet's document it better in the code, and then commit it to CVS HEAD. Thanks!
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is the expected reroll, with the documentation fix.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commenteddocs are in.
Comment #21
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #23
catchWe just reverted this in #523694: Speed up t() five times when xdebug profiling is enabled., re-opening this one since the simpletest issue will remain.
This fix made t() quite a bit slower - we replaced a single function_exists() with module_exists(), module_list(), and array_key_exists() which has a measurable performance impact in HEAD.
edit: also it looks like someone reverted the extra documentation in the interim - wasn't there when I was patching last night, so only found this issue when chx hinted at it after commit.
Comment #24
plachCleaning-up the "language system" issue queue as per http://cyrve.com/criticals.
Comment #25
marvil07 CreditAttribution: marvil07 commentedI am wondering if the patch at #523694-14: Speed up t() five times when xdebug profiling is enabled. actually solves the original issue(parent site with locale enabled) here.
Original comment there mentions:
but I guess that's about the other issue, but it could solve this one, am I right?
PS: moving to 8.x since AFAIK the change needs to be done there first.
Comment #26
jhedstromThe
t()function and the underlying subsystem has evolved much in the past 4 years. Is there anything left to be done here?Comment #40
pameeela CreditAttribution: pameeela commented@Damien Tournoud @jhedstrom I don't know enough about multilingual to say, but it seems likely this can be closed, given the comment at #26 and lack of follow up?
Comment #41
pameeela CreditAttribution: pameeela commentedComment #42
pameeela CreditAttribution: pameeela commentedClosing since no update, feel free to set back to active if this is a mistake.
Comment #43
marvil07 CreditAttribution: marvil07 as a volunteer commented@pameeela Thanks for taking care of this old bug!
The change was actually added to D8, but later code changed, so I am moving it to fixed, just for archival reasons.