Permissions not updated when relationship renamed

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.

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.

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.
    drupal_static_reset('user_relationships_types_load');
    $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?

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

This would also require an update function...

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.

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.

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.

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

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

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