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.

Files: 
CommentFileSizeAuthor
#141 300993-drupalSetPermissions-D7.patch11.05 KBDave Reid
Passed: 13509 passes, 0 fails, 0 exceptions
[ View ]
#134 drupal.user-role-set-permissions.134.patch10.28 KBsun
Failed: Failed to apply patch.
[ View ]
#132 drupal.user-role-set-permissions.132.patch10.53 KBsun
Failed: Failed to apply patch.
[ View ]
#130 drupal.user-role-set-permissions.130.patch11.02 KBsun
Failed: Failed to apply patch.
[ View ]
#126 drupal.user-role-set-permissions.125.patch9.93 KBsun
Failed: Failed to apply patch.
[ View ]
#123 drupal.user-role-set-permissions.123.patch10.21 KBsun
Failed: 12925 passes, 1 fail, 0 exceptions
[ View ]
#122 drupal.user-role-set-permissions.122.patch10.11 KBsun
Failed: 12968 passes, 1 fail, 0 exceptions
[ View ]
#120 drupal.user-role-set-permissions.120.patch10 KBsun
Failed: 12846 passes, 151 fails, 10 exceptions
[ View ]
#115 drupal.user-role-set-permissions.115.patch8.23 KBsun
Failed: Failed to apply patch.
[ View ]
#113 drupal.user-role-set-permissions.113.patch8.22 KBsun
Failed: Failed to apply patch.
[ View ]
#111 drupal.user-role-set-permissions.111.patch8.19 KBsun
Failed: Failed to install HEAD.
[ View ]
#109 drupal.user-role-set-permissions.109.patch8.19 KBsun
Failed: Failed to install HEAD.
[ View ]
#107 drupal.user-role-set-permissions.107.patch8.19 KBsun
Failed: Invalid PHP syntax in profiles/default/default.install.
[ View ]
#104 drupal.user-role-set-permissions.104.patch8.36 KBsun
Failed: Failed to apply patch.
[ View ]
#103 drupal.user-role-set-permissions.103.patch7.71 KBsun
Failed: Failed to apply patch.
[ View ]
#101 drupal.user-role-set-permission.patch5.87 KBsun
Failed: Failed to apply patch.
[ View ]
#91 drupal.user-system-changelog.patch1.81 KBsun
Failed: Failed to apply patch.
[ View ]
#89 user-roles-API-300993-89.patch16.32 KBdropcube
Failed: Failed to apply patch.
[ View ]
#87 user-roles-API-300993-86.patch16.45 KBdropcube
Failed: Failed to apply patch.
[ View ]
#84 user-roles-API-300993-84.patch16.45 KBdropcube
Failed: Failed to apply patch.
[ View ]
#81 user-roles-API-300993-81.patch15.74 KBdropcube
Failed: Failed to apply patch.
[ View ]
#77 user-roles-API-300993-77.patch14.37 KBdropcube
Failed: Failed to apply patch.
[ View ]
#74 user-roles-API-300993-74.patch14.39 KBdropcube
Failed: Failed to apply patch.
[ View ]
#71 user-roles-API-300993-71.patch19 KBdropcube
Failed: Failed to install HEAD.
[ View ]
#64 300993-drupalSetPermissions-D7.patch34.37 KBDave Reid
Failed: 10180 passes, 7 fails, 0 exceptions
[ View ]
#63 300993-drupalSetPermissions-D7.patch36.84 KBDave Reid
Failed: 10180 passes, 7 fails, 0 exceptions
[ View ]
#59 300993-drupalSetPermissions-D7.patch33.89 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#55 300993-drupalSetPermissions-D7.patch30.27 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#53 300993-drupalSetPermissions-D7-5.patch31.12 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#50 300993-drupalSetPermissions-D7-4.patch31.81 KBDave Reid
Failed: Failed to install HEAD.
[ View ]
#48 300993-drupalSetPermissions-D7-4.patch32.07 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#44 300993-drupalSetPermissions-V2-D7.patch18.44 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#43 300993-drupalSetPermissions-V2-D7.patch18.44 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#33 300993-drupalSetPermissions-D7.patch13.68 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#32 300993-drupalSetPermissions-D7.patch14.58 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#31 300993-drupalSetPermissions-D7.patch14.67 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#29 300993-drupalSetPermissions-D7.patch14.72 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#20 300993-drupalSetPermissions-D7.patch14.75 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#16 300993-drupalSetPermissions-D7.patch7.92 KBDave Reid
Failed: 7355 passes, 2 fails, 0 exceptions
[ View ]
#14 300993-drupalSetPermissions-D7.patch7.65 KBDave Reid
Failed: 7413 passes, 2 fails, 0 exceptions
[ View ]
#12 300993-drupalSetPermissions-D7.patch3.66 KBDave Reid
Failed: Invalid PHP syntax.
[ View ]
#10 300993-drupalSetPermissions-D7.patch3.64 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#9 300993-drupalSetPermissions-D7.patch3.65 KBDave Reid
#4 300993-simpletest-drupalSetPermission-D7.patch1.46 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#1 anonymous_user_perms.patch3.28 KBjhedstrom
Failed: Failed to install HEAD.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.28 KB
Failed: Failed to install HEAD.
[ View ]

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

