Steps to recreate:

1. Create a relationship
2. Go to admin/people/permissions and select Have, Maintain, Request, Delete permissions for a role
3. Edit the relationship from Step #1 and modify the name - I changed the first letter from lowercase to uppercase
4. Reload admin/people/permissions and notice Have, Maintain, Request, Delete permissions are no longer selected
5. Select Have, Maintain, Request, Delete permissions and save the form - notice they still appear unselected

1. I assume removing the relationship and re-adding it would fix the problem, but I did not test this I used workaround #2
2. Manually changing the entries in the database table role_permission from the former name to the changed name definitely fixed the problem

#6 user_relationships-key_permissions_by_type_id-1335950-4.patch8.65 KBtmsimont
FAILED: [[SimpleTest]]: [MySQL] 458 pass(es), 384 fail(s), and 212 exception(s). View


Berdir’s picture

Hm, just wondering, what happens if you do the same with e.g. a content type?

That 4. happens is to be expected and also happens with content types I think, but 5. sounds weird, there is no reason something like that should happen.

joel_osc’s picture

Very good question, with an interesting answer since content-types have both a name and a machine-name.

Test #1: Change name of content-type
Result #1: Permission name changes properly and setting is maintained
ex. "Article: Edit any content" became "Article Test: Edit any content"

Test #2: Change the machine name of the content-type (ex. article changed to article_test)
Result #2: Permission setting is lost, but can be selected and saved again
- in addition changing the machine-name back the original setting reappeared

The results in #2 kinda make sense given that you changing the machine-name. And, as you say the issue really is #5 in the above where the permissions cannot be re-saved.

tmsimont’s picture

This comes from hook_permission():

  foreach (user_relationships_types_load() as $type) {
    $permissions['can have ' . $type->name . ' relationships'] = array(
      'title' => t('Have %name relationships', array('%name' => $type->name)),
      'description' => t('The user may have relationships of this type.'),
    $permissions['maintain ' . $type->name . ' relationships'] = array(
      'title' => t('Maintain %name relationships', array('%name' => $type->name)),
      'description' => t('The user may approve or decline relationship requests of this type.'),
    $permissions['can request ' . $type->name . ' relationships'] = array(
      'title' => t('Request %name relationships', array('%name' => $type->name)),
      'description' => t('The user may request relationships of this type.'),
    $permissions['delete ' . $type->name . ' relationships'] = array(
      'title' => t('Delete %name relationships', array('%name' => $type->name)),
      'description' => t('The user may delete current relationships of this type.'),

The key of the $permissions array should use $type->rtid, not $type->name. This is why in D7 perms are arrays, so we can be more mechanical in defining permissions while allowing the readable permissions to be more human-friendly.

I'd put up a patch, but I don't personally feel comfortable changing it, as I can see there being a high potential for the permission to be called by user_access() in a number of places with the $type->name value.

tmsimont’s picture

Component: User interface » Code
tmsimont’s picture

OK, i grep'd the text "can have " to see how many files were referencing that permission directly, and it seems like a lot of them are going through an abstraction of user_access, so attached is a patch. There's one test that needs to be updated still, but I'm not sure how to get the rtid of freshly created relationship type. See: ./user_relationship_privatemsg/user_relationship_privatemsg.test function testSendToRelationship()

   // Create relationship.
    $relationship = array(
      'name' => $this->randomName(),
      'plural_name' => $this->randomName(),
      'requires_approval' => FALSE,
      'expires_val' => 0,
    user_relationships_type_save((object) $relationship);

    // Flush permission cache.
    $this->checkPermissions(array(), TRUE);

    $have_permission = 'can have ' . $relationship['name'] . ' relationships';
    $request_permission = 'can request ' . $relationship['name'] . ' relationships';
    $maintain_permission = 'maintain ' . $relationship['name'] . ' relationships';

How can you get the permissions to use the rtid's in this test?

tmsimont’s picture

Status: Active » Needs review
8.65 KB
FAILED: [[SimpleTest]]: [MySQL] 458 pass(es), 384 fail(s), and 212 exception(s). View
tmsimont’s picture

crap even if that passes testbot I think there are even more changes required -- the User Relationships UI module also keys permissions with type name instead of id...

tmsimont’s picture

This would also require an update function...

Berdir’s picture

I'm not convinced that this is the right thing to do.

For example, there was an issue in Core to make taxonomy permissions use the vocabulary name and not the id. While they are not and won't be any time soon I guess types should actually be exportable entities. So you could put them into features/code, including related permissions.

The update function that this will require won't be any easier than writing a function that updates the permissions table if the name changes, which shouldn't happen that often? Unlike 6.x, renaming a permission is easy in 7.x as they are in a separate table.

tmsimont’s picture

I hear your concern about using the rtid... What about creating a machine name that stays locked after the relationship is created? That's how content types work, right?

I think the biggest concern is just the unexpected loss of permissions that occurs when you change a relationship name.

Berdir’s picture

Content type machine names can be changed. And I just checked and you lose all permission configuration if you do that. even the administrator role permissions are gone. So I don't worry too much about this if not even content types can handle it :)

IMHO, we have to options:
- Update the permission names on a change directly in the database. A bit more involving. And as I said, we have to write the logic for this anyway, either in a save method or a update function.
- Display a message and tell the user that he needs to re-configure the permissions. Making the unexpected loss a bit less unexpected.

tmsimont’s picture

Option 1 sounds great, but definitely more involved.

If content types don't even do it right, I like option 2... But maybe use that message as help text underneath the name value so the user would know before making the change rather than after

Status: Needs review » Needs work

The last submitted patch, user_relationships-key_permissions_by_type_id-1335950-4.patch, failed testing.

tmsimont’s picture

Category: bug » feature
Priority: Normal » Minor
mrded’s picture

Status: Needs work » Needs review

Hey guys, please check my patch, it should fix your issue.
#1980610: Add machine name to user_relationships

Andre-B’s picture

Status: Needs review » Fixed

#1980610: Add machine name to user_relationships is the cleaner solution for this

Status: Fixed » Closed (fixed)

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