Drupal core exports vocabulary permissions using the vocabulary id.
So you get "delete terms in 10", etc.

This creates a problem when the "auto increment increment" setting is not the same across setups.
If that setting is 1, the ids are 1, 2, 3, 4, 5; but if that setting is two (something that d.o does, for example) then the ids are 1, 3, 5, 7, 9.
This leads to a crash, since Features tries to import a non-existent permission.

Features could implement a workaround here, replacing the id with the machine name before export, and then back to the id on import.

Files: 
CommentFileSizeAuthor
#34 features-use-machine-names-for-taxonomy-permissions-1722524-33.patch4.92 KBgapple
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 export_vocab_perms-1722524-31.patch1.03 KBjyee
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es).
[ View ]
#25 features-n1722524-25.patch5.04 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es).
[ View ]
#20 features-n1722524-20.patch5.04 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 38 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#17 features-use-machine-names-for-taxonomy-permissions-php-5.4-1722524-15.patch1.11 KBNaX
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-php-5.4-1722524-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 features-use-machine-names-for-taxonomy-permissions-1722524-15.patch5.02 KBcbeier
PASSED: [[SimpleTest]]: [MySQL] 40 pass(es).
[ View ]
#10 features-use-machine-names-for-taxonomy-permissions-1722524-10.patch4.93 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 features-use-machine-names-for-taxonomy-permissions-1722524-7.patch5.45 KBarnested
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 use-machine-names-for-taxonomy-permissions-1722524-6.patch4.75 KBwulff
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch use-machine-names-for-taxonomy-permissions-1722524-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 2012-08-18-patchresult1.JPG167.43 KBs1l
#2 2012-08-18-patchresult1-before-after.JPG73.35 KBs1l
#1 machine_name4vocabulary_permissions-1722524-1.patch4.69 KBvasike
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]

there's a first patch for this.
with this i was able to create a feature and enable it for a vocabulary permission.

this one is not aware about the previous features vocabulary permissions.
i think there's more to be done here.

I've tested this by using the patched Features (dev 2012-Aug-08) when installing Commerce Kickstart v2 (beta1 and dev 2012-08-15)

right before and after step install profile 121 I get notices about
trying to get property of non-object in _user_features_change_term_permission()(line 995 ..)

Not sure yet if this is because of this patch as I did not test Commerce Kickstart + clean Features dev yet.

of course it doesn't work with Kickstart, as i said : "is not aware about the previous features vocabulary permissions."
To work we need to recreate the Kickstart features using the patched version.

or better to find a way to make it work also for old vocabulary permissions features.

@s1l : with the last dev you shouldn't get this errors, as the vocabulary permissions are not included in the Kickstart features anymore.

Thanks @vasike!
Bojan made me aware of it in #1694434: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null.
beta2 works great for me too!

I realize that basically this doesn't solve 'auto increment increment setting' problems, but it solves mine and makes Kickstart v2 work on my server.

Great work guys, Kickstart v2 is truly an amazing distribution. Thanks a lot for this, I owe you Commerce Guys one.
As I said earlier I'm happy to create a blogpost with a screencast about it applying it to the Dutch market or so.
Don't hesitate to PM me with toughts and ideas about anything I can do to help Kickstart.

cheers!

StatusFileSize
new4.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch use-machine-names-for-taxonomy-permissions-1722524-6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Because of a weird install profile issue, we've had to use a slightly modified version of the patch in #1 to make this work as expected.

StatusFileSize
new5.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Wulffs patch did not put the machine name in the exported comment.

I changed that and made sure the patch adheres better to the coding standards.

Version:7.x-1.x-dev» 7.x-2.x-dev
Status:Needs review» Needs work

This needs to be rerolled for v2.

Status:Needs work» Needs review
StatusFileSize
new4.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll of #7 for 7.x-2.x.

Never mind about #1911946.

Status:Needs review» Reviewed & tested by the community

I tested out Albert's patch on a site and it works well. The steps I took:

  • I generated a feature with a vocabulary that did not have any term permissions enabled for any roles, i.e. other than "administer terms" all of the individual term permissions were unchecked.
  • I manually changed the [featurename].features.user_permission.inc file to grant the 'administrator' role both the "edit" and "delete" permissions, to introduce a difference between the code and what was in the database.
  • I enabled the new feature.
  • Upon visiting the permissions admin page I saw that the permissions for the exported vocabulary had changed to match the altered values in the inc file, just as it should have done.

This gets a +1 from me, and I think it's RTBC for the v2 branch.

