While attempting #295994: TestingParty08: Anonymous voting on polls, it was discovered that the drupal web test case doesn't have an existing method for granting anonymous users permissions.
Comment | File | Size | Author |
---|---|---|---|
#141 | 300993-drupalSetPermissions-D7.patch | 11.05 KB | Dave Reid |
#134 | drupal.user-role-set-permissions.134.patch | 10.28 KB | sun |
#132 | drupal.user-role-set-permissions.132.patch | 10.53 KB | sun |
#130 | drupal.user-role-set-permissions.130.patch | 11.02 KB | sun |
#126 | drupal.user-role-set-permissions.125.patch | 9.93 KB | sun |
Comments
Comment #1
jhedstromThe attached patch adds a drupalAnonymousUserPermissions method to the DrupalWebTestCase class, and some anonymous user permission tests for that method to user.test.
I wasn't sure if those tests should go there, or in simpletest.test, since they moreover test the simpletest method, rather than core user functionality.
Comment #2
Dave ReidSubscribing. This would be very useful for a multitude of tests.
Comment #3
Dave ReidWe could actually maybe use some code already written in the contact.test file:
and rename that drupalSetPermission.
Comment #4
Dave ReidPatch for #3.
Comment #5
jhedstromThe patch in #1 (which still applies btw, not sure why the automated test says it's failing) mimicked the functionality of drupalCreateUser(), which takes an array of permissions, letting the test writer ignore roles.
I'm not opposed to the method in #4, but if we go that route I would like to see a wrapper function that hides rid (sets it to 1 for anonymous user role). The tests from the patch in #1 should also be merged with the patch in #4.
Comment #6
jhedstromThe more I think about this, the more I think the method employed in #1 is the way to go. It creates the permissions in the same manner as drupalCreateUser. Until that method is updated, this function should probably stay as close to that as possible.
Comment #8
boombatower CreditAttribution: boombatower commenteddue to: #74645: modify file_scan_directory to include a regex for the nomask.
Comment #9
Dave ReidHere's a revision combining the current code in drupalWebTestCase with the patch in #1. It can be used for any user roles, including 'anonymous user'. Ran the user tests and they passed, so let's see how it does on the whole suite. This would be useful as we have in several tests have to implement a setPermissions function (see contact.test) to assign/remove permissions in tests.
Comment #10
Dave ReidQuick revision that now works with an empty array of permission strings that caused exceptions on the user administration test.
Comment #12
Dave ReidRe-rolled to HEAD and a little cleaning.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks great. Some remarks:
- drupalSetPermissions() should be protected, not public (those function are only called from descendants of DrupalWebTestCase)
- _drupalCreateRole() should be called drupalCreateRole(). I see no reason for this to be private, so we could move it to protected too (its signature and contract is clear enough
- we should remove the assertions in those functions. It is not useful at all to check if the database correctly inserted rows! (those assertions cannot fail in isolation: if the database does not insert rows correctly, everything else will probably fall into pieces anyway).
Comment #14
Dave ReidRevised patch that replaces the contact.test function setPermissions with drupalSetPermissions
Comment #15
Dave ReidI think we posted at the same time Damien. :) I'll get another revision going.
Comment #16
Dave ReidNot sure what assertions we should remove specifically, but here's another revision that has changes addressed in #13.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedInteresting... :)
This serves no purpose and can go.
Idem.
Comment #19
Dave ReidWell shucks, dimitrig01 started making some of the changes in #340081: SimpleTest: Clean-up drupalCreateUser() that will conflict with this. :/
Comment #20
Dave ReidRevised patch that passes all tests (well locally, let's see how the testing bot does). I do not want to remove the assertions that Damien wants me to in #17, since it causes a large chain of changes throughout simpletest.test and more. If we want to remove those, I feel like that should be a separate patch.
So a summary so far of changes:
- Adds a new protected function drupalSetPermissions($role, array $permissions = array()) to drupalWebTestCase to assign permissions to role name $role.
- Replaces the manual permissions setting in comment.test and contact.test with the more elegant drupalSetPermissions(). The permissions interface is tested throughly in user.test.
- Renamed _drupalCreateRole() to drupalCreateRole() for standardization.
- Reworked the default parameters of drupalCreateRole(), drupalSetPermissions(), and drupalCreateUser() for easier use.
- Reworked the tests in simpletest.test since assertAssertion() uses strpos and not a regex-style string matching. The assertion being checked for is 'Assigned permissions to role %role: @perms'. During simpletest.test, we don't have any way to get the name of the user role created during the test, so we just do two checks. One for 'Assigned permissions to role' and another for $this->valid_permission.
- Picked up trailing whitespace in a documentation.
- Fixed a bug in drupalLogin($user = NULL)! The documentation for the function says specifically "If no user is specified then a new user will be created and logged in."
Before:
After:
$user variable needs to be an actual user, not the ID of a user role. Kind of hard to log in an role ID.
Please review, this would more useful for several tests and more efficient than the current user/role implementation.
Comment #21
Dave ReidHere's also a comparison on the number of assertions changed with the patch. Should be a good speed up in the comment.test and contact.test:
Comment #22
Dave ReidBTW, this passed testing bot, but results have not made it back here for some reason or another (maybe related to d.org db issues tonight).
http://testing.drupal.org/pifr/file/1/2177
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedThe role/permissions APi in core is awful. I think if we focus on core we might not need these methods in test framework. we need good api for install profiles and update functions and so on. like install_profile_api module. my .02
Comment #24
David StraussI agree with Moshe. This API belongs outside the testing framework.
Comment #25
Dave ReidI respectfully disagree. We have an easy way to create users, to create roles, and assign roles to created users, but nothing for anonymous users in DrupalWebTestCase. Multiple core tests would be able to take advantage of this new function (comment, contact, poll) along with many possible contrib modules that do things a little differently for anonymous users. All that adding drupalSetPermissions() really does it take the work out of drupalCreateRole() and make it available for all tests to use. If we change the permissions page, we'll only need to make adjustments in drupalSetPermissions instead of all the tests that had to implement their own permissions-setting function.
Comment #26
David Strauss@Dave I think you're overlooking the fact that putting the API outside the testing framework makes it accessible to both general modules and unit tests.
Comment #27
Dave ReidI'm in complete agreement that the user/role API needs a rewrite and could be very improved, but I don't see why we can't make it easier to write tests for multiple core modules. Once the API is rewritten (which I haven't seen any recent issues for, so I'm assume it will be a while), we can replace the DrupalWebTestCase functions (including drupalCreateUser and drupalCreateRole) with their native functions. Anyway, I've made my case for this so I'll leave it up for more discussion. :)
Comment #28
jhedstromI tend to agree with what Dave said in #27--as the user/role APIs are re-worked, this functionality can be removed/reworked in the testing code. If we don't include this functionality in the testing framework now, then what will happen is that each isolated area of code that needs to test anonymous user permissions will get a different, home-brewed solution that will be much harder to root out once there is a better API in core to work with.
Comment #29
Dave ReidRe-rolling for change to drupal_web_test_case.php
Comment #31
Dave ReidRetry
Comment #32
Dave ReidThis time a patch without a commented-out debugging line...
Comment #33
Dave ReidReposting after today's simpletest commits.
Comment #35
Dave ReidTesting failure...
Comment #36
Dave ReidComment #37
moshe weitzman CreditAttribution: moshe weitzman commentedFrom my perspective, this patch is won't fix. I earlier said that code for programmatically creating a user and role should be in user module. The reply from Dave and jhedstrom was essentially "we need it now in testing, and when core gets its act together, we'll just use that.". But this issue *is* core getting its act together. I disagree that a whole core API rewrite is needed - this patch is basically whats needed.
If we put this in testing code, then contrib modules all have to implement their own duplicate functions. For example, see install_profile_api.
I'm willing to let the comitters decide, but thats my .02
Comment #38
webchickI'm in Moshe's camp. Dave asked me to take a look at this and comment on the patch.
I whole-heartedly agree that function setAnonymousUserComment($enabled, $without_approval) is doing this the WRONG way, and abstracting this out into a generalized thing is a good and blessed thing. So the driving purpose of this patch is very sound.
And there are functions in here, such as function drupalCreateUser(array $permissions = array('access comments', 'access content', 'post comments', 'post comments without approval')) which only make sense within the context of the testing framework. Never in your life in a "real world" context are you going to want to auto-create a randomly-named role per user.
However, function drupalSetPermissions($role, array $permissions = array()) *totally* makes sense in a zillion of other contexts (install profiles, other modules, etc. etc.). This should be moved up a level to user_set_permissions($role, $permissions = array()) so that everyone can take advantage of this.
function drupalCreateRole(array $permissions = array()) is in kind of an odd grey area. You definitely want to be able to create a role with permissions, but in the "real world" you're going to want to give it a name as well. So how about if we made this user_add_role($name, $permissions = array()) and kept this wrapper function so that the testing framework could stick with its convenient auto-named, non-colliding roles?
And a sister user_delete_role($name) would just make me giddy with excitement. ;)
Comment #39
Dave ReidOk I'm started on abstracting the code out into user.module. I'm wondering what should be done with DWTC->checkPermissions(). Should that be abstracted as well? Or should it be left to check the permissions in DWTC->drupalCreateRole()?
Comment #40
Dave ReidShould we also leave a simplified drupalSetPermissions() in DWTC that uses checkPermissions()? It feels like we should. The test should fail if an invalid permission is supplied. Fixing cross-post.
Comment #41
webchickOh! And maybe user_[un]assign_role($account, $role). Then drupalCreateUser() could call user_add_role() and user_assign_role() internally.
$role should probably be one of those "magic" arguments that can accept either a role name or a role ID. Also see install_profile_api's user.inc file for a starter on this.
I will stop eating baby kittens now. ;)
Comment #42
webchickHm. Core doesn't tend to babysit broken code, so yeah, probably leave checkPermissions in SimpleTest.
Comment #43
Dave ReidOk so here's a revision that abstracts out a user_add_role(), user_set_permissions(), and user_get_role_id() in user.module. This patch still leaves in a DWTC->drupalSetPermissions() so it can still use DWTC->checkPermissions(), but uses the new function user_set_permissions(). :)
Comment #44
Dave Reidoops...had two different function names.
Comment #45
Dave ReidAnother reason for keeping DWTC->setPermissions is because since we'll want to backport this change to 6.x-2.x simpletest, we can basically use the patch from #33 then since we will not have the new user api functions available.
Comment #46
Dave ReidPatch could use some polish and a role administration test.
Comment #47
Dave ReidComment #48
Dave ReidFinally had time to revisit this patch. Moving to user.module since this is where most of the work is done now. New patch that adds the following role/permission API functions to user.module:
Changes to SimpleTest API:
- Moved the default permissions parameter to drupalCreateUser() from _drupalCreateRole().
- Changes _drupalCreateRole() to drupalCreateRole() that is now simply a wrapper for user_add_role().
- Adds a new function drupalSetPermissions() that is simply a wrapper for user_set_permissions with permission string checking with DWTC->checkPermissions().
Other changes:
- Adds EXTENSIVE role and permission tests. I don't think there is any new or existing line of role or permission code not covered by the tests. Here's looking at you, webchick!
- Fixes a bug in the user role admin screen where an empty string could be used as a user role name.
- Changes all the hackish permission code in comment.test and contact.test to use the APIs.
- system_install() will only add the user roles, but not their permissions. Permission adding has been moved to the default.profile.
Please review and let's help improve the role/permissions API!
Comment #50
Dave ReidRemoved the already fixed whitespace changes in DWTC.
Comment #51
Dave ReidNot sure if I should mark #300093: Provide function to add/delete permisions from a given role as a duplicate of this issue. It is interesting to note the issue numbers...300093...300993 for nearly the same purpose. :)
Comment #53
Dave ReidOk, actually installable and testable now. :)
Comment #54
moshe weitzman CreditAttribution: moshe weitzman commentedAmen. Great patch.
Comment #55
Dave ReidRemoved the accidental bit I had from #374568: Useless JOIN - loading role. Will revise again with moshe's thoughts tomorrow.
Comment #56
Dave ReidReviewing my own patch...
Also need to remove from UserRolesPermissionsFunctionalTest:
Remove the check for user_get_role_id() in user_add_role().
$form_state['values']['name'] needs to be actually trim()'d in user_admin_role_validate().
Comment #57
Dave Reid@moshe, Which queries should be converted to db_select()? What do you think we should use as the query tag? user_role, user_access?
Comment #58
Dave Reid@moshe: A merge query doesn't look like it will work on the role_permissions table in MySQL since the table is just two primary key fields and there are no fields to update. See #374940: MergeQuery with only key fields should not fail.
Comment #59
Dave ReidRe-rolled now that #374940: MergeQuery with only key fields should not fail is fixed. Passed all tests locally and should be ready for another review.
Comment #60
Dave ReidAdding tags
Comment #61
Dries CreditAttribution: Dries commentedThis patch looks great. Like it a lot.
My only remark is that we should consider renaming user_role_add() to user_role_create(). It flows a bit more naturally to me, and I think it does for you to. In your code comments, you always use 'create' and not 'add'. user_role_add() sounds as if we're adding a role to a user, but we're actually creating a new role and not assigning it to any user.
Comment #62
Dave ReidHmm...it looks like we don't really use the verb 'add' or 'create' in core really at all. Maybe user_role_save (and it's opposite user_role_delete)?
Comment #63
Dave ReidRevised patch with user_role_save() instead of user_role_add(). Also adds a test for DWTC->drupalSetPermissions() in simpletest.test.
Comment #64
Dave ReidOk this one should be easier to read since I haven't moved a function around in user.module.
Comment #66
moshe weitzman CreditAttribution: moshe weitzman commentedAny chance Dave can reroll this?
Comment #67
Dave ReidI've actually hit a road block writing the tests for this patch. I can't test the user/x/edit form because SimpleTest tries to submit disabled checkboxes even when they aren't included in the parameters. See #335035: drupalPost() incorrectly submits input for disabled elements.
Comment #68
moshe weitzman CreditAttribution: moshe weitzman commented@Dave - that issue is stalled. Is there some way to get acceptable test coverage without fixing that issue? It is a real shame to postpone nice architcture like this because of a limitation in the test framework.
Comment #69
boombatower CreditAttribution: boombatower commentedLets knock out the other issue. I'll see if I can get it moving again.
Comment #70
dropcube CreditAttribution: dropcube commentedMarked #546668: User system doesn't communicate with modules about roles: add user roles API a duplicate of this. I am going to continue the work here.
Comment #71
dropcube CreditAttribution: dropcube commentedThis patch merged the patch posted at http://drupal.org/node/546668#comment-1916184 (#546668: User system doesn't communicate with modules about roles: add user roles API), with some of the features in #69 of this issue.
Additions:
- API functions to manipulate user roles:
user_role_load($role)
,user_role_save($role, $permissions)<code>user_role_load($role)
,user_role_delete($role)
,user_role_get_id($role_name)
- hooks
hook_user_role_insert($role)
,hook_user_role_update($role)
,hook_user_role_delete($role)
, to inform other modules about role changes.- Implementation of
hook_user_role_delete()
in core modules filter.momdule and block.mdouleChanges:
-
system_install()
will only add the user roles, but not their permissions. Permission adding should be responsibility of the install profile, so has been moved to the default.install.- Used roles API to create roles and permissions in system_install() and profile install.
- Used roles API in DrupalWebTestCase to create roles and permissions.
I didn't add the tests stuff in #69 yet, I suggest to add the API first and then make use of it to clean/improve the tests.
Reviews appreciated!
Comment #73
Dries CreditAttribution: Dries commentedI like this clean-up a lot but I think it needs a bit more work. Here is a quick review -- it's not an extensive review yet so I recommend others have a look at this as well.
- The user_role_save() API feels broken in that (i) it tries to do to much -- it shouldn't attempt to save permissions IMO and (ii) does not allow me to rename a role. It should probably be user_role_save($rid, $role) so a role could be renamed programmatically.
- There is something weird with having both user_role_get_id() and user_role_load(). I'd suggest that we remove user_role_get_id() in favor of user_role_load()? Just inline user_role_get_id() in user_role_load().
- There is still a TODO in the code.
- :repl should probably be :replacement.
Comment #74
dropcube CreditAttribution: dropcube commented@Dries, thanks for the review.
- Ok, user_role_save() shouldn't attempt to save permissions, it's true, there is a different API function for that.
- user_role_save($role) receives a role object, so if you want to rename the role, just work on the role object and then save the changes to the database. It's consistent with other API functions in core.
- user_role_load() changed to receive both string and integer parameters, and removed user-role_get_id in favor of this.
Updated patch:
- Fixed issues on install reported y test.
- Fixed the issues above.
Comment #75
kyle_mathews CreditAttribution: kyle_mathews commentedNice cleanup. Everything looks rtbc except for one debug() left in the code.
Comment #76
Dries CreditAttribution: Dries commentedComment #77
dropcube CreditAttribution: dropcube commentedRemoving debug() from patch.
Comment #78
moshe weitzman CreditAttribution: moshe weitzman commentedback to rtbc
Comment #79
webchickWow, this patch is great!
A couple quick nits.
These two lines left me wondering what user_role_load() returned when it didn't find a result. However...
This doesn't really tell me. Is it FALSE? NULL? 'Your mom'? Let's better-document the return values here and in other functions. For example...
What data type will the status be? "updated" vs. "inserted"? 1 vs. 0? It's not clear to me from the documentation, and it's even less clear to me from the code. :)
Cool. But how do I tell if my module needs to implement these hooks? The delete one hints at it, but it'd be nice to spell it out more explicitly.
Also, is it possible to add a line or two of example code to each hook? Our track record for filling these in after the fact is not great at all. :P
This assertion is kind of confusing. If it created a new role, then $rid should be set, no? Why the check around it?
(minor) This needs to be moved to right above the function declaration or api.drupal.org won't pick it up.
Also, is there a reason filter and block modules don't also implement hook_user_role_update()? Is it because they're basing off role ID and not name? If so, maybe make that clearer in the hook's documentation so modules know when they need to invoke and when they don't.
After these things are cleared up, I'm happy to commit this!
I'm on crack. Are you, too?
Comment #80
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #81
dropcube CreditAttribution: dropcube commentedUpdated patch clearing up issues pointed by webchick.
- Improved documentation of hooks in user.api.php
- Improved API functions documentation
- Fixed minor coding style issues.
- Fixed assertion on role creation.
Comment #82
moshe weitzman CreditAttribution: moshe weitzman commentedAFAICT, we are there.
Comment #83
webchickSorry. One last question as I went through. Please mark back to RTBC when it is answered/addressed.
I see the equivalent of this moved to profiles/default.install but not profiles/expert.install. This seems like a regression. Was this done intentionally, or is it an oversight? Esp. anonymous/authenticated not having access to "access content" seems like it'd be problematic.
This review is powered by Dreditor.
Comment #84
dropcube CreditAttribution: dropcube commentedAdded
to both, profiles/default.install and profiles/expert.install.
Comment #85
dropcube CreditAttribution: dropcube commentedComment #87
dropcube CreditAttribution: dropcube commentedNot sure is the bot is right, I can still can apply the patch.... attaching the same patch
Comment #89
dropcube CreditAttribution: dropcube commentedHey bot, are you sure you can't apply this patch ? I can ;)
Comment #90
Dries CreditAttribution: Dries commentedThis looks ready to me. Thanks for incorporating all our feedback. Committed to CVS HEAD!
Comment #91
sunAt your service. :)
Comment #92
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #93
webchickDocs, plz!
Comment #94
dropcube CreditAttribution: dropcube commentedDocumentation added here: http://drupal.org/update/modules/6/7#user-roles-api
Comment #95
catchComment #96
sunSorry, we need to re-open this.
...provides no way to revoke certain permissions for a role. You can only add some (by merging), or you can nuke 'em all and only grant certain permissions (which I really think is an edge-case and only used by install profiles and the like [and I do not really understand why such code cannot simply empty the permissions table for a certain role manually before granting new permissions...]).
I'm trying to get the overhauled user role configuration in #11218: Allow default text formats per role, and integrate text format permissions working. My first and foremost assumption was that I could just pass an permissions array in the form of
(alternatively replacing those integers with Booleans for clarity) but that is not the case, and currently unsupported.
Two ways to get out of that:
1) Use a keyed array where the value determines the change to make (grant/revoke).
2) Split the $permissions arguments into 2 function arguments, $grant and $revoke, both being arrays.
The first sounds more reasonable to me.
Comment #97
catchI used this on a patch somewhere, and the $merge = FALSE also seemed really odd to me - very easy to forget that argument and mess up someone's permissions set up
Comment #98
sunI can work on this change, but I need to know whether to go with option 1) or 2) upfront.
1)
2)
In SimpleTests, we rather use the scheme of 2). However, I'd say the use-case of configuring multiple permissions (on/off) at once is more common, simply because the usual UI element for configuring permissions is a checkbox, and toggling that checkbox should have an effect in both directions. Splitting form values into two separate arrays is slightly tougher. So I am leaning towards 1).
Comment #99
webchickWhat about two functions? user_role_assign_permissions() and user_role_revoke_permissions() or similar?
Comment #100
sunHrm... admittedly, that would be option 3).
However, tinkering some more about this, option 1), a keyed array of permissions to grant/revoke determined by value, could be directly used in the permissions page form submit handler - NOT nuking all existing permissions for a role.
That would have the potential to also fix #535680: Prevent removing permissions of disabled modules in one shot. (Only uninstalling a module should remove any permission data rows of a module)
Also: Tagging for feature freeze.
Comment #101
sunNeaty.
Comment #102
sunTo clarify in advance: Removal of permissions of uninstalled modules is already catered for in http://api.drupal.org/api/function/user_modules_uninstalled/7
Comment #103
sunTests!
Comment #104
sunForgot to update the PHPDoc.
Comment #105
sunTests pass - RTBC anyone? :)
Comment #106
dropcube CreditAttribution: dropcube commentedPatch looks good, indeed option 1 seems more reasonable. Some comments about the implementation:
(minor) We can use array_fill_keys($permissions, 1) in situations like this, not required to iterate over the permissions array.
(minor optimization) Can we use array functions to extract the permissions that need to be revoked, and execute only one DELETE query.
(minor) We can use array_fill_keys($permissions, 1) here as well.
Comment #107
sunNice nits!
Comment #109
sunbah, stupid typo.
Comment #111
sunbah, another stupid typo ;)
Comment #113
sunwow, this time for real - sorry! :)
Comment #114
Dave Reid@dropcube: Awesome finds and good use of PHP 5.2. :)
"Boolean" being capitalized mid-sentence looked really odd. PHP does not do it (http://us3.php.net/manual/en/language.types.boolean.php) and I've never seen it used like that in core documentation. Also maybe explain that if the boolean is TRUE, that means grant. It's not clear what the boolean value means.
This line was confusing. Maybe reword this as something more like "existing permissions already assigned to the given role not specified in $permissions will not be changed."
Very small nitpicks.
This review is powered by Dreditor.
Comment #115
sunRe-worded after IRC discussion :)
Comment #116
Dave ReidLooking good and passed the bot. As the great Zaboo would say, RTBC-ed.
Comment #117
dropcube CreditAttribution: dropcube commentedRTBC, as far as I can see...
Comment #118
webchickCode like this makes me barf; I'm frequently assigning permissions in install profiles and there is no form involved. having to use array_fill_keys() is nasty.
We talked on IRC and decided that what probably makes the most sense is a compromise:
user_role_set_permissions() is something that can be called by forms to do setting of both enabled/disabled permissions.
Under the hood, it calls user_role_assign_permissions() and user_role_revoke_permissions(), which install profiles or other things that programmatically create users can use.
Win-win? :)
Comment #119
sunRight.
One problem: user_role_set_permissions() calls user_access(NULL, NULL, TRUE) after doing its job to reset all statically cached permissions. However, they are re-generated for the current user when being called that way.
Making user_role_set_permissions() invoke two sub-functions, user_role_grant_permissions() and user_role_revoke_permissions(), would mean that all of those functions need to reset the static cache on their own. In case of user_role_set_permissions(), user_access(NULL, NULL, TRUE) would have to be called three times. And each time it is called, the permissions for the current user will be re-generated.
Luckily, I already found that $reset argument to user_access() totally ugly some hours ago, so the solution is already waiting for a commit: #574002: Remove $reset argument from user_access() :)
Given that, all three functions can independently reset the drupal_static() variable without any performance impact.
Comment #120
sunBlocker is gone. New patch attached.
Additionally renamed user_role_set_permissions() to user_role_change_permissions(), because "set" rather referred to the old implementation, where all existing permissions were nuked and all permissions for the role were "set" to those passed to the function. "change" means to change the list of permissions that are passed, not touching any other permissions.
Comment #121
Dave ReidTwo different variables: $grant and $grants.
What happens if an invalid user role ID or name is provided. Is user_role_grant_permissions() and user_role_revoke_permissions() going to cause a PHP error when they use $role->rid? Is this something we should handle? Feels like we should.
Why not use the $admin_role object instead of causing a lookup?
This review is powered by Dreditor.
Comment #122
sunI love you! :)
Comment #123
sunok, that validation of $role was a bit sluggish. Better one.
Comment #124
Dave Reid+ if (!is_object($role)) {
+ $role = user_role_load($role);
+ }
+ if (!is_object($role) || !isset($role->rid)) {
+ return FALSE;
+ }
Maybe we can just do
if (!is_object($role) && !($role = user_role_load($role)) { return FALSE; }
Comment #126
sunThanks!
Completely revised that. Functions taking arbitrary data types for a single argument are highly confusing and error-prone anyway.
So, all of those functions should be passed a proper $role object - easily loadable via user_role_load(). user_role_load() still accepts a 'rid' or 'name', which is ok for me.
Comment #127
sunI can now confirm that this patch also fixes #535680: Prevent removing permissions of disabled modules. Two in one shot! :)
Comment #128
dropcube CreditAttribution: dropcube commentedLooks good to me. RTBC as far as I can see.
Comment #129
webchickLet's make the documentation of user_role_change_permissions() explain why you use it over the alternatives, and put a @see to the two functions it calls.
Comment #130
sunAdded those requested docs.
Comment #131
Dries CreditAttribution: Dries commentedPersonally, I think it is fairly ugly for an API function to have those kind of checks along with a silent fail. Can't we kill those checks?
Technically, all we seem to need is a role ID, not an object. Passing in a role object might be more consistent with the other API functions though. If that is the case, I'm 100% happy with using an object.
Comment #132
sunTinkered a bit about this. I think you're right - user_role_load/save/delete($role) acts on the $role object itself, while user_role_change/grant/revoke_permissions($rid, $permissions) primarily works on the role's permissions, not on the role. Therefore, we can pass a simple role ID instead of a requiring a full $role object.
Attached patch implements $rid as the argument instead of $role.
Ready to fly if bot comes back green.
--
In D8, we should additionally allow user_role_change_permissions() to accept a matrix of roles/permissions to simplify contextual configuration of user permissions (like in Filter UI's case).
Comment #134
sunRe-rolled. Still looking good.
Comment #135
moshe weitzman CreditAttribution: moshe weitzman commentedNot related to this patch, but I dislike this emerging pattern in drupal. It is no good when one function deletes the statics that belong to another. We should use the $reset on user_access(). We introduced the static API in order to facilitate drupal_static_reset() during simpletest suite. But we're now abusing it, IMO.
Comment #136
sun@moshe: #574002: Remove $reset argument from user_access() removed that $reset argument from user_access() and user_role_permissions(). We should use the new drupal_static() paradigm consistently throughout core, especially in API functions that are commonly used by contributed modules. It's unlikely that a module needs to reset those statics on its own, i.e. without invoking one of the other API functions that reset the drupal_static() after performing an action -- which means that such a module would want to play games with the data that is normally static per request, potentially to "trick" something weird into the system. Such modules/developers should really know what they are doing. I'd rather like to see all $reset arguments from public API functions vanish, so 99.9% of all developers don't screw up the site's performance, and require those 0.1% to really understand what they're doing.
The hunk you remarked just moves the hook invocation before the drupal_static_reset() invocations, because that is the pattern we currently use everywhere:
1) Perform the action and invoke all hooks.
2) Clear any caches.
However, this is just for consistency. Actually, I believe we should open a separate issue to change (reverse) that logic - because: in case any hook implementation wants to update other, depending data to account for the new data, it can't use the public API functions, since they still return the old (cached) data.
Comment #137
moshe weitzman CreditAttribution: moshe weitzman commentedsun - you and i simply disagree on what clean, modular code means for this particular pattern. Thats OK - happens all the time.
Comment #138
sunLet's move that discussion: #581626: Use a consistent/clean pattern for using $reset or drupal_static() :)
Comment #139
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #140
ebeyrent CreditAttribution: ebeyrent commentedIt looks like this work will remove the necessity for having to port the Permissions API module to Drupal 7, unless there are features in that module that can be moved into core?
Comment #141
Dave ReidFollow-up patch that removes cruft in test functions before we had these APIs. This was in the original patches before we concentrated on just the API functions.
Comment #142
sunHah, didn't even think of so many clean-up possibilities due to this patch - awesome! :)
Comment #143
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #144
Dave ReidI don't actually see a CVS commit for this. :/
Comment #145
webchickOk, *now* it's committed to HEAD. ;)
Comment #146
Dave ReidThanks Angie! :)