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/common.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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JamesAn’s picture

Status: Active » Needs review
FileSize
6.32 KB
JamesAn’s picture

Question:

drupal_add_js() has a reset input, but it's stored in $options['type']. No calls from core functions use the reset input, but I think it's appropriate to remove the reset parameter here too, right?

I can roll that change into the patch currently in this issue.

Berdir’s picture

@JamesAn.
Yes, I would say remove that too, we don't need it anymore.

pwolanin’s picture

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

FileSize
6.7 KB

I've removed the reset check in drupal_add_js(). No functions had more than one static variable, so the ':' suffix separator wasn't used here. Here's attempt #2.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

I would not make this change:

 function drupal_random_bytes($count)  {
-  static $random_state;
+  $random_state = &drupal_static(__FUNCTION__);

that static should never be reset.

pwolanin’s picture

Also, check w/ chx or boombatower about this one:

@@ -788,7 +788,7 @@ function _drupal_log_error($error, $fata
   // When running inside the testing framework, we relay the errors
   // to the tested site by the way of HTTP headers.
   if (preg_match("/^simpletest\d+/", $_SERVER['HTTP_USER_AGENT']) && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
-    static $number = 0;
+    $number = &drupal_static(__FUNCTION__, 0);

Likely you have to fix the test so it resets the js using the new method.

JamesAn’s picture

FileSize
7.64 KB
pwolanin’s picture

See #7 - that function should not be converted.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
7.27 KB

Oy vey. Sorry I missed that. This reflects #7 now.

pwolanin’s picture

Looks fine - looking at _drupal_log_error() I'm still not sure we want or need to convert that - but I'm not sure it makes much difference.

JamesAn’s picture

FileSize
6.79 KB

Oops.. this issue fell slightly by the wayside. I was unsure about the static $number in _drupal_log_error(); I've set it back to just a static var.

pwolanin’s picture

Can you add a brief code comment to the ones that are not converted?

JamesAn’s picture

FileSize
8.07 KB

Added comments. There are three instances of static vars here that don't use the new drupal_static method:

_drupal_log_error(): $number should never be reset as it counts the # of PHP errors and tags them for the simpletest framework
drupal_load_stylesheet(): $_optimize is set by optimize and doesn't have a separate reset need
drupal_random_bytes(): $random_state stores a string of random bytes that don't need to be reset

pwolanin’s picture

if the second param is always passed to drupal_load_stylesheet() then byassing it makes sense (and maybe it should jsut be a required param) - otherwise maybe it should be converted.

JamesAn’s picture

Status: Needs review » Needs work

I just realized some of the comments I made in the patch don't follow Drupal's comment formatting conventions. I'll fix those in the next patch after I figure out what's going on with drupal_load_stylesheet().

JamesAn’s picture

Status: Needs work » Needs review
FileSize
8.16 KB

Comments fixed. I didn't finish them with a period.
$_optimize of drupal_load_stylesheet() has been converted to use drupal_static.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review

Why did it fail? It passed twice before.. =\ Let's try again.

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

FileSize
8.16 KB

Updated patch against HEAD. #372563: Rename drupal_set_html_head()'s work broke the patch; here's the updated version.

JamesAn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

JamesAn’s picture

Status: Needs work » Needs review

..? It passed before.. =\

Retesting.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

patch fails to apply: Hunk #4 FAILED at 164.

function drupal_set_header() seems to have moved to bootstrap.inc

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.75 KB

re-roll just omitting that hunk, since the function in bootstrap.inc seems to be already converted.

pwolanin’s picture

Status: Needs review » Needs work

seems to be a problem in the code here:

 function drupal_load_stylesheet($file, $optimize = NULL) {
-  static $_optimize;
+  // $_optimize does not use drupal_static as it is set by $optimize.
+  $_optimize = &drupal_static(__FUNCTION__);
JamesAn’s picture

Status: Needs work » Needs review
FileSize
5.53 KB

6 more hunks have failed as those changes seem to already have been made.

I've fixed the $_optimize comment mismatch, restoring it back to static $_optimize;.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

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

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