Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
vasikethere'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.
Comment #2
silkogelman CreditAttribution: silkogelman commentedI'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.
Comment #3
vasikeof 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.
Comment #4
vasike@s1l : with the last dev you shouldn't get this errors, as the vocabulary permissions are not included in the Kickstart features anymore.
Comment #5
silkogelman CreditAttribution: silkogelman commentedThanks @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!
Comment #6
wulff CreditAttribution: wulff commentedBecause 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.
Comment #7
arnested CreditAttribution: arnested commentedWulffs 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.
Comment #8
DamienMcKennaThis needs to be rerolled for v2.
Comment #9
DamienMcKennaRelated: #1911946: All taxonomy exports should use the machine name, not the vid
Comment #10
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll of #7 for 7.x-2.x.
Comment #11
DamienMcKennaNever mind about #1911946.
Comment #12
DamienMcKennaI tested out Albert's patch on a site and it works well. The steps I took:
This gets a +1 from me, and I think it's RTBC for the v2 branch.
Comment #13
DamienMcKennaI tested backwards compatibility:
However, when you do "drush fr" it says "Current state already matches defaults, aborting."
Comment #14
cbeier CreditAttribution: cbeier commentedThe patch needs to be rerolled to the latest -dev. The patch from #10 failed on the latest features 2.x release.
Comment #15
cbeier CreditAttribution: cbeier commentedRe-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.
Comment #16
HnLn CreditAttribution: HnLn commentedCould 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).
Comment #17
NaX CreditAttribution: NaX commentedI have test this and it works, in one instance I had a some PHP 5.4 notices.
Considering it is possible that taxonomy_vocabulary_machine_name_load can return FALSE maybe we should wrap it in an if statement.
Comment #19
hefox CreditAttribution: hefox commentedJust 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
Comment #20
DamienMcKennaThis combines #15 and #17 into one patch.
Comment #21
jlapp CreditAttribution: jlapp commentedPatch #20 worked well for me. I tested exporting, importing, overrides, and reverting (all through the UI, not drush).
Comment #22
RobLoach#20: features-n1722524-20.patch queued for re-testing.
Comment #23
RobLoachLooks pretty good... A couple of questions:
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.
Trailing whitespace.
Comment #24
hefox CreditAttribution: hefox commentedneeds 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"
Comment #25
DamienMcKennaFixed the whitespace, didn't make any other changes.
Comment #27
hefox CreditAttribution: hefox commentedTest failure unrelated, haven't looked into why, just same failure as a patch that had nothing to do with it
Comment #28
hefox CreditAttribution: hefox commented#25: features-n1722524-25.patch queued for re-testing.
Comment #29
mpotter CreditAttribution: mpotter commentedWhile 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.
Comment #30
jyee CreditAttribution: jyee commentedI'm not sure if it's worth re-opening this, but doesn't this always throw the warning message in user_permission_features_rebuild()?
$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()
Comment #31
jyee CreditAttribution: jyee commentedIt appears that moving _user_features_change_term_permission() up resolves the issue.
Comment #32
briand44 CreditAttribution: briand44 commentedPatch in #31 worked for me.
Comment #33
bleen CreditAttribution: bleen commented#31 worked for me as well
RTBC++
Comment #34
gappleHere'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.Comment #36
hefox CreditAttribution: hefox commentedPlease use do-not-test format for patch if patching for previous (e.g. 1.x) issues
Comment #37
kenorb CreditAttribution: kenorb commentedThe same issue here:
Patch #31 fixes the problem.
Comment #38
mpotter CreditAttribution: mpotter commentedCommitted #31 to 1722524.
Comment #40
steve.m CreditAttribution: steve.m commentedHas anyone tested #34 with 7.x-1.x?