It was a bit unexpected that a role would be created by enabling a feature. I would expect it to set the permissions for existing roles only. I'm sure there's reasoning behind this. Thanks!

CommentFileSizeAuthor
#12 553866_user_role.patch6.3 KByhahn
#4 features_roles.diff1.06 KBq0rban

Comments

q0rban’s picture

q0rban commented

After looking at user_features_revert(), I noticed that roles aren't created there either, which makes sense. So this is broken one way or another. If roles are created when you enable a feature, they should also be created when reverting (What happens if you update the feature to include new roles). I'm not saying it should work that way obviously.. ;)

Role creation just doesn't make much sense to me, especially when you think about disabling a feature. If it creates roles, those roles will still exist after disabling the feature, which is messy IMO.

q0rban’s picture

q0rban commented

On further thinking, I would think that the best way to do this would be to create a new hook for user roles: hook_user_default_roles(). That way a module could list default roles and default permissions separately, and be able to define permissions for roles that may or may not exist. Make sense?

q0rban’s picture

q0rban commented
Title: Do not add roles if non-existent » Move role creation to hook_user_default_roles()

Sorry for so many posts. Re-titling

Another problem here is that a component can only have one hook. Not sure I understand why that is, but that would mean that the user component can't have a permissions hook and a roles hook, correct?

q0rban’s picture

q0rban commented
Status: Active » Needs work
StatusFileSize
new features_roles.diff1.06 KB

Here's a patch for step one of this (removing role creation from permissions defaults)

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

IMO creating roles is the correct behaviour. Maybe what is needed here is thinking about having settings per component, so for example one can decide for roles not to be created.

q0rban’s picture

q0rban commented

Thanks for the feedback..

IMO creating roles is the correct behaviour.

Can you describe why?

Here's why I think its incorrect. I would much rather it just set the permissions for the role if the role exists and not create the role. That way you can have multiple multiple roles in the feature with permissions saved for each role, and if the role exists, the permissions are set properly. Otherwise, there's not a good way for the feature to clean up after itself after being disabled/uninstalled. You don't want the feature deleting roles, IMO.

Thanks!

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

> Can you describe why?

I think both cases are valid. For me a feature is something you can turn on and off. When you turn on - you get everything to work including the roles. When you turn off, all the things related to the feature needs to be cleaned-out.

q0rban’s picture

q0rban commented

When you turn off, all the things related to the feature needs to be cleaned-out.

I totally agree, and this is why I think it should *not* create roles, as there is no way for features to remove roles safely.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented

Agreed that automatically creating every role that has a given permission is not the expected behaviour.

But we do need role creation if it's desired. So, yes, it should be possible to explicitly designate roles to be added.

yhahn’s picture

yhahn commented
Assigned: Unassigned » yhahn

FYI I am in agreement with the direction here, just haven't had a chance to fully review the patch.

yesct’s picture

yesct commented

I wanted to add roles to my feature, as a component. Is this issue addressing that too?
Would it help if it did?

How about:

If a role did not exist, and was not explicitly included in the feature under "add components", even if a permission for it was included in the feature, then it would not create the role.

If the role was added as a component, and did not exist, it would create it.

If a permission was added as a component, the role did not exist, and the role was not added as a component, then the role would not be created.

If the role existed (even if it was not a component in the feature) and a permission for it was a component, then the permission would be added. (this is the way it works now, yes?)

And on deleting roles...

If the role existed already, and it was part of a feature that was enabled, can it track that, and when the feature is disabled, the role is NOT removed. If the role did not exist before the feature was enabled and the feature created it, when the feature is disabled, the role IS removed?

yhahn’s picture

yhahn commented
Status: Needs work » Needs review
StatusFileSize
new 553866_user_role.patch6.3 KB

This patch implements new and separate components user_permission and user_role and deprecated the old user component. It provides a pipe that will migrate any user components to user_permission on a Features update or recreate.

Note that the hook for default permissions remains the same so old Features will continue to work with the exception that override detection and reverting will not work until the Feature is updated to the new format.

If you have concerns about this change please voice your opinions now! : )

yhahn’s picture

yhahn commented
Status: Needs review » Fixed

Committed http://drupal.org/cvs?commit=396964

Status: Fixed » Closed (fixed)

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