Two functions, mostly notably user_access(), rely on unset() to clear their internal cache when a certain condition is met. However, unset() does not perform as expected when the thing being cleared is a static. According to the PHP function docs, "If a static variable is unset() inside of a function, unset() destroys the variable only in the context of the rest of a function. Following calls will restore the previous value of a variable."
Take user_access() as an example. As a result of the above oddity, when user_access() is first called with $reset==TRUE, the cache is cleared, and the data is regenerated. But the next time user_access() is called, it reuses the data from before the cache was cleared.
The attached patch is a way around this problem. In the case of user_access(), it simply replaces the unset() with an empty array assignment. The other case, involving module_list() has a similar fix. I scoured the entire source tree, and these were the only two cases of this problem I could find.
Comment | File | Size | Author |
---|---|---|---|
#28 | module-list-static-D6.patch | 683 bytes | vaish |
#22 | 275796-module-list-static.patch | 3.67 KB | Damien Tournoud |
#19 | 275796-module-list-static.patch | 4.07 KB | Damien Tournoud |
#17 | 275796-module-list-static.patch | 4.15 KB | Damien Tournoud |
#9 | 275796-module-list-static-D7.patch | 3.54 KB | Dave Reid |
Comments
Comment #1
chuckdeal97 CreditAttribution: chuckdeal97 commentedIssue #300906: Unset on static variable in user_access might not actually reset permissions cache. is addressing this same problem.
Comment #2
dergachev CreditAttribution: dergachev commentedI'm using this patch to fix a serious bug in Login Toboggan. See http://drupal.org/node/319438#comment-1112176
Comment #3
Dave ReidPlease help fix in 7.x first, then we can backport. Also be sure to check out http://drupal.org/patch/create.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe user_access fix was a duplicate of #329646: user_access() don't reset correctly if called with uid = 1. The module_list fix is still valid.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a proper patch for D7, including a test case.
Please credit Gribnif for the discovery.
Comment #6
Dries CreditAttribution: Dries commentedThe test code is interesting in that it exposes some "API roughness" in my opinion. For example,
drupal_install_modules(array('system_test'))
looks a bit weird and suggests that the function should take a single module as input. Similarly, the way you have to specify the$fixed_list
is somewhat heavy. Not things we should fix (assuming they can be fixed!), but it is interesting nonetheless.Other than that, this patch looks good to me. :)
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commented@Dries: one of the key advantage of the testing initiative, in my opinion, is precisely that it forces us to correctly specify and use our API. There are way to many issues in which people battle over behavior, each side calling a "bug" the behavior described by the other side. Let's hope we will have less of them when Drupal 7 will reach its maintenance phase.
Comment #8
Dave ReidDon't commit yet please...I'm revising the test.
Comment #9
Dave ReidConfirmed that the patch fixes the bug in module_list(). I rewrote the test to fix a few spelling errors and bring it down to a more reasonable size. Uses assertEqual, which accomplishes the same thing as the custom assertSameValues with arrays. I tested it on a lot of arrays just to make sure.
Comment #11
Dave ReidWell, sorry Damz, this passed for me on my system.
Comment #12
Dave ReidTesting bot failure...
Comment #13
Dave ReidComment #14
Dries CreditAttribution: Dries commentedDave, I'm not sure your patch brings it down to a more reasonable size. In fact, your patch seems to be more lines (but smaller in size)? I've been staring at the difference and I have no strong preferences. Is
assertSameValues()
more correct thanassertEqual()
?Comment #15
Dave ReidassertEqual when run with arrays tests for same key/value pairs. They just don't have to be in the same order. assertIdentical will check if they have the same key/value pairs and they're in the same order. I just didn't know why we needed a whole new function just to use assertEqual.
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet me work on a better version.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere we are, with a more throughout test.
Comment #18
Dave ReidFirst quick look, the following code can be removed since it has no purpose:
I'll re-test and everything when I get back from my interview today.
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commented@Dave, good catch. Here is a reroll.
Comment #20
Dries CreditAttribution: Dries commentedPatch looks good, but as I said above, lots of API clean-up that could be done here. I'd like to ask Dave to review the patch one more time, and to mark it RTBC. Thanks!
Comment #21
Dave Reid+ * Unit tests for the module PAI.
Still misspelled. :)
+ protected $moduleList;
Should this have a default value of array()?
What is the purpose of the test's code in setUp() or the protected class variable? None of it is used by the new, added test, but if it's intended to be re-used with more (future) tests, that's fine. Right now I don't see any use for it.
+ protected function assertModuleList(Array $expected_values, $condition)
The type-hint should be lowercase.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedTook care of all this. I think the API revamp will have to wait for another issue.
Comment #23
Dries CreditAttribution: Dries commentedThanks for the final review Dave. Committed to CVS HEAD.
Thanks Damien and Gribnif.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis needs to be backported to D6.
Marking #374377: module_list() using unset() to reset a static variable as a duplicate.
Comment #26
vaish CreditAttribution: vaish commentedDo I just re-upload my patch from #374377: module_list() using unset() to reset a static variable to this issue or there is some particular procedure to be followed when backporting?
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commented@vaish: your patch from the other issue should be enough. There is only one change after all :)
Comment #28
vaish CreditAttribution: vaish commented@Damien: I guess my question was more of a general nature so that I know the correct procedure in the future. I just overlooked simplicity of the patch at hand :)
So here is the patch for D6.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedPatch in #26 is a straight backport from D7. Putting it in Gabor's list.
Comment #30
Gábor HojtsyCommitted to Drupal 6, thanks!