| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed |
Since #254491-108: Standardize static caching we lost the version which stored the defaults and drupal_static_reset only resets inside drupal_static and does not affect the function that stores a reference to the drupal_static storage. The only reason we see drupal_static_reset working is that after the reset, we unconditionally call drupal_static with the default we want and because inside drupal_static the storage is now unset, the default gets stored. However, if the default would be a costly affair to calculate and we would take to care to pass it to drupal_static once, expecting it to hold for us, things would break. Or if a genius like Casey (really nice job!) wants to make sure we call drupal_static only once as in #619666: Introduce new pattern for drupal_static() in performance-critical functions then suddenly things fall apart. I am attaching a patch that was RTBC'd in the original issue, carries the same amount of lines of code and works a hell lot better by storing the default and doing a real reset. Memory issues were cited -- given that the default is mostly just array() or FALSE, that's not a really good argument compared to the fact that the memory saving version is broken.
#1
The last submitted patch failed testing.
#2
Sorry for the patch mixup.
#3
The last submitted patch failed testing.
#4
catch gave me an important hint to what happens here. Thanks!
#5
The last submitted patch failed testing.
#6
Yeah, yeah, if you reset before drupal_static is called w a default then now you get a notice...
#7
The last submitted patch failed testing.
#8
Hm, right.
#9
The last submitted patch failed testing.
#10
OK, no more trying to fix. I rewrote it. The code flow now is a lot clearer, too. First we handle the reset-all case, then the reset-one case and explicitly deal with the "reset called before we have a default" case.
#11
If this would be wrong, then a lot of tests would fail. So it seems this is almost RTBC.
+++ includes/bootstrap.inc 2009-11-07 09:30:56 +0000@@ -2066,31 +2066,34 @@ function registry_rebuild() {
if ($reset) {
...
+ if (array_key_exists($name, $default)) {
+ $data[$name] = $default[$name];
}
else {
...
+ // Reset was called before a default is set and yet a variable must be
+ // returned.
+ return $data;
+ }
}
Could we add some inline comments here as well?
(Please also note the trailing whites-pace.)
+++ includes/bootstrap.inc 2009-11-07 09:30:56 +0000@@ -2066,31 +2066,34 @@ function registry_rebuild() {
+ elseif (!array_key_exists($name, $data)) {
+ $default[$name] = $data[$name] = $default_value;
}
Inline comments here, too, as this seems to be the main static/reference handling.
I'm on crack. Are you, too?
#12
Added even more comments. It's now (way) more comments than code.
#13
Alright, I'm more happy with this.
#14
Looks good to me too. Committed.
#15
drupal_static_reset() is still broken when called globally (without passing a $name parameter). This was the case prior to this issue, but I'm attaching the patch here, since it's related to this issue. The bug hasn't manifested yet, because we have no places within core where drupal_static_reset() is called without passing $name. This patch includes a test that fails unless this patch is applied.
#16
Nice! But can't you simply do
foreach ($default as $name => $value) $data[$name] = $value;?#17
Yes.
#18
Cross-linking this to #601584: setUp() function for unit and web test cases should reset all resettable statics, because that issue now includes this same fix. If that issue lands first, then this one can be marked "fixed" again as per #14. On the other hand, if this issue lands first, then that one will need to be re-rolled.
#19
Thanks.
#20
Committed to HEAD with some minor comment/whitespace clean-up.
#21
Awesome! So, now that global drupal_static_reset() isn't broken, can anyone here help get #601584: setUp() function for unit and web test cases should reset all resettable statics to RTBC level?
#22
Automatically closed -- issue fixed for 2 weeks with no activity.