Use vocabulary machine name for permissions

Ouch, definite +1

We should have an upgrade path, though.

subscribe

subscribe

Tested out the patch, and very much yes please! If it gets an upgrade path can we have it in 7.1?

subscribe

subscribe.

... and, 8.x first.

Rerolled for D8.

subscribe

subscribe

subscribe

Patch in #12 works for me in D8.x HEAD.

Setting back to "needs work" for the upgrade path work to be done.

Do we need to add/update any tests for this?

First real crack at this, so feedback welcome. Was, and still am a little confused as to what update number this should take on? Is it 8001? 7011? Is it different if it gets back-ported to D7? Here is a patch against 8.x anyway. I couldn't see a need for, nor think of any appropriate tests, but, perhaps I'm wrong there.

Marked #986168: Use machine name for taxonomy permissions as duplicate.

Copying my comment from that issue since I think none of the points raised there have been fixed by the current patch:

Using the machine name instead of vid in the permission name certainly makes sense from a DX perspective. A couple of issues with the current patch though:

• Contrary to vids, machine names can actually change during the life-time of a vocabulary. You'll have to come up with a mechanism to determine when the machine name is being altered, and update the permissions accordingly
• Machine names can be up to 255 chars, while permissions are limited to 128 chars, which means the above patch breaks when using ridiculously long machine names for a vocabulary

Reviewing your patch also made me look for the original patch that introduced these permissions. Take a look at #340652: Edit/delete terms permission per vocabulary, it has in-depth information on vid versus machine name. If you go through with this patch, make sure you address the issues raised in that thread. IIRC, one of the concerns raised in that thread is the possible performance penalty for mulitiple additional taxonomy_vocabulary_loads.

subscribe

Marked #1300962: Change taxonomy permissions to use $vocabulary->machine_name rather than vid as duplicate of this issue.

I think this patch addresses the majority of the concerns in #18:

• Updates the role_permission table on vocab machine name change
• Extends permission field in role_permission to 255 (things could still break here on really long vocab machine names, so that may need work)
• Added a test to verify role permission table is updated
• Addressed a TODO item around old machine name.

Also, I removed the update hook for now, which won't be needed if this can be backported to 7.x.

Looks like it has test coverage, which is good. Few minor points about the patch:

1. +++ b/core/modules/taxonomy/taxonomy.moduleundefined
   @@ -588,6 +585,29 @@ function taxonomy_taxonomy_vocabulary_update($vocabulary) {
   + * Updates corresponding permissions in the role_permission table when a
   + * vocabulary machine name changes.
   

   The function summary should be one line less than 80 characters (probably just a matter of rewording it a little).

2. +++ b/core/modules/taxonomy/taxonomy.testundefined
   @@ -207,9 +207,11 @@ class TaxonomyVocabularyUnitTest extends TaxonomyWebTestCase {
   +    $admin_user = $this->drupalCreateUser(array('create article content', 'administer taxonomy', 'edit terms in ' . $this->vocabulary->machine_name, 'delete terms in ' . $this->vocabulary->machine_name));
   

   Can we reformat this using the multi-line array formatting for readability?

3. +++ b/core/modules/taxonomy/taxonomy.testundefined
   @@ -370,6 +372,21 @@ class TaxonomyVocabularyUnitTest extends TaxonomyWebTestCase {
   +      $this->assertTrue(!empty($exists), t('Found renamed permission %permission in the role_permission table.', array('%permission' => $permission)));
   

   Let's remove t() from the assertion message. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

Regarding whether this is backportable... I could easily see a contributed module interacting with vocabulary permissions and using edit terms in $vid where $vid comes from some vocabulary picked by the module for some purpose. So I'm leery of backporting this. Can someone convince me I'm being paranoid?

The function summary should be one line less than 80 characters (probably just a matter of rewording it a little).

Function summaries need to be less than 80 characters? I thought they just needed to wrap at 80.

Addressed the bits in #23.

#25 looks good to me. So the next step is to decide whether we'll actually backport this. If so, I think #25 is RTBC. If not, then we'll want to add an update hook and upgrade path tests to the D8 patch (both to update the field size and to update existing role names in the database). More thoughts on the backport question, anyone?

Edit:

Function summaries need to be less than 80 characters? I thought they just needed to wrap at 80.

Yep, the first line needs to be a summary that is one line only. Reference: http://drupal.org/node/1354#functions

More thoughts on the backport question, anyone?

I don't think this is a candidate for backporting, for the same reason you brought up in #23: IMO names for permissions exposed by Drupal core are part of its API, and API changes should not be back-ported.

+++ b/core/modules/user/user.installundefined
@@ -62,7 +62,7 @@ function user_schema() {
+        'length' => 255,

While it would be silly to use names that long, this can still cause a database error with vocabulary machine names longer than 240'ish characters.

While it would be silly to use names that long, this can still cause a database error with vocabulary machine names longer than 240'ish characters.

So you are suggesting adding code to truncate the machine name dynamically before sticking it in the permission name?

Closed #1449726: Vocabulary permissions should use machine name as duplicate of this issue.

Closed duplicate #1592172: Use vocabulary machine name instead of vid in permissions

EDIT this issue could be superseded by #1552396: Convert vocabularies into configuration

Re-rolled for D7.15 to avoid bad hunks.

Going through old issues referenced in Views @todos.

I noticed that there's a testTaxonomyVocabularyChangeMachineName() test for changing a vocabulary's machine name, im guessing permissions won't be changed.

Question is, should they?

#31: 995156-31_portable_taxonomy_permissions-D7-15.patch queued for re-testing.

See #30, I suggest to close this issue in favor of #1552396: Convert vocabularies into configuration, the result of that is that $vocabulary->vid *is* the machine_name, there's no id anymore. So if this would be commited, the other issue would just have to revert it.

Agreed.

AFAICT, #1552396: Convert vocabularies into configuration is not going to be backported to D7 for perhaps obvious reasons (API change, etc.)

Nevertheless, we need some fix for this in D7 anyway, which the http://drupal.org/project/taxonomy_access_fix module aims to provide.
That module is waiting for _this_ issue to be backported, though...

What does that mean for _this_ issue and the related issue in taxonomy_access_fix:
#1637446: Taxonomy Access Fix additional permissions should use vocab machine name and not vid

@Leeteq: Because the D8 change turned into a bigger architectural change, D7 is left out in the cold. Lets focus on improving taxonomy_access_fix.

This could still be fixed for D7 even though the approach used for D8 is radically different.

It might be one of the patches in this issue or the patch in http://drupal.org/node/1300962

I am using this patch on D7 for a year, waiting it to be commited because we need to use machine names to improve scalability. Today, Features includes a bit of code to bypass this issue, using a complicated conversion system whereas this could be in core since a long time.

Here is the patch found in #31 including an upgrade path.

What do we need to have this commited now ?

The concerns from #18 perhaps:

• Contrary to vids, machine names can actually change during the life-time of a vocabulary. You'll have to come up with a mechanism to determine when the machine name is being altered, and update the permissions accordingly
• Machine names can be up to 255 chars, while permissions are limited to 128 chars, which means the above patch breaks when using ridiculously long machine names for a vocabulary

and maybe the concern from the original issue #340652: Edit/delete terms permission per vocabulary:

Couple of things - since term objects don't contain the name of their parent vocabulary, only the vid, I've used that to differentiate the permissions instead of vocabulary->name - otherwise we'd be loading vocabularies on term pages just to check a permission... Since we have permission titles and descriptions now, this only affects the values in the database, and saves a query or two.

Ok ! Thank you for the pointers.

Contrary to vids, machine names can actually change during the life-time of a vocabulary. You'll have to come up with a mechanism to determine when the machine name is being altered, and update the permissions accordingly

This can easily be addressed as we can read the $vocabulary->original->machine_name property during the taxonomy_vocabulary_save. (See the attached patch)

Machine names can be up to 255 chars, while permissions are limited to 128 chars, which means the above patch breaks when using ridiculously long machine names for a vocabulary

What would be the best solution for this issue ?

• restrict vocabulary machine names to 64 characters (like many other machine names in the core),
• hash machine names longer than X characters in the permission name (Views is using something like that),
• - feel free to insert your idea here -

Couple of things - since term objects don't contain the name of their parent vocabulary, only the vid, I've used that to differentiate the permissions instead of vocabulary->name - otherwise we'd be loading vocabularies on term pages just to check a permission... Since we have permission titles and descriptions now, this only affects the values in the database, and saves a query or two.

We have now a vocabulary_machine_name key in the term object so I don't think this remains a problem. Plus, the only function which seems to need to load the vocabulary is taxonomy_form_term and it needs it for other purposes.

The attached patch already fixes the first point but I need some help to think about the second one.

Keeping sync with changing machine names looks good.
I'm quite ok with limiting vocabulary machine names to 64 chars (or even shorter). But nothing is ever easy, so how to deal with already-existing names that are longer?
I do agree on the performance considerations.

Here is a new patch implementing something like Views do which truncate the longest machine names and use their hash to shorten them. I don't think we have any better option but suggestions are welcome.

PS : machine names are only truncated in permissions names.

I like it. Thanks!

I only reviewed it without extensive testing and I am no core/taxonomy maintainer so I'm not setting to RTBC.
Let's hope someone else can ensure this does get committed sometime before the D8 release.

Here is a minor update to fix an issue when the old machine name was already containing more than 112 chars.

I looked over the patch and quickly reviewed the "use hash when machine name is too long" and it looks good! I haven't actually manually tested this patch, though. It seems strange that this doesn't break any unit tests - maybe more test coverage is needed?

This patch would break permissions for existing contrib and custom modules in Drupal 7. I don't think this is a safe D7 backport at all. Even if we upgrade permissions in the site database, permission names are very frequently hardcoded as strings in code, and this patch would break that code. It's especially risky considering that this is an access control change, and there are contrib alternatives possible.

So, marking wontfix.

For everyone who wants this in D7, the Features approach isn't perfect but it just works...

There's also a patch floating around the Taxonomy Access Fix issue queue to update it too (although the module maintainers won't commit it)

@xjm: Ah, yeah, that makes perfect sense. :-)

Here is the issue with patch for the Taxonomy Access Fix module: #1637446: Taxonomy Access Fix additional permissions should use vocab machine name and not vid

Let's pick this up in contrib!

This is a re-roll of the patch from #995156-46: Use vocabulary machine name for permissions against Drupal 7.36

This re-roll was necessary due to a series of unfortunate circumstances. ;-)