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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vasike’s picture

Status: Active » Needs review
FileSize
4.69 KB

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.

silkogelman’s picture

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.

vasike’s picture

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.

vasike’s picture

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

silkogelman’s picture

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!

wulff’s picture

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.

arnested’s picture

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.

DamienMcKenna’s picture

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

This needs to be rerolled for v2.

DamienMcKenna’s picture

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.93 KB

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

DamienMcKenna’s picture

Never mind about #1911946.

DamienMcKenna’s picture

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.

DamienMcKenna’s picture

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

cbeier’s picture

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.

cbeier’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.02 KB

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.

HnLn’s picture

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

NaX’s picture

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
hefox’s picture

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

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
5.04 KB

This combines #15 and #17 into one patch.

jlapp’s picture

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

RobLoach’s picture

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

RobLoach’s picture

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.

hefox’s picture

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"

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
5.04 KB

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

Status: Needs review » Needs work

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

hefox’s picture

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

hefox’s picture

Status: Needs work » Needs review

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

mpotter’s picture

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.

jyee’s picture

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()?

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()

jyee’s picture

Status: Fixed » Needs review
FileSize
1.03 KB

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

briand44’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #31 worked for me.

bleen’s picture

#31 worked for me as well

RTBC++

gapple’s picture

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
hefox’s picture

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

kenorb’s picture

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.

mpotter’s picture

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.

steve.m’s picture

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