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

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

Here'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.

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

Revised 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:

The 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

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

I'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. :)

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

From 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 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. ;)

Ok 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()?

Ok 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(). :)

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

Finally 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:

<?php/** * Get the role ID of a role name. * * @param $role_name * The name of the role. * @return * An integer of the role's ID, or FALSE if the role was not found. */function user_get_role_id($role_name)

/** * Create a new user role. * * @param $name * The name of the new user role. * @param $permissions * An array of permissions to assign to the role. * @return * The role ID of the new role, or FALSE if a role with $name already exists. */function user_add_role($name, array $permissions = array())

/** * Delete a user role. * * @param $role * A string with the role name, or an integer with the role ID. * @return * TRUE on success, FALSE otherwise. */function user_delete_role($role)

/** * Assign an array of permissions to a role. * * @param $role * A string with the role name, or an integer with the role ID. * @param $permissions * An array of permissions strings. * @param $merge * A boolean if TRUE, will only add permissions instead of clearing first. * @return * TRUE on success, FALSE otherwise. */function user_set_permissions($role, array $permissions = array(), $merge = FALSE)?>

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.

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.

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

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

Changes:
- 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.

I 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().

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

These two lines left me wondering what user_role_load() returned when it didn't find a result. However...

+++ modules/user/user.module 22 Aug 2009 20:11:39 -0000@@ -2256,6 +2256,108 @@ /**+ * Fetch a user role from database.+ *+ * @param $role+ * A string with the role name, or an integer with the role ID.+ */+function user_role_load($role) {

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

+++ modules/user/user.module 22 Aug 2009 20:11:39 -0000@@ -2256,6 +2256,108 @@+/**+ * Save a user role to the database.+ *+ * @param $role+ * A role object to modify or add. If $role->rid is not specified, a new+ * role will be created.+ * @return+ * Status constant indicating if role was created or updated.+ */+function user_role_save($role) {

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

+++ modules/user/user.api.php 22 Aug 2009 20:10:57 -0000@@ -423,5 +423,45 @@ /**+ * Act on user roles when inserted.+ *+ * Modules implementing this hook can act on the user role object when saved to+ * the database....+/**+ * Act on user roles when updated.+ *+ * Modules implementing this hook can act on the user role object when updated....+/**+ * Inform other modules that a user role has been deleted.+ *+ * This hook allows you act when a user role has been deleted.+ * It is recommended that you implement this hook if your module+ * stores references to roles.

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

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

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.

...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...]).

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

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.

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

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.

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.

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.

Personally, 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.

Tinkered 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).

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

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