I tested backwards compatibility:

  • Going to the recreate page for an existing feature correctly pulls in the correct permissions if they had been exported previously.
  • Using the admin or "drush fu" to recreate a feature correctly updates it to use the new permissions.
  • Doing "drush fd" on a feature that had permissions defined in the vid format shows something like this:
    Component: user_permission
      array(
    <   'delete terms in 4' => array(
    ---
    >   'delete terms in grades' => array(
          'module' => 'taxonomy',
    <     'name' => 'delete terms in 4',
    ---
    >     'name' => 'delete terms in grades',
          'roles' => array(
            'administrator' => 'administrator',
    <       'editor' => 'editor',
          ),
        ),

    However, when you do "drush fr" it says "Current state already matches defaults, aborting."

Status:Reviewed & tested by the community» Needs work

The patch needs to be rerolled to the latest -dev. The patch from #10 failed on the latest features 2.x release.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 40 pass(es).
[ View ]

Re-roll of #10 for 7.x-2.x-dev (7.x-2.0-beta2). The patch has not significantly changed. I have the patch only reformatted so that it can be applied without error to the current code base.

Could this patch also take the extra permissions of http://drupal.org/project/taxonomy_access_fix into consideration. It adds a new permission (add terms in X) and thus provides a more logical approach to the the taxonomy permissions (with this module it's actually possible to restrict access on adding and editing terms in a specific vocabulary).

StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-php-5.4-1722524-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I have test this and it works, in one instance I had a some PHP 5.4 notices.

Notice: Trying to get property of non-object in _user_features_change_term_permission() (line 990 of /sites/all/modules/features/features.module).

Considering it is possible that taxonomy_vocabulary_machine_name_load can return FALSE maybe we should wrap it in an if statement.

Status:Reviewed & tested by the community» Needs work

Just a quick look.

IMO it'd be better to introduce two hooks/alters, one that changes edit term x to edit term x_name, and one that does the opposite (or better, one that calls for a map and uses that in both places) and then implement those hooks, as this is likely not the only module that has an issue like this

Status:Needs work» Needs review
StatusFileSize
new5.04 KB
FAILED: [[SimpleTest]]: [MySQL] 38 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This combines #15 and #17 into one patch.

Patch #20 worked well for me. I tested exporting, importing, overrides, and reverting (all through the UI, not drush).

#20: features-n1722524-20.patch queued for re-testing.

Looks pretty good... A couple of questions:

+++ b/features.moduleundefined
@@ -634,6 +634,18 @@ function features_get_info($type = 'module', $name = NULL, $reset = FALSE) {
+        // Rework the features array, to change the vocabulary permission
+        // features.
+        foreach ($row->info['features'] as $component => $features) {
+          if ($component == 'user_permission') {
+            foreach ($features as $key => $feature) {

Might be a bit out of scope, but should this be a features_get_info_alter() hook, or something along those lines. I see us wanting to alter additional things in regards to Feature information.

+++ b/features.moduleundefined
@@ -964,6 +976,29 @@ function features_hook_info() {
+    preg_match("/(?<=\040)([^\s]+?)$/", trim($perm), $voc_id);
+    $vid = $voc_id[0];    ¶
+    if (is_numeric($vid) && $type == 'vid') {

Trailing whitespace.

Status:Needs review» Needs work

needs work for whitespace

However, I'll reiterate -- I really don't want this to be vocabulary specific. Implement a hook of permission map changes that any module can use. There's other cases like this where a numeric id is in the permission name.

Actually, prob doesn't need to be permissions specific event. A hook to "when you see component x, change it to component y"

Status:Needs work» Needs review
StatusFileSize
new5.04 KB
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es).
[ View ]

Fixed the whitespace, didn't make any other changes.

Status:Needs review» Needs work

The last submitted patch, features-n1722524-25.patch, failed testing.

Test failure unrelated, haven't looked into why, just same failure as a patch that had nothing to do with it

Status:Needs work» Needs review

#25: features-n1722524-25.patch queued for re-testing.

Status:Needs review» Fixed

While I agree with hefox that there is probably a more general way to handle this, I've committed this to 895e65d for the sake of fixing what definitely needs it. We can always start a new issue to work on a more general patch later if it's really needed.

I'm not sure if it's worth re-opening this, but doesn't this always throw the warning message in user_permission_features_rebuild()?

<?php
function user_permission_features_rebuild($module) {
  if (
$defaults = features_get_default('user_permission', $module)) {
   
// Make sure the list of available node types is up to date, especially when
    // installing multiple features at once, for example from an install profile
    // or via drush.
   
node_types_rebuild();
   
$modules = user_permission_get_modules();
   
$roles = _user_features_get_roles();
   
drupal_static_reset();
   
$permissions_by_role = _user_features_get_permissions(FALSE);
    foreach (
$defaults as $permission) {
     
$perm = $permission['name'];
      if (empty(
$modules[$perm])) {
       
$args = array('!name' => $perm, '!module' => $module,);
       
$msg = t('Warning in features rebuild of !module. No module defines permission "!name".', $args);
       
drupal_set_message($msg, 'warning');
        continue;
      }
     
// Export vocabulary permissions using the machine name, instead of
      // vocabulary id.
     
_user_features_change_term_permission($perm, 'vid');
      foreach (
$roles as $role) {
?>

$defaults will contain the vocabulary permission by name (e.g. "edit terms in foobar"), but $modules references core generated permissions, so it will still have the vid (e.g. "edit terms in 5"), so for vocabularies it will always throw a warning... even if it manages to correct the permission string later with _user_features_change_term_permission()

Status:Fixed» Needs review
StatusFileSize
new1.03 KB
PASSED: [[SimpleTest]]: [MySQL] 36 pass(es).
[ View ]

It appears that moving _user_features_change_term_permission() up resolves the issue.

Status:Needs review» Reviewed & tested by the community

Patch in #31 worked for me.

#31 worked for me as well

RTBC++

StatusFileSize
new4.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch features-use-machine-names-for-taxonomy-permissions-1722524-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a re-roll of the patch for 7.x-1.x for those that may need it still, that includes the check that taxonomy_vocabulary_machine_name_load() returns a valid value.

Status:Reviewed & tested by the community» Needs work

Please use do-not-test format for patch if patching for previous (e.g. 1.x) issues

Status:Needs work» Reviewed & tested by the community

The same issue here:

Warning in features rebuild of stoneridge_careers. No module defines permission "administer panelizer taxonomy_term departments defaults".         [warning]
Warning in features rebuild of stoneridge_careers. No module defines permission "delete terms in departments".                                     [warning]
Warning in features rebuild of stoneridge_careers. No module defines permission "edit terms in departments".                                       [warning]

Patch #31 fixes the problem.

Status:Reviewed & tested by the community» Fixed

Committed #31 to 1722524.

Status:Fixed» Closed (fixed)

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

Has anyone tested #34 with 7.x-1.x?