Creating vocabularies with machine-names "List" or "Add" breaks links in taxonomy overview admin area.

We need this.... subscribe

ksenzee says anything you can bork through the UI is critical.

+++ modules/taxonomy/taxonomy.adminNEW.inc	2010-07-01 17:10:42.000000000 -0700
@@ -203,7 +203,11 @@ function taxonomy_form_vocabulary_valida
+    // Do not allow machine names to conflict with taxonomy path arguments.

based on this logic shouldn't we disallow "view" "edit" "feed" and (I think) "delete" also? I haven't tested this...

Powered by Dreditor.

re #3: I think the world of Katherine but I disagree with her on this point. "Anything you can bork through the UI is critical" is not a sensible rule. Is Drupal really useless unless this is fixed? It sounds to me like this can be addressed by saying "don't do that!" and then can be fixed post-7.0. So I suggest marking this normal priority.

http://acquia.com/drupal-7-get-real-get-dirty-get-it-done

Look - this kind of bug is why in places I changed the callback paths for things like managing content type and menus - without that, you have to create a path black list, which is always going to fail in some cases and is really a half-assed solution.

so - 2 possible solutions:

1) change the paths for the /add, etc

2) blacklist (as per the patch above).

Someone already (unwisely) must have decided on these current paths in D7, because they are not the same in D6. I'm sure that changing the paths, although it's the right thing to do, would break all kinds of things by now. Sigh.

The blacklist idea is a total hack though.

