Posted by pwolanin on April 3, 2009 at 2:51am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
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)
Comments
#1
#2
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.
#3
@JamesAn.
Yes, I would say remove that too, we don't need it anymore.
#4
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');#5
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.
#6
The last submitted patch failed testing.
#7
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.
#8
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.
#9
#10
See #7 - that function should not be converted.
#11
Oy vey. Sorry I missed that. This reflects #7 now.
#12
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.
#13
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.
#14
Can you add a brief code comment to the ones that are not converted?
#15
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
#16
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.
#17
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().
#18
Comments fixed. I didn't finish them with a period.
$_optimize of drupal_load_stylesheet() has been converted to use drupal_static.
#19
The last submitted patch failed testing.
#20
Why did it fail? It passed twice before.. =\ Let's try again.
#21
The last submitted patch failed testing.
#22
Updated patch against HEAD. #372563: Rename drupal_set_html_head()'s work broke the patch; here's the updated version.
#23
#24
The last submitted patch failed testing.
#25
..? It passed before.. =\
Retesting.
#26
The last submitted patch failed testing.
#27
patch fails to apply: Hunk #4 FAILED at 164.
function drupal_set_header() seems to have moved to bootstrap.inc
#28
re-roll just omitting that hunk, since the function in bootstrap.inc seems to be already converted.
#29
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__);
#30
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;.#31
Committed to CVS HEAD. Thanks!
#32
Automatically closed -- issue fixed for 2 weeks with no activity.