Subscribing. This would be very useful for a multitude of tests.

We could actually maybe use some code already written in the contact.test file:

<?php
 
/**
   * Set permission.
   *
   * @param string $role User role to set permissions for.
   * @param array $permissions Key-value array of permissions to set.
   */
 
function setPermission($role, $permissions) {
   
// Get role id (rid) for specified role.
   
$rid = db_result(db_query("SELECT rid FROM {role} WHERE name = '%s'", array($role)));
    if (
$rid === FALSE) {
     
$this->fail(t(' [permission] Role "' . $role . '" not found.'));
    }
   
// Create edit array from permission.
   
$edit = array();
    foreach (
$permissions as $name => $value) {
     
$edit[$rid . '[' . $name . ']'] = $value;
    }
   
$this->drupalPost('admin/user/permissions', $edit, t('Save permissions'));
   
$this->assertText(t('The changes have been saved.'), t(' [permission] Saved changes.'));
  }
?>

and rename that drupalSetPermission.

StatusFileSize
new1.46 KB
Failed: Failed to apply patch.
[ View ]

Patch for #3.

Status:Needs review» Needs work

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.

Status:Needs work» Needs review

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Title:Create drupal web test case method for granting anonymous users permissionsCreate public method for granting user permissions
StatusFileSize
new3.65 KB

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.

StatusFileSize
new3.64 KB
Failed: Failed to apply patch.
[ View ]

Quick revision that now works with an empty array of permission strings that caused exceptions on the user administration test.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.66 KB
Failed: Invalid PHP syntax.
[ View ]

Re-rolled to HEAD and a little cleaning.

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

StatusFileSize
new7.65 KB
Failed: 7413 passes, 2 fails, 0 exceptions
[ View ]

Revised patch that replaces the contact.test function setPermissions with drupalSetPermissions

Status:Needs review» Needs work

I think we posted at the same time Damien. :) I'll get another revision going.

Status:Needs work» Needs review
StatusFileSize
new7.92 KB
Failed: 7355 passes, 2 fails, 0 exceptions
[ View ]

Not sure what assertions we should remove specifically, but here's another revision that has changes addressed in #13.

<?php
-    $admin_user = $this->drupalCreateUser(array('administer site-wide contact form', 'administer permissions'));
+   
$admin_user = $this->drupalCreateUser(array('administer site-wide contact form'));
?>

Interesting... :)

<?php
+    $count = db_query("SELECT COUNT(*) FROM {role_permission} WHERE rid = :rid", array(':rid' => $rid))->fetchField();
+   
$result = $count == count($permissions);
+   
$this->assertEqual($result, t('Assigned role %role permissions: @perms', array('%role' => $role, '@perms' => implode(', ', $permissions))), t('Role'));
?>

This serves no purpose and can go.

<?php
     $role
= db_fetch_object(db_query("SELECT * FROM {role} WHERE name = '%s'", $role_name));
    