Other places in core where there are menu router items that have this potential problem (i.e. machine names being used in router paths that could conflict with other paths):
- system_menu: 'admin/config/regional/date-time/types/%/delete' -- the % for type here is a machine name, and there is also 'admin/config/regional/date-time/types/add'
- shortcut_menu: 'admin/config/user-interface/shortcut/%shortcut_set' -- this is a shortcut set name, and there is also 'admin/config/user-interface/shortcut/add-set' and several with ../shortcut/link/* etc.

I personally think we should fix the paths, although it will be painful at this point.

Changing paths is not that painful - needs to be done asap.

Fixed the tests.

oops - bad diff

What about the other spots with the same problem in #7?

Looks like there are still 4 tests failing due to bad paths.

Also, the other spots mentioned in #7 have not been addressed...

Thanks for testing...
- Agreed on the date-time paths -- I couldn't make it break either.
- On the shortcut sets, it appears that the machine name of shortcut sets is defined by the shortcut module to be shortcut-set-1, shortcut-set-2, etc. So the name you choose for the shortcut set cannot affect the path adversely.

For D7 I guess we allowed an extra level or two for router items - so these paths will work, though note that that there is some potential performance penalty for longer paths.

#17: 843162-taxonomy-vocab-paths-17.patch queued for re-testing.

Simple re-roll of the patch. Only had a single failed hunk in taxonoy.admin.inc.

I'm however wondering if this isn't an API break.. userpoints.module for example contains a link to the vocabulary page which it has defined and this patch is going to break that I think :)

Yes, I imagine it is too late to fix this in Drupal 7 by changing the paths.

We could conceivably "fix" this by not allowing people to save vocabularies whose machine names are "list" or "add".

Back to the idea of patch #1 then....

I took a look at taxonomy_menu(), and I'm quite certain that 'list' and 'add' are the only problematic vocabulary names.

So the ideas that have been proposed to solve this problem this are:
- Patch #1 - in validation for the vocabulary add/edit form, throw a form error if the machine name is 'list' or 'add'
- Patch #22 and others - changing paths - bad at this stage of D7
- Treat 'list' and 'add' as machine names that are in use and do whatever they do - I think this would be bad from a UI perspective, because the user will not understand what it means. For instance, they create their first vocabulary and happen to call it List, and it says "machine name list is already in use" or some such error message - that would be confusing.
- Modify machine names automatically - I also think this is not a good idea. We should let the user deal with the problem, not guess at a solution.

The problem is, the patch in #1 will not apply, because the form validate function that it was modifying no longer exists. The validation is now happening only in form_validate_machine_name(), which is calling taxonomy_vocabulary_machine_name_load() in order to verify that the machine name doesn't already exist.

So instead what we'd need to do is add an additional #validate to the form, and put in those 5 lines from patch #1.

Here's a patch. It works for me on my test box - prevents adding 'list' or 'add' machine names, but doesn't prevent anything else. Sorry, I couldn't resist also bringing the docblocks up to standards, since I needed to put some @see lines in there... hope that's OK.

I'm wondering if this is something that we might want to add to the generic machine_name validate function? I guess there are more forms with machine_names which need a similiar validation, I myself have a module that does...

Looking at #902644: Machine names are too hard to implement. Date types and menu names are not validated, there are several #machine_name_something properties that allow to fine-tune the validation and other things. Theoretically, I guess it would be possible to do this through the regular expression but that is quite complicated. Maybe we could add something like this:

'#machine_name_blacklist' => array('list', 'add')

Also..

+    form_set_error('machine_name', t('The machine-readable name cannot be "add" or "list".'));

I don't think that we can add new strings at this point, string-freeze and so. Obviously it would be better UX than a generic "This machine name is not valid" error message but I guess we'll have to live with that...

I can create a patch, just wanted to ask for feedback before doing that...

I don't think there is an existing string error message that will do. Take a look at http://api.drupal.org/api/drupal/includes--form.inc/function/form_valida... -- you will see that all the messages there are quite specific. So the choices will be to use a message that is misleading/incorrect, ot to add a new string.

I do like your idea of adding a name blacklist to the machine name element as a better way to solve this.

And I'm not sure why the test failed. The function works just fine on my test machine.

Small update of patch: we should check is the machine name key is set, because this validation also passing during deletion, that's wy test crashes with Notice

+1 on this patch. Doc is good anyway.

Previous patch failed. Re-rolled patch from root to fix file paths.

Applied patches, ran tests, and tested all the validation functionality of the patch. Looks ready to go!

Needs to go to d8 and then d7

tagging for backport.

I'm willing to commit this patch in order to make progress with 7.x and 8.x. However, I'm not sure this is the most elegant fix for 8.x. Once we get this in in 7.x and 8.x as a stop gap solution, should we try to resurrect http://drupal.org/node/843162#comment-3912612 for D8? I'm recommending that we do.

Note to anyone that lands here looking at the Git commit history for the Update manager. Dries accidentally committed the fix for #1008328: Uniqueify update-cache and update-extraction directories to prevent "Permission denied" errors as http://drupalcode.org/project/drupal.git/commit/02dd3c5 which said:

- Patch #843162 by pwolanin, Scott Falconer, Berdir, jhodgdon, a.mikheychik, mtift: creating vocabularies with machine-names 'List' or 'Add' breaks links in taxonomy overview admin area.

Just leaving a pointer in case anyone ever tries to sort out the Git archeology and is wondering WTF happened... ;)

Cheers,
-Derek

RE #35 - agreed, for d8 we should eventually change the paths like the patch in #22 above. But probably not for D7.

Committed this patch to 7.x and 8.x. I'm changing the status field to 'needs work' and lowering the priority so we can discuss code clean-ups for 8.x.

Setting status to needs work (or should it be active? not sure..)

Is this a good issue to change a router paths all over core? suppose we should start new one and change all menus to format:
.../[module-entity]/[action]/[machine name and other parameters]

Also this could not be backported to D7 because of contrib modules that already use current paths.

I think if you want to change router paths all over core (again -- there were a LOT of changes for D7 too), it should be a separate issue.

Related issue #1552396: Convert vocabularies into configuration

Suppose it's now obsolete after #1978112: Convert taxonomy admin path to follow other core entity patterns

D8 uses common pattern for path after #1978112: Convert taxonomy admin path to follow other core entity patterns