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)

Files: 
CommentFileSizeAuthor
#30 jamesan_422358-30.patch5.53 KBJamesAn
Passed: 11513 passes, 0 fails, 0 exceptions View
#28 common-inc-static-422358-28.patch7.75 KBpwolanin
Failed: Failed to apply patch. View
#22 jamesan_422358-22.patch8.16 KBJamesAn
Failed: Failed to apply patch. View
#18 jamesan_422358-18.patch8.16 KBJamesAn
Failed: Failed to apply patch. View
#15 jamesan_422358-15.patch8.07 KBJamesAn
Failed: Failed to apply patch. View
#13 jamesan_422358-13.patch6.79 KBJamesAn
Failed: Failed to apply patch. View
#11 jamesan_422358-11.patch7.27 KBJamesAn
Failed: Failed to apply patch. View
#9 jamesan_422358-9.patch7.64 KBJamesAn
Failed: Failed to apply patch. View
#5 jamesan_422358-2.patch6.7 KBJamesAn
Failed: 10687 passes, 1 fail, 0 exceptions View
#1 jamesan_422358.patch6.32 KBJamesAn
Failed: Failed to apply patch. View

Comments

JamesAn’s picture

Status: Active » Needs review
FileSize
6.32 KB
Failed: Failed to apply patch. View
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
Failed: 10687 passes, 1 fail, 0 exceptions View

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
Failed: Failed to apply patch. View
pwolanin’s picture

See #7 - that function should not be converted.

JamesAn’s picture

Status: Needs work » Needs review
FileSize
7.27 KB
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Failed: Failed to apply patch. View

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
Passed: 11513 passes, 0 fails, 0 exceptions View

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.