convert locale.inc to use new static caching API [#422364]

Comment	File	Size	Author
#12	jamesan_422364-12.patch	2.64 KB	JamesAn

#9	jamesan_422364-9.patch	1.36 KB	JamesAn

#6	jamesan_422364-3.patch	1.36 KB	JamesAn

#3	jamesan_422364-2.patch	1.36 KB	JamesAn

#1	jamesan_422364.patch	1.33 KB	JamesAn

Comment #1

JamesAn CreditAttribution: JamesAn commented 3 April 2009 at 05:13

Status:

Active

» Needs review

File	Size
jamesan_422364.patch	1.33 KB

Let's give this static catching change a shot!

Log in or register to post comments

Comment #2

3 April 2009 at 05:30

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Log in or register to post comments

Comment #3

JamesAn CreditAttribution: JamesAn commented 3 April 2009 at 05:30

File	Size
jamesan_422364-2.patch	1.36 KB

Bah. I misunderstood; I was wondering why the first parameter was always __FUNCTION__ and then realized, that was just to ensure the parameter was unique. So functions with multiple static vars need to rename subsequent strings sent into drupal_static. I gets it now!

Log in or register to post comments

Comment #4

JamesAn CreditAttribution: JamesAn commented 3 April 2009 at 05:32

Status:

Needs work

» Needs review

Gah. Forgot to flip the switch.

Log in or register to post comments

Comment #5

pwolanin CreditAttribution: pwolanin commented 3 April 2009 at 15:49

Per discussion w/ catch - if you have a variable name more complex than just __FUNCTION__, use a ':' to separate the suffix (this will avoid colliding with any other valid function name). e.g. :

&drupal_static(__FUNCTION__ . ':second_var');

Log in or register to post comments

Comment #6

JamesAn CreditAttribution: JamesAn commented 3 April 2009 at 16:35

File	Size
jamesan_422364-3.patch	1.36 KB

Gotcha. Here's attempt #3!

Log in or register to post comments

Comment #7

Dries CreditAttribution: Dries commented 3 April 2009 at 17:28

The default value of drupal_static() is NULL so you don't need to pass that in explicitly. See the last chunk of your patch.

Log in or register to post comments

Comment #8

pwolanin CreditAttribution: pwolanin commented 3 April 2009 at 17:28

Per IRC discussion, in this case I might write something like this, since the logic suggests there is some decoupling between header and string processing, but we want to reset strings and report at the same time.

function _locale_import_one_string($op, $value = NULL, $mode = NULL, $lang = NULL, $file = NULL, $group = 'default') {
  $report = &drupal_static(__FUNCTION__);
  $strings = &drupal_static(__FUNCTION__ . ':strings');
  $headerdone = &drupal_static(__FUNCTION__ . ':headerdone', FALSE);

  if (!isset($report)) {
    // Either the first invocation, or after a reset.
    $report = array('additions' => 0, 'updates' => 0, 'deletes' => 0, 'skips' => 0);
    $strings = array();
  }

  ...

Log in or register to post comments

Comment #9

JamesAn CreditAttribution: JamesAn commented 3 April 2009 at 17:59

File	Size
jamesan_422364-9.patch	1.36 KB

@Dries: Thanks for pointing that out. It's fixed.

@pwolanin: I don't know. The logic and purpose of $strings and $report seem pretty distinct to me. I don't think it'd be good to group them together like that as it creates an exceptional case where drupal_static_reset() will reset two variables, but not the third.

There should be a way to either reset specific vars (as it is now) or bulk-reset all vars of a function. Resetting some and not others seems a bit too ad-hoc, in my opinion. yes/no?