This is a child task for #422380: Convert all core module to use new static caching API focusing on the locale module.

CommentFileSizeAuthor
#15 480424-15.patch3.1 KBJamesAn
#13 480424-13.patch3.1 KBJamesAn
#11 480424-11.patch3 KBJamesAn
#5 480424-5.patch2.99 KBJamesAn
#3 jamesan_480424-3.patch2.75 KBJamesAn
#1 jamesan_480424-1.patch1.82 KBJamesAn

Comments

JamesAn’s picture

JamesAn commented
Status: Active » Needs review
StatusFileSize
new jamesan_480424-1.patch1.82 KB

A wrapper function as per #422362-20: convert form.inc to use new static caching API was created to reset the static vars.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review
StatusFileSize
new jamesan_480424-3.patch2.75 KB

Oops. I missed changes in locale.test. All done.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review
StatusFileSize
new 480424-5.patch2.99 KB

I think I caught the bug this time. The static var, $locale_formula, in locale_get_plural() needed to be initialized to an array and the check had to be changed from !isset($locale_formula) to empty($locale_formula), as sometimes $locale_formula would be set to an empty array, which passed the isset() check.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Postponed

I'm getting strange test results.

The fails/exceptions don't correspond to any failed assertions. There's actually no corresponding message or assertion to the fails. I've filed an issue, #483486: Test returns 1 fail and 1 exception, but themes the result as passing (i.e. green), with simpletest in hopes that people more adept in simpletest can figure out what's going on.

I suppose this issue is postponed on account of that one.

damien tournoud’s picture

damien tournoud commented
Status: Postponed » Needs work

Reopening. The bug is in locale_get_plural(), in which two static are sharing the same key:

  $locale_formula = &drupal_static(__FUNCTION__, array());
  $plurals = &drupal_static(__FUNCTION__, array());
JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review
StatusFileSize
new 480424-11.patch3 KB

Oy vey.. thanks for catching that. I was breaking my head over this problem and didn't see it. ^^"

Done. testbot?

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review
StatusFileSize
new 480424-13.patch3.1 KB

Rerolled against #334283: Add msgctxt-type context to t(), which adds a context parameter to locale().

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review
StatusFileSize
new 480424-15.patch3.1 KB

Rerolled.

Re-test of 480424-15.patch from comment #15 was requested by Arancaytar.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Reviewed & tested by the community

This also looks good. I was wondering whether we need a "locale_reset()" one-line function for the only two times it is called, but from looking at the API that seems to be the proper pattern...

Status: Reviewed & tested by the community » Needs review

Re-test of 480424-15.patch from comment #15 was requested by webchick.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Reviewed & tested by the community

It seems this patch is still passing.

webchick’s picture

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

Thanks! Committed to HEAD.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Fixed » Needs work
Issue tags: +Perfomance

Suppose this change is not good! locale() is used very often so this change should be tested for performance!

If you wanna use drupal_static() so this should be $drupal_static_fast; http://api.drupal.org/api/function/drupal_static/7

alexanderpas’s picture

alexanderpas commented
Issue tags: -Perfomance +Performance

spelling

hgoto’s picture

hgoto commented
Issue summary: View changes
Status: Needs work » Closed (fixed)

I believe, this issue can be closed because #1387766: Use fast drupal static pattern for locale already introduced $drupal_static_fast pattern into locale() and fixed the point #21.

If you find something wrong, please re-open this.