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.

CommentFileSizeAuthor
#141 300993-drupalSetPermissions-D7.patch11.05 KBDave Reid
#134 drupal.user-role-set-permissions.134.patch10.28 KBsun
#132 drupal.user-role-set-permissions.132.patch10.53 KBsun
#130 drupal.user-role-set-permissions.130.patch11.02 KBsun
#126 drupal.user-role-set-permissions.125.patch9.93 KBsun
#123 drupal.user-role-set-permissions.123.patch10.21 KBsun
#122 drupal.user-role-set-permissions.122.patch10.11 KBsun
#120 drupal.user-role-set-permissions.120.patch10 KBsun
#115 drupal.user-role-set-permissions.115.patch8.23 KBsun
#113 drupal.user-role-set-permissions.113.patch8.22 KBsun
#111 drupal.user-role-set-permissions.111.patch8.19 KBsun
#109 drupal.user-role-set-permissions.109.patch8.19 KBsun
#107 drupal.user-role-set-permissions.107.patch8.19 KBsun
#104 drupal.user-role-set-permissions.104.patch8.36 KBsun
#103 drupal.user-role-set-permissions.103.patch7.71 KBsun
#101 drupal.user-role-set-permission.patch5.87 KBsun
#91 drupal.user-system-changelog.patch1.81 KBsun
#89 user-roles-API-300993-89.patch16.32 KBdropcube
#87 user-roles-API-300993-86.patch16.45 KBdropcube
#84 user-roles-API-300993-84.patch16.45 KBdropcube
#81 user-roles-API-300993-81.patch15.74 KBdropcube
#77 user-roles-API-300993-77.patch14.37 KBdropcube
#74 user-roles-API-300993-74.patch14.39 KBdropcube
#71 user-roles-API-300993-71.patch19 KBdropcube
#64 300993-drupalSetPermissions-D7.patch34.37 KBDave Reid
#63 300993-drupalSetPermissions-D7.patch36.84 KBDave Reid
#59 300993-drupalSetPermissions-D7.patch33.89 KBDave Reid
#55 300993-drupalSetPermissions-D7.patch30.27 KBDave Reid
#53 300993-drupalSetPermissions-D7-5.patch31.12 KBDave Reid
#50 300993-drupalSetPermissions-D7-4.patch31.81 KBDave Reid
#48 300993-drupalSetPermissions-D7-4.patch32.07 KBDave Reid
#44 300993-drupalSetPermissions-V2-D7.patch18.44 KBDave Reid
#43 300993-drupalSetPermissions-V2-D7.patch18.44 KBDave Reid
#33 300993-drupalSetPermissions-D7.patch13.68 KBDave Reid
#32 300993-drupalSetPermissions-D7.patch14.58 KBDave Reid
#31 300993-drupalSetPermissions-D7.patch14.67 KBDave Reid
#29 300993-drupalSetPermissions-D7.patch14.72 KBDave Reid
#20 300993-drupalSetPermissions-D7.patch14.75 KBDave Reid
#16 300993-drupalSetPermissions-D7.patch7.92 KBDave Reid
#14 300993-drupalSetPermissions-D7.patch7.65 KBDave Reid
#12 300993-drupalSetPermissions-D7.patch3.66 KBDave Reid
#10 300993-drupalSetPermissions-D7.patch3.64 KBDave Reid
#9 300993-drupalSetPermissions-D7.patch3.65 KBDave Reid
#4 300993-simpletest-drupalSetPermission-D7.patch1.46 KBDave Reid
#1 anonymous_user_perms.patch3.28 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
3.28 KB

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.

Dave Reid’s picture

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

Dave Reid’s picture

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

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

Dave Reid’s picture

Patch for #3.

jhedstrom’s picture

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.

jhedstrom’s picture

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.

boombatower’s picture

Dave Reid’s picture

Title: Create drupal web test case method for granting anonymous users permissions » Create public method for granting user permissions
FileSize
3.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.

Dave Reid’s picture

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.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
3.66 KB

Re-rolled to HEAD and a little cleaning.

Damien Tournoud’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

Status: Needs review » Needs work

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

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
7.92 KB

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

Damien Tournoud’s picture

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

Interesting... :)

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

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

Dave Reid’s picture

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

Dave Reid’s picture

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:

  protected function drupalLogin($user = NULL) {
    if ($this->isLoggedIn) {
      $this->drupalLogout();
    }

    if (!isset($user)) {
      $user = $this->_drupalCreateRole();
    }
...

After:

  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.

Dave Reid’s picture

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
Dave Reid’s picture

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

moshe weitzman’s picture

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

David Strauss’s picture

I agree with Moshe. This API belongs outside the testing framework.

Dave Reid’s picture

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.

David Strauss’s picture

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

Dave Reid’s picture

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

jhedstrom’s picture

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.

Dave Reid’s picture

Re-rolling for change to drupal_web_test_case.php

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
14.67 KB

Retry

Dave Reid’s picture

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

Dave Reid’s picture

Reposting after today's simpletest commits.

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Testing failure...

Dave Reid’s picture

Status: Needs work » Needs review
moshe weitzman’s picture

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

webchick’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

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.

webchick’s picture

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

webchick’s picture

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

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
18.44 KB

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

Dave Reid’s picture

oops...had two different function names.

Dave Reid’s picture

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.

Dave Reid’s picture

Status: Needs review » Needs work

Patch could use some polish and a role administration test.

Dave Reid’s picture

Dave Reid’s picture

Title: Create public method for granting user permissions » Improve the role and permissions API
Component: simpletest.module » user.module
Status: Needs work » Needs review
FileSize
32.07 KB

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:

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

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
31.81 KB

Removed the already fixed whitespace changes in DWTC.

Dave Reid’s picture

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.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
31.12 KB

Ok, actually installable and testable now. :)

moshe weitzman’s picture

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.
Dave Reid’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
33.89 KB

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.

Dave Reid’s picture

Issue tags: +permissions, +user roles

Adding tags

Dries’s picture

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.

Dave Reid’s picture

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

Dave Reid’s picture

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

Dave Reid’s picture

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.

moshe weitzman’s picture

Any chance Dave can reroll this?

Dave Reid’s picture

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.

moshe weitzman’s picture

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

boombatower’s picture

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

dropcube’s picture

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.

dropcube’s picture

Title: Improve the role and permissions API » User roles and permissions API
Status: Needs work » Needs review
FileSize
19 KB

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.

Dries’s picture

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.

dropcube’s picture

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

kyle_mathews’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Needs work
dropcube’s picture

Status: Needs work » Needs review
FileSize
14.37 KB

Removing debug() from patch.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

webchick’s picture

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?

mattyoung’s picture

subscribe

dropcube’s picture

Status: Needs work » Needs review
FileSize
15.74 KB

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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

AFAICT, we are there.

webchick’s picture

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.

dropcube’s picture

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.

dropcube’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

dropcube’s picture

Status: Needs work » Needs review
FileSize
16.45 KB

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.

dropcube’s picture

Status: Needs work » Needs review
FileSize
16.32 KB

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

Dries’s picture

Status: Needs review » Fixed

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

sun’s picture

Status: Fixed » Needs review
FileSize
1.81 KB

At your service. :)

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD.

webchick’s picture

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

Docs, plz!

dropcube’s picture

Priority: Critical » Normal
catch’s picture

Status: Needs work » Fixed
sun’s picture

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

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.

catch’s picture

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

sun’s picture

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

1)

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

2)

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

webchick’s picture

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

sun’s picture

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.

sun’s picture

Status: Active » Needs review
FileSize
5.87 KB

Neaty.

sun’s picture

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

sun’s picture

sun’s picture

Forgot to update the PHPDoc.

sun’s picture

Tests pass - RTBC anyone? :)

dropcube’s picture

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.

sun’s picture

Nice nits!

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

bah, stupid typo.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

bah, another stupid typo ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.22 KB

wow, this time for real - sorry! :)

Dave Reid’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
8.23 KB

Re-worded after IRC discussion :)

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

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

dropcube’s picture

RTBC, as far as I can see...

webchick’s picture

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

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
10 KB

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.

Dave Reid’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
10.11 KB

I love you! :)

sun’s picture

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

Dave Reid’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
9.93 KB

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.

sun’s picture

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

dropcube’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
11.02 KB

Added those requested docs.

Dries’s picture

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

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
10.28 KB

Re-rolled. Still looking good.

moshe weitzman’s picture

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.

sun’s picture

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

moshe weitzman’s picture

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

sun’s picture

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

ebeyrent’s picture

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?

Dave Reid’s picture

Assigned: dropcube » Unassigned
Status: Fixed » Needs review
FileSize
11.05 KB

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Dave Reid’s picture

Status: Fixed » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Dave Reid’s picture

Thanks Angie! :)

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