Problem/Motivation

The current UI allows you to select a user role or individual permissions to export. The latter choices export the permission with its setting for all roles. The former only exports the role. With a typical site using lots of modules and having lots of permissions, having to click on each permission is not exactly desirable. Instead, the intuitive approach would include the permissions for a role with the role. This feels the same as adding a content type and automatically getting all the fields.

Proposed resolution

Modify user_role_features_export_render() to include the permissions assigned for each role; modify user_role_features_rebuild() to reload the permissions.

The individual modules that define each permission could be added as dependencies in user_role_features_export(). However, this seems unnecessary. If the permission is added to the database, but the module that implements this is not enabled, will this trigger a run-time error? Something tells me you will be exporting features for that module or will have it in a drush make file, profile, or equivalent.

Remaining tasks

Review, write test, document.

I would suggest renaming the 'perm' key used by _features_get_roles() and the user_permission routines to 'permissions.' The abbreviated key corresponds to the truncated hook_perm (which, of course, was renamed in D7).

User interface changes

I see no reason to add the individual permissions to the UI simply to provide an indication that they will be exported. To me, the permissions feel more like a configuration setting for the role (similar to the various properties for a field, which are not shown in the UI). However, others may find this departs from the Features approach to things. The sheer volume of permissions would simply clutter the UI and not add value. Also, to be fair, to export permissions by role (as opposed to all roles for each permission) would suggest displaying each exported permission once for each exported role it applies to. This, again, is UI noise without benefit. To provide the $pipe functionality in the context of the permissions for a role, the current permission routines would need to be modified to restrict their scope to a role.

API changes

None.

CommentFileSizeAuthor
#14 features-1411678-export-permissions-with-role-14.patch3.38 KBtobiasb
#13 features-1411678-export-permissions-with-role-13.patch3.33 KBtobiasb
#7 1411678-export-permissions-with-role.patch3.65 KBsolotandem
#1 1411678-export-permissions-with-role.patch1.52 KBsolotandem

Comments

solotandem’s picture

solotandem commented
Status: Active » Needs review
StatusFileSize
new 1411678-export-permissions-with-role.patch1.52 KB

Attached patch implements the proposed resolution.

Grayside’s picture

Grayside commented

Not every site using a given feature will use all the permissions. I understand the logic, but it sounds like the difference between managing configuration for an entire site vs. building an export that packages a specific use case. Can't muddle the latter to support the former.

solotandem’s picture

solotandem commented

Are you arguing that the configuration for an entire site is not a valid use of exportables? I can see permissions as being more often a site configuration concern than a specific use case.

Grayside’s picture

Grayside commented

I'm saying the primary use case of the Features module is not full export of site configuration.

hefox’s picture

hefox commented
Status: Needs review » Needs work

I agree with grayside; this, as part of features core, is too restrictive and undesirable. Since automatically added items cannot be removed, this would prevent users from just exporting some roles and *some* relavent permissions (and force them to export all permissions assigned to that role). Need to be careful with anything automatically added, specially when it doesn't fall under "dependency" territory.

I'm actually poking around a module where this would be perfect for -- http://drupal.org/sandbox/hefox/1413000, how about move this over there?

(Wrote a blog post about features auto detecting last night actually).

edit: Oh, and actually looked at the patch (before I glanced at it but didn't digest it). This would introduce two ways for permissions to be defined; ouch. I think this might actually be a duplicate or at least have a similair issue somewhere; some others were trying to redo how permissions were handled. Anyhow adding a features pipe to my sandbox that pipes in permissions for exported roles.

hefox’s picture

hefox commented
Version: 6.x-1.x-dev » 7.x-1.x-dev

Also, all feature requests need to go to 7.x

solotandem’s picture

solotandem commented
Status: Needs work » Needs review
StatusFileSize
new 1411678-export-permissions-with-role.patch3.65 KB

The attached patch does what was intimated in the "User interface changes" section of the issue summary, namely it provides a new export option of "role plus permissions." As mentioned above, including the permissions with the role simply feels right and is what most would expect, like coupling fields to content types. To argue consistently against this option, you should also recommend uncoupling fields from content types, etc. Yet, somehow the commentators above do not find the latter troublesome as "automatically added items [that] cannot be removed."

Grayside’s picture

Grayside commented

Actually, I wouldn't mind decoupling that. Which is part of the philosophy on the issues trying to turn the automatically added stuff to default "on" suggestions that can be turned off.

patrickd’s picture

patrickd commented

+1

hefox’s picture

hefox commented
Status: Needs review » Needs work

Having two ways to export roles + permissions is a bad idea user experience wise I suspect, and feels like it's ripe for conflicts and problems.

Check out hook_features_pipe_alter and what it can do.

bkonetzny’s picture

bkonetzny commented

Changes in patch from #7 still work for 7.x-2.x-dev.
I don't think this should be included in the current permission or role UI, as exporting these alone is a different use-case. I like the solution to have these three options seperated, so exporting a role, permission or "role with permissions" fit the different needs.

Vietyank’s picture

Vietyank commented

My version of Features is 7.x-1.0-rc2+0-dev

The following is from an issue I posted on our own bug tracking system. Local terminology included.

I have modified the permissions for the several roles which relate to group content publishing so that they can make posts sticky . I recreated the DA System Roles and Permissions feature on preview and used git to deploy it. I can see that the module has been deployed as the version of production is correct and the version of the module and feature is correct. The problem is, none of the permission changes I made get installed.

When I inspect the module, I can see that the new permissions are included in the module:

// Exported permission: grant sticky.
  $permissions['grant sticky'] = array(
    'name' => 'grant sticky',
    'roles' => array(
      0 => 'DA International publisher',
      1 => 'admin',
      2 => 'chapter publisher',
      3 => 'country publisher',
      4 => 'dennis test role',
      5 => 'region publisher',
      6 => 'site publisher',
    ),
    'module' => 'da_core',

When I look at the feature [on our production site] it is flagged as "needs review". I can see these new settings are listed as overriding the existing ones. I accept the review but nothing changes.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new features-1411678-export-permissions-with-role-13.patch3.33 KB

The attached patch fixes the warning:

Strict warning: Only variables should be passed by reference in user_role_plus_features_export_render()

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
StatusFileSize
new features-1411678-export-permissions-with-role-14.patch3.38 KB

Found one more warning:

Strict warning: Only variables should be passed by reference in user_role_plus_features_rebuild()

mpotter’s picture

mpotter commented

I'm still not convinced on the need for this. We are trying to decouple exports, not couple them together. For example, the recent work to decouple field bases from instances.

This patch would probably be better converted into a contrib module that people could use as desired. I'd like to keep core Features cleaner and not have multiple ways to export the same data.

hefox’s picture

hefox commented
Status: Needs review » Postponed

I agree w/ 15, poor usability, not needed for most

Also you could do a hook features pipe component alter and suggest all the permissions for that role without having to defining a new component (see https://drupal.org/project/features_pipexplosion for examples)

Postponed as a precursser to closed (won't fix) [as most don't notice when an issue goes into won't fix)

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Issue summary: View changes
Status: Postponed » Closed (fixed)

I created a new module for this use case. https://drupal.org/project/features_user_role_plus

joelstein’s picture

joelstein commented

FYI, I create yet another Features module that manages roles and permissions, adding the ability to export individual permissions per role: Features Role Permission.

delacosta456’s picture

delacosta456 commented

@tobiasb , @joelstein thanks for your work.

Please i would like the one we should use on a Panopoly distribution

thanks