Simpletest: check permissions to make sure they are valid

boombatower - June 5, 2008 - 23:35
Project:Drupal
Version:7.x-dev
Component:simpletest.module
Category:task
Priority:normal
Assigned:boombatower
Status:closed
Description

This patch checks permissions passed to drupalCreateUser to ensure that invalid permissions aren't accidentally used.

This is very helpful in spotting out-dated tests and typos.

Requested by Dries http://drupal.org/node/266539#comment-871680, but mentioned several times before.

#1

boombatower - June 5, 2008 - 23:39

Hmm...patch was there when I previewed.

AttachmentSizeStatusTest resultOperations
simpletest_check_permissions.patch1.86 KBIgnoredNoneNone

#2

webchick - June 6, 2008 - 00:13
Status:needs review» needs work

Oh, *awesome*!

One suggestion I have is to add static caching to that $available array. It's probably expensive to re-generate that each time drupalCreateUser is called.

see http://www.lullabot.com/articles/a_beginners_guide_to_caching_data for more info.

#3

webchick - June 6, 2008 - 00:15

Oh, also, you're missing a patch to simpletest.test to make sure this function is working properly. ;)

#4

Damien Tournoud - June 6, 2008 - 00:33

Hum. Just a though here, why not implement that check in user_access() in order to enforce it everywhere in Drupal? I remember that this loosely typed permission bit me several times.

#5

boombatower - June 6, 2008 - 00:42
Status:needs work» needs review

Updated to include test and caching.

AttachmentSizeStatusTest resultOperations
simpletest_check_permissions.patch4.66 KBIgnoredNoneNone

#6

boombatower - June 6, 2008 - 16:14

Any thoughts on #4. I would be for it.

#7

Dries - June 7, 2008 - 11:00

I would leave #4 for another issue. I'm not sure I'd want to check the permissions for every page view. I'd try to work it into devel.module or so.

#8

boombatower - June 7, 2008 - 18:50

Ok, so any reviews? :)

#9

catch - June 8, 2008 - 18:07
Status:needs review» reviewed & tested by the community

It's a good idea and all the tests pass, RTBC.

#10

Dries - June 8, 2008 - 19:14
Status:reviewed & tested by the community» fixed

Commited to CVS HEAD. Thanks again, boombatower.

#11

Anonymous (not verified) - June 22, 2008 - 19:22
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.