Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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)
Comment | File | Size | Author |
---|---|---|---|
#30 | jamesan_422358-30.patch | 5.53 KB | JamesAn |
#28 | common-inc-static-422358-28.patch | 7.75 KB | pwolanin |
#22 | jamesan_422358-22.patch | 8.16 KB | JamesAn |
#18 | jamesan_422358-18.patch | 8.16 KB | JamesAn |
#15 | jamesan_422358-15.patch | 8.07 KB | JamesAn |
Comments
Comment #1
JamesAn CreditAttribution: JamesAn commentedComment #2
JamesAn CreditAttribution: JamesAn commentedQuestion:
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.
Comment #3
Berdir@JamesAn.
Yes, I would say remove that too, we don't need it anymore.
Comment #4
pwolanin CreditAttribution: pwolanin commentedPer 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. :
Comment #5
JamesAn CreditAttribution: JamesAn commentedI'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.
Comment #7
pwolanin CreditAttribution: pwolanin commentedI would not make this change:
that static should never be reset.
Comment #8
pwolanin CreditAttribution: pwolanin commentedAlso, check w/ chx or boombatower about this one:
Likely you have to fix the test so it resets the js using the new method.
Comment #9
JamesAn CreditAttribution: JamesAn commentedComment #10
pwolanin CreditAttribution: pwolanin commentedSee #7 - that function should not be converted.
Comment #11
JamesAn CreditAttribution: JamesAn commentedOy vey. Sorry I missed that. This reflects #7 now.
Comment #12
pwolanin CreditAttribution: pwolanin commentedLooks 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.
Comment #13
JamesAn CreditAttribution: JamesAn commentedOops.. 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.
Comment #14
pwolanin CreditAttribution: pwolanin commentedCan you add a brief code comment to the ones that are not converted?
Comment #15
JamesAn CreditAttribution: JamesAn commentedAdded 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
Comment #16
pwolanin CreditAttribution: pwolanin commentedif 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.
Comment #17
JamesAn CreditAttribution: JamesAn commentedI 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().
Comment #18
JamesAn CreditAttribution: JamesAn commentedComments fixed. I didn't finish them with a period.
$_optimize of drupal_load_stylesheet() has been converted to use drupal_static.
Comment #20
JamesAn CreditAttribution: JamesAn commentedWhy did it fail? It passed twice before.. =\ Let's try again.
Comment #22
JamesAn CreditAttribution: JamesAn commentedUpdated patch against HEAD. #372563: Rename drupal_set_html_head()'s work broke the patch; here's the updated version.
Comment #23
JamesAn CreditAttribution: JamesAn commentedComment #25
JamesAn CreditAttribution: JamesAn commented..? It passed before.. =\
Retesting.
Comment #27
pwolanin CreditAttribution: pwolanin commentedpatch fails to apply: Hunk #4 FAILED at 164.
function drupal_set_header() seems to have moved to bootstrap.inc
Comment #28
pwolanin CreditAttribution: pwolanin commentedre-roll just omitting that hunk, since the function in bootstrap.inc seems to be already converted.
Comment #29
pwolanin CreditAttribution: pwolanin commentedseems to be a problem in the code here:
Comment #30
JamesAn CreditAttribution: JamesAn commented6 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;
.Comment #31
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!