$this->assertTrue($role, t('Created role of name: @role_name, id: @rid', array('@role_name' => $role_name, '@rid' => (isset($role->rid) ? $role->rid : t('-n/a-')))), t('Role'));
?>

Idem.

Status:Needs review» Needs work

The last submitted patch failed testing.

Well shucks, dimitrig01 started making some of the changes in #340081: SimpleTest: Clean-up drupalCreateUser() that will conflict with this. :/

StatusFileSize
new14.75 KB
Failed: Failed to apply patch.
[ View ]

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:

<?php
 
protected function drupalLogin($user = NULL) {
    if (
$this->isLoggedIn) {
     
$this->drupalLogout();
    }
    if (!isset(
$user)) {
     
$user = $this->_drupalCreateRole();
    }
...
?>

After:

<?php
 
protected function drupalLogin($user = NULL) {
    if (
$this->isLoggedIn) {
     
$this->drupalLogout();
    }
    if (!isset(
$user)) {
     
$user = $this->drupalCreateUser();
    }
...
?>

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

Status:Needs work» Needs review

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

Before patch:
Comment     612 passes, 0 fails, and 0 exceptions
Contact     327 passes, 0 fails, and 0 exceptions
SimpleTest  81 passes,  0 fails, and 0 exceptions
User        320 passes, 0 fails, and 0 exceptions
After patch:
Comment     518 passes, 0 fails, and 0 exceptions
Contact     298 passes, 0 fails, and 0 exceptions
SimpleTest  83 passes,  0 fails, and 0 exceptions
User        320 passes, 0 fails, and 0 exceptions

BTW, 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

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 agree with Moshe. This API belongs outside the testing framework.

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.

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

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.

StatusFileSize
new14.72 KB
Failed: Failed to apply patch.
[ View ]

Re-rolling for change to drupal_web_test_case.php

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.67 KB
Failed: Failed to apply patch.
[ View ]

Retry

StatusFileSize
new14.58 KB
Failed: Failed to apply patch.
[ View ]

This time a patch without a commented-out debugging line...

Assigned:jhedstrom» Dave Reid
StatusFileSize
new13.68 KB
Failed: Failed to apply patch.
[ View ]

Reposting after today's simpletest commits.

Status:Needs review» Needs work

The last submitted patch failed testing.

Testing failure...

Status:Needs work» Needs review

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 willing to let the comitters decide, but thats my .02

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Status:Needs review» Needs work

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

Oh! 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. ;)

Hm. Core doesn't tend to babysit broken code, so yeah, probably leave checkPermissions in SimpleTest.

Status:Needs work» Needs review
StatusFileSize
new18.44 KB
Failed: Failed to apply patch.
[ View ]

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

StatusFileSize
new18.44 KB
Failed: Failed to apply patch.
[ View ]

oops...had two different function names.

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.

Status:Needs review» Needs work

Patch could use some polish and a role administration test.

Title:Create public method for granting user permissionsImprove the role and permissions API
Component:simpletest.module» user.module
Status:Needs work» Needs review
StatusFileSize
new32.07 KB
Failed: Failed to apply patch.
[ View ]

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.

Please review and let's help improve the role/permissions API!

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.81 KB
Failed: Failed to install HEAD.
[ View ]

Removed the already fixed whitespace changes in DWTC.

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.12 KB
Failed: Failed to apply patch.
[ View ]

Ok, actually installable and testable now. :)

Amen. Great patch.

  1. Perhaps use a merge query in user_set_permissions() when merge is TRUE.
  2. I would love to see db_select() for these API queries. I think they are ripe for hook_query_alter() when sites do their usual tricks with users.
  3. I would name the API functions together like user_role_foo() instead of user_foo_role(). This is nice for IDE users.
  4. Lets make a user.role.inc. It is time to break apart user.module.

StatusFileSize
new30.27 KB
Failed: Failed to apply patch.
[ View ]

Removed the accidental bit I had from #374568: Useless JOIN - loading role. Will revise again with moshe's thoughts tomorrow.

Status:Needs review» Needs work

Reviewing my own patch...

Also need to remove from UserRolesPermissionsFunctionalTest:

  protected $test_user;
  protected $rid;

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

