follow-up to: #254491: Standardize static caching

see last patch examples: http://drupal.org/node/254491#comment-1430180 and also: http://drupal.org/node/224333#static_variable_api

Apply this conversion pattern to includes/locale.inc to convert all static variables there. Pay close attention to any place a reset parameter is provided and add a call to drupal_static_reset() where appropriate (e.g. in any calling function that uses the reset parameter)

CommentFileSizeAuthor
#12 jamesan_422364-12.patch2.64 KBJamesAn
#9 jamesan_422364-9.patch1.36 KBJamesAn
#6 jamesan_422364-3.patch1.36 KBJamesAn
#3 jamesan_422364-2.patch1.36 KBJamesAn
#1 jamesan_422364.patch1.33 KBJamesAn

Comments

JamesAn’s picture

JamesAn commented
Status: Active » Needs review
StatusFileSize
new jamesan_422364.patch1.33 KB

Let's give this static catching change a shot!

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

JamesAn commented
StatusFileSize
new jamesan_422364-2.patch1.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!

JamesAn’s picture

JamesAn commented
Status: Needs work » Needs review

Gah. Forgot to flip the switch.

pwolanin’s picture

pwolanin commented

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');
JamesAn’s picture

JamesAn commented
StatusFileSize
new jamesan_422364-3.patch1.36 KB

Gotcha. Here's attempt #3!

dries’s picture

dries commented

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.

pwolanin’s picture

pwolanin commented

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();
  }

  ...
JamesAn’s picture

JamesAn commented
StatusFileSize
new jamesan_422364-9.patch1.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?

dries’s picture

dries commented

While we are at it, we might want to rename 'headerdone' -- we don't usually glue variable names together.

pwolanin’s picture

pwolanin commented

@JamesAn - either way is ok for me - this function doesn't have any reset functionality currently, so no real expectation to meet.

JamesAn’s picture

JamesAn commented
StatusFileSize
new jamesan_422364-12.patch2.64 KB

headerdone changed to header_done.

@pwolanin, yea, reset doesn't happen there.

pwolanin’s picture

pwolanin commented

Looks good

pwolanin’s picture

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

Since all tests pass and code style looks fine, I think it's good enough.

dries’s picture

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

This looks like proper use of the static caching API. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

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

Issue tags: +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.