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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chuckdeal97’s picture

dergachev’s picture

I'm using this patch to fix a serious bug in Login Toboggan. See http://drupal.org/node/319438#comment-1112176

Dave Reid’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

Please help fix in 7.x first, then we can backport. Also be sure to check out http://drupal.org/patch/create.

Damien Tournoud’s picture

The 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.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Here is a proper patch for D7, including a test case.

Please credit Gribnif for the discovery.

Dries’s picture

The 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. :)

Damien Tournoud’s picture

@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.

Dave Reid’s picture

Status: Needs review » Needs work

Don't commit yet please...I'm revising the test.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
3.54 KB

Confirmed 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Well, sorry Damz, this passed for me on my system.

Dave Reid’s picture

Testing bot failure...

Dave Reid’s picture

Status: Needs work » Needs review
Dries’s picture

Dave, 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 than assertEqual()?

Dave Reid’s picture

assertEqual 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.

Damien Tournoud’s picture

Assigned: Unassigned » Damien Tournoud
Status: Needs review » Needs work

Let me work on a better version.

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned
Status: Needs work » Needs review
FileSize
4.15 KB

Here we are, with a more throughout test.

Dave Reid’s picture

First quick look, the following code can be removed since it has no purpose:

+    $sorted_new_module_list = $new_module_list;
+    ksort($sorted_new_module_list);

I'll re-test and everything when I get back from my interview today.

Damien Tournoud’s picture

@Dave, good catch. Here is a reroll.

Dries’s picture

Patch 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!

Dave Reid’s picture

+ * 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.

Damien Tournoud’s picture

Took care of all this. I think the API revamp will have to wait for another issue.

Dries’s picture

Status: Needs review » Fixed

Thanks for the final review Dave. Committed to CVS HEAD.

Thanks Damien and Gribnif.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Damien Tournoud’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

This needs to be backported to D6.

Marking #374377: module_list() using unset() to reset a static variable as a duplicate.

vaish’s picture

Do 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?

Damien Tournoud’s picture

@vaish: your patch from the other issue should be enough. There is only one change after all :)

vaish’s picture

Status: Patch (to be ported) » Needs review
FileSize
683 bytes

@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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #26 is a straight backport from D7. Putting it in Gabor's list.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.