@moshe, Which queries should be converted to db_select()? What do you think we should use as the query tag? user_role, user_access?

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

Status:Needs work» Needs review
StatusFileSize
new33.89 KB
Failed: Failed to apply patch.
[ View ]

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

Issue tags:+permissions, +user roles

Adding tags

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

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

StatusFileSize
new36.84 KB
Failed: 10180 passes, 7 fails, 0 exceptions
[ View ]

Revised patch with user_role_save() instead of user_role_add(). Also adds a test for DWTC->drupalSetPermissions() in simpletest.test.

StatusFileSize
new34.37 KB
Failed: 10180 passes, 7 fails, 0 exceptions
[ View ]

Ok this one should be easier to read since I haven't moved a function around in user.module.

Status:Needs review» Needs work

The last submitted patch failed testing.

Any chance Dave can reroll this?

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

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

Lets knock out the other issue. I'll see if I can get it moving again.

Assigned:Dave Reid» dropcube
Category:feature» task
Priority:Normal» Critical
Issue tags:+DX (Developer Experience), +API change, +API addition

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

Title:Improve the role and permissions APIUser roles and permissions API
Status:Needs work» Needs review
StatusFileSize
new19 KB
Failed: Failed to install HEAD.
[ View ]

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

Reviews appreciated!

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

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

- There is still a TODO in the code.

- :repl should probably be :replacement.

StatusFileSize
new14.39 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Nice cleanup. Everything looks rtbc except for one debug() left in the code.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new14.37 KB
Failed: Failed to apply patch.
[ View ]

Removing debug() from patch.

Status:Needs review» Reviewed & tested by the community

back to rtbc

Status:Reviewed & tested by the community» Needs work

Wow, this patch is great!

A couple quick nits.

+++ modules/user/user.admin.inc 22 Aug 2009 20:10:52 -0000
@@ -776,13 +765,13 @@
+      $role = user_role_load($form_state['values']['name']);
+      if ($role && $role->rid != $form_state['values']['rid']) {

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

+++ modules/simpletest/drupal_web_test_case.php 22 Aug 2009 20:10:03 -0000
@@ -892,26 +892,19 @@
+    $this->assertTrue(user_role_load($name), t('Created role of name: @name, id: @rid', array('@name' => $name, '@rid' => (isset($role->rid) ? $role->rid : t('-n/a-')))), t('Role'));

This assertion is kind of confusing. If it created a new role, then $rid should be set, no? Why the check around it?

+++ modules/filter/filter.admin.inc 22 Aug 2009 20:09:30 -0000
@@ -452,3 +452,14 @@
+/**
+ * Implement hook_user_role_delete().
+ */
+
+function filter_user_role_delete($role) {

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

subscribe

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

AFAICT, we are there.

Status:Reviewed & tested by the community» Needs review

Sorry. One last question as I went through. Please mark back to RTBC when it is answered/addressed.

+++ modules/system/system.install 25 Aug 2009 19:42:35 -0000
@@ -385,21 +385,6 @@
-  $query = db_insert('role_permission')->fields(array('rid', 'permission'));
-  // Anonymous role permissions.
-  $query->values(array(
-    'rid' => DRUPAL_ANONYMOUS_RID,
-    'permission' => 'access content',
-  ));
-
-  // Authenticated role permissions.
-  foreach (array('access comments', 'access content', 'post comments', 'post comments without approval', 'view own unpublished content') as $permission) {
-    $query->values(array(
-      'rid' => DRUPAL_AUTHENTICATED_RID,
-      'permission' => $permission,
-    ));
-  }
-  $query->execute();

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.

StatusFileSize
new16.45 KB
Failed: Failed to apply patch.
[ View ]

Added

  // Enable default permissions for system roles.
  user_role_set_permissions(DRUPAL_ANONYMOUS_RID, array('access content'));
  user_role_set_permissions(DRUPAL_AUTHENTICATED_RID, array('access content', 'access comments', 'post comments', 'post comments without approval'));

to both, profiles/default.install and profiles/expert.install.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.45 KB
Failed: Failed to apply patch.
[ View ]

Not sure is the bot is right, I can still can apply the patch.... attaching the same patch

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.32 KB
Failed: Failed to apply patch.
[ View ]

Hey bot, are you sure you can't apply this patch ? I can ;)

Status:Needs review» Fixed

This looks ready to me. Thanks for incorporating all our feedback. Committed to CVS HEAD!

Status:Fixed» Needs review
StatusFileSize
new1.81 KB
Failed: Failed to apply patch.
[ View ]

At your service. :)

Status:Needs review» Fixed

Committed to CVS HEAD.

Status:Fixed» Needs work
Issue tags:+Needs Documentation

Docs, plz!

Priority:Critical» Normal

Status:Needs work» Fixed

Priority:Normal» Critical
Status:Fixed» Active

Sorry, we need to re-open this.

function user_role_set_permissions($role, array $permissions = array(), $merge = FALSE) {

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

<?php
array(
 
'use this' => 0,
 
'access that' => 1,
 
'edit that' => 0,
)
?>

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

I 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

I can work on this change, but I need to know whether to go with option 1) or 2) upfront.

