Currently we are inserting machine names for existing roles when the module is installed. And currently it is not possible to update the machine name of an existing role.

This is a problem for existing sites with existing roles. Let's say a Drupal distribution update is rolled out, it contains the new role_export module and has the new role ids in exported Features configurations. But the role already exists in the site, so when the Features are reverted the role id is not changed. This means that the Features are broken because they of course rely on the new id.

So I propose:

  • Implement hook_user_roles_update() to react on updated roles (i.e. if the machine_name is inserted we need an updated id to be consistent)
  • On update we will have to fix references in the users_roles and permissions DB tables
  • Remove the machine name auto-generation from hook_install(), because then the machine_name does not match the id.
  • Make existing roles editable in the UI so that they can be made exportable. Display a big fat warning that changing the machine name affects all non-core configuration items.
CommentFileSizeAuthor
#9 1323242-role-export-change-warning.patch1.2 KBklausi
#1 1323242-role-export-updating.patch2.8 KBklausi

Comments

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs review
StatusFileSize
new 1323242-role-export-updating.patch2.8 KB

And here is the patch. Displays a warning only after a role machine name has been changed. Maybe we should add a confirmation form when the machine name changes.

Steven Brown’s picture

Steven Brown commented

So if I understand this right. It sounds like the issue is if there is an existing role of "Blog Admin" and someone installs a Blog feature that has the role "Blog Admin" then Features will consider this to be "Overridden". Then if you try to revert the role you get the wrong id because we aren't updating the "machine_name" or "rid" upon update.

Trying to make sure I understand the scenario here.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Yes, you understood it correctly.

Steven Brown’s picture

Steven Brown commented

During the installation we had an update statement for all existing roles. This would have taken care of this issue because the role would have already been in existence. So upon installation of role_export it would have converted the name into a machine name and generated the id. Which this would now match the feature. So if you had to revert it should be fine.

I think I might be missing something here.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

What we did in the install function was inconsistent: we added a machine name to existing roles, but we did not update the role id. So if I would enable role export and then would export configurations that rely on that role I would export the old serial id in them, which of course would not work on other sites. But on the other hand, if we would update also the role id in the install function we would break existing configurations in the site that rely on the old role id. So I think it is best to not touch any existing role at all. Maybe a warning message after the installation would be good that says that existing roles where not changed and should be updated manually (if needed).

I think it can be dangerous to just change role ids upon installation. People might try the module on production sites and messing with their role system without asking might lead to critical bugs.

Steven Brown’s picture

Steven Brown commented

Tested and committed the patch made by klausi. http://drupalcode.org/project/role_export.git/commit/3cfda33

Steven Brown’s picture

Steven Brown commented
Status: Needs review » Fixed
fago’s picture

fago
Primary language German
Location Vienna
commented
Title: Updating existing roles » Allow updating existing roles
Status: Fixed » Needs work

+1 on not automatically coming up with machine names upon installation. We really should ensure machine-names and ids are matching, so user should be able to manually re-name (and re-id) the role. 

+++ b/role_export.module
@@ -128,6 +129,38 @@ function role_export_user_role_insert($role) {
+  if (empty($role->machine_name)) {
+    $role->machine_name = role_export_machine_name_gen($role->name);
+  }

Imho this shouldn't be there. I should be able to change my human readable role name and leave the machine name empty, in case I don't want to change the id and keep my configuration intact.

Also, there should be a warning in the #description of the machine name that tells me about the consequences of changing a machine name...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new 1323242-role-export-change-warning.patch1.2 KB

Here is a patch that adds #description and also displays a warning for old roles that do not have a machine name yet.

I disagree that we should accept empty machine names, that would be pretty inconsistent. I experimented with the machine name form element, but it gets auto-populated with the role name anyway. So it is never empty to the user editing the role.

Steven Brown’s picture

Steven Brown commented

Correct I utilized the machine_name field which automatically pulls data from a field named 'name' which already existed on the form.

fago’s picture

fago
Primary language German
Location Vienna
commented

I see - once we've the warning it should be fine that way.

Steven Brown’s picture

Steven Brown commented
Status: Needs review » Fixed

This patch works for me and has been committed. http://drupalcode.org/project/role_export.git/commit/9082784

Status: Fixed » Closed (fixed)

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