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.

CommentFileSizeAuthor
#19 407256-we-want-robust-t.patch874 bytesdamien tournoud
#7 407256-we-want-robust-t.patch680 bytesdamien tournoud
#5 407256-we-want-robust-t.patch691 bytesdamien tournoud
#3 407256-we-want-robust-t.patch691 bytesdamien tournoud
#1 407256-we-want-robust-t.patch780 bytesdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
Issue tags: +Needs backport to D6
StatusFileSize
new 407256-we-want-robust-t.patch780 bytes

Hopefully, it is easy enough to fix.

Status: Needs review » Needs work

The last submitted patch failed testing.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 407256-we-want-robust-t.patch691 bytes

Wrong order in my patch stack.

Status: Needs review » Needs work

The last submitted patch failed testing.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 407256-we-want-robust-t.patch691 bytes

Dear Damien, please be more careful with your merges.

Status: Needs review » Needs work

The last submitted patch failed testing.

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 407256-we-want-robust-t.patch680 bytes

:(

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Harmless for sure. And if you say it helps testing, then in it goes.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Component: base system » language system
Status: Reviewed & tested by the community » Needs review

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

damien tournoud’s picture

damien tournoud commented

This 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".

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

agreed.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'd say it should be drupal_function_exists() instead.

damien tournoud’s picture

damien tournoud commented

I'd say it should be drupal_function_exists() instead.

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

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Hm. 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?

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

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

dries’s picture

dries commented

Per #9, I've asked Gabor to comment on this issue, and will pull the trigger if he gives me a 'yay'.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

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

dries’s picture

dries commented

Let's document it better in the code, and then commit it to CVS HEAD. Thanks!

damien tournoud’s picture

damien tournoud commented
Status: Needs work » Needs review
StatusFileSize
new 407256-we-want-robust-t.patch874 bytes

Here is the expected reroll, with the documentation fix.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

docs are in.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Closed (fixed) » Active

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

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Component: language system » locale.module

Cleaning-up the "language system" issue queue as per http://cyrve.com/criticals.

marvil07’s picture

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

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

static needs resetting both ways, same problem as the original patch.

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.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue summary: View changes
Status: Active » Postponed (maintainer needs more info)

The t() function and the underlying subsystem has evolved much in the past 4 years. Is there anything left to be done here?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed 435c939 on 8.3.x 
    - Patch #407256 by Damien Tournoud: make t() more robust.

  • Dries committed 435c939 on 8.3.x 
    - Patch #407256 by Damien Tournoud: make t() more robust.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • Dries committed 435c939 on 8.4.x 
    - Patch #407256 by Damien Tournoud: make t() more robust.

  • Dries committed 435c939 on 8.4.x 
    - Patch #407256 by Damien Tournoud: make t() more robust.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • Dries committed 435c939 on 9.1.x 
    - Patch #407256 by Damien Tournoud: make t() more robust.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pameeela’s picture

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?

pameeela’s picture

pameeela commented
Issue tags: +Bug Smash Initiative
pameeela’s picture

pameeela commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Closing since no update, feel free to set back to active if this is a mistake.

marvil07’s picture

marvil07 commented
Status: Closed (outdated) » Fixed

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

Status: Fixed » Closed (fixed)

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