1)

<?php
$perms
= array(
 
'use this' => 0,
 
'access that' => 1,
 
'edit that' => 0,
);
user_role_set_permissions($role, $perms);
?>

2)

<?php
$grant
= array('access that');
$revoke = array('use this', 'edit that');
user_role_set_permissions($role, $grant, $revoke); // $revoke being optional.
?>

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

What about two functions? user_role_assign_permissions() and user_role_revoke_permissions() or similar?

Issue tags:+API clean-up

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

Status:Active» Needs review
StatusFileSize
new5.87 KB
Failed: Failed to apply patch.
[ View ]

Neaty.

To clarify in advance: Removal of permissions of uninstalled modules is already catered for in http://api.drupal.org/api/function/user_modules_uninstalled/7

StatusFileSize
new7.71 KB
Failed: Failed to apply patch.
[ View ]

Tests!

StatusFileSize
new8.36 KB
Failed: Failed to apply patch.
[ View ]

Forgot to update the PHPDoc.

Tests pass - RTBC anyone? :)

Patch looks good, indeed option 1 seems more reasonable. Some comments about the implementation:

+++ modules/simpletest/drupal_web_test_case.php 10 Sep 2009 18:14:49 -0000
@@ -901,8 +901,12 @@ class DrupalWebTestCase extends DrupalTe
+    $grants = array();
+    foreach ($permissions as $permission) {
+      $grants[$permission] = 1;
+    }
+    user_role_set_permissions($role->name, $grants);

(minor) We can use array_fill_keys($permissions, 1) in situations like this, not required to iterate over the permissions array.

+++ modules/user/user.module 10 Sep 2009 18:18:22 -0000
@@ -2327,28 +2327,36 @@ function user_role_delete($role) {
+      db_delete('role_permission')
+        ->condition('rid', $role->rid)
+        ->condition('permission', $name)
+        ->execute();
+    }

(minor optimization) Can we use array functions to extract the permissions that need to be revoked, and execute only one DELETE query.

+++ profiles/default/default.install 10 Sep 2009 17:54:11 -0000
@@ -184,15 +184,19 @@ function default_install() {
+  foreach (array_keys(module_invoke_all('permission')) as $permission) {
+    $admin_role_permissions[$permission] = 1;
+  }

(minor) We can use array_fill_keys($permissions, 1) here as well.

StatusFileSize
new8.19 KB
Failed: Invalid PHP syntax in profiles/default/default.install.
[ View ]

Nice nits!

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.19 KB
Failed: Failed to install HEAD.
[ View ]

bah, stupid typo.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.19 KB
Failed: Failed to install HEAD.
[ View ]

bah, another stupid typo ;)

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.22 KB
Failed: Failed to apply patch.
[ View ]

wow, this time for real - sorry! :)

Status:Needs review» Needs work

@dropcube: Awesome finds and good use of PHP 5.2. :)

+++ modules/user/user.module 10 Sep 2009 21:21:35 -0000
@@ -2327,29 +2327,38 @@ function user_role_delete($role) {
+ *   value is an integer or Boolean that determines whether to grant or revoke

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

+++ modules/user/user.module 10 Sep 2009 21:21:35 -0000
@@ -2327,29 +2327,38 @@ function user_role_delete($role) {
+ *   Other permissions assigned to the given role are not altered.

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.

Status:Needs work» Needs review
StatusFileSize
new8.23 KB
Failed: Failed to apply patch.
[ View ]

Re-worded after IRC discussion :)

Status:Needs review» Reviewed & tested by the community

Looking good and passed the bot. As the great Zaboo would say, RTBC-ed.

RTBC, as far as I can see...

Status:Reviewed & tested by the community» Needs work

+++ modules/simpletest/drupal_web_test_case.php 10 Sep 2009 20:28:22 -0000
@@ -901,8 +901,8 @@ class DrupalWebTestCase extends DrupalTe
-    user_role_set_permissions($role->name, $permissions);
-
+    user_role_set_permissions($role->name, array_fill_keys($permissions, 1));

Code 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? :)

Right.

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.

Status:Needs work» Needs review
StatusFileSize
new10 KB
Failed: 12846 passes, 151 fails, 10 exceptions
[ View ]

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

Status:Needs review» Needs work

+++ modules/user/user.module 11 Sep 2009 15:02:24 -0000
@@ -2317,26 +2317,56 @@ function user_role_delete($role) {
+  $grant = array_filter($permissions);
+  if (!empty($grants)) {

Two different variables: $grant and $grants.

+++ modules/user/user.module 11 Sep 2009 15:02:24 -0000
@@ -2317,26 +2317,56 @@ function user_role_delete($role) {
+  if (!is_object($role)) {
+    $role = user_role_load($role);
   }

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.

+++ profiles/default/default.install 11 Sep 2009 15:00:17 -0000
@@ -184,15 +184,15 @@ function default_install() {
+  user_role_grant_permissions($admin_role->name, array_keys(module_invoke_all('permission')));

Why not use the $admin_role object instead of causing a lookup?

This review is powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new10.11 KB
Failed: 12968 passes, 1 fail, 0 exceptions
[ View ]

I love you! :)

StatusFileSize
new10.21 KB
Failed: 12925 passes, 1 fail, 0 exceptions
[ View ]

ok, that validation of $role was a bit sluggish. Better one.

+ 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; }

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.93 KB
Failed: Failed to apply patch.
[ View ]

Thanks!

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.

I can now confirm that this patch also fixes #535680: Prevent removing permissions of disabled modules. Two in one shot! :)

Status:Needs review» Reviewed & tested by the community

Looks good to me. RTBC as far as I can see.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.02 KB
Failed: Failed to apply patch.
[ View ]

Added those requested docs.

+++ modules/user/user.module 11 Sep 2009 20:15:29 -0000
@@ -2304,39 +2304,79 @@ function user_role_delete($role) {
+  if (!is_object($role) || !isset($role->rid)) {
+    return FALSE;
+  }

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.

StatusFileSize
new10.53 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.28 KB
Failed: Failed to apply patch.
[ View ]

Re-rolled. Still looking good.

Status:Needs review» Reviewed & tested by the community

@@ -2301,39 +2301,69 @@ function user_role_delete($role) {
     ->condition('rid', $role->rid)
     ->execute();
+  module_invoke_all('user_role_delete', $role);
+
   // Clear the user access cache.
   drupal_static_reset('user_access');
   drupal_static_reset('user_role_permissions');

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.

sun - you and i simply disagree on what clean, modular code means for this particular pattern. Thats OK - happens all the time.

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks.

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

Assigned:dropcube» Unassigned
Status:Fixed» Needs review
StatusFileSize
new11.05 KB
Passed: 13509 passes, 0 fails, 0 exceptions
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Hah, didn't even think of so many clean-up possibilities due to this patch - awesome! :)

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks!

Status:Fixed» Reviewed & tested by the community

I don't actually see a CVS commit for this. :/

Status:Reviewed & tested by the community» Fixed

Ok, *now* it's committed to HEAD. ;)

Thanks Angie! :)

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