Originally mentioned in #614030-24: Cannot edit fields for taxonomy terms and now repeated in #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc.:

The usage of the vocabulary id instead of the vocabulary's machine name in admin paths:

admin/structure/taxonomy/$vid[/...]

is a problem for Field UI (as fields are bound to bundle names, i.e. machine names), but also contributed modules who try to work with those administrative paths (f.e. admin_menu).

In general, the additional/historical vocabulary id in addition to a machine name seems superfluous, but at this stage, we can only fix those paths.

CommentFileSizeAuthor
#24 744258-24.patch944 bytesjpmckinney
#23 744258-23.patch937 bytesjpmckinney
#22 744258-22.txt937 bytesjpmckinney
#15 744258-unique-key.patch922 bytescatch
#5 drupal.taxonomy-admin-paths.5.patch14.92 KBsun
#2 drupal.taxonomy-admin-paths.2.patch12.19 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I think we should probably drop vocabulary ID in D8 - although note if we do that it means denormalizing the machine name to the taxonomy_term_data table - we'll need to check query efficiency since many, many taxonomy queries include vid.

It would be easy to add taxonomy_vocabulary_machine_name_load() as a wrapper around taxonomy_vocabulary_get_multiple() so that we can continue to use proper menu loaders on those paths. Is there any thing else to do there apart from updating some links?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal.taxonomy-admin-paths.2.patch12.19 KB

Right, basically just those paths.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Looks great. Please wait for test bot before commit.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: Unassigned » sun
Status: Reviewed & tested by the community » Needs work

hah, I knew there'd be some hiccup. Not sure when I'll be able to get back to fix those, but feel free to beat me to it :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal.taxonomy-admin-paths.5.patch14.92 KB

Fixed those tests.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Reviewed & tested by the community

Looks good.

Dries’s picture

Dries CreditAttribution: Dries commented

I'm not sure I understand the problem this patch solves. The issue description is rather cryptic.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

@Dries, all the background is in #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc..

Dries’s picture

Dries CreditAttribution: Dries commented

I re-read #742318 but I still don't get it. Could be my jetlag.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The current administrative menu paths for taxonomy are using:

admin/structure/taxonomy/$vid/fields/...

$vid, however, is not the name of the bundle, to which fields are attached to.

Fields are attached to $vocabulary->machine_name in http://api.drupal.org/api/function/taxonomy_entity_info/7

In reality, the vid should have been removed when machine names for vocabularies were introduced. It's superfluous now.

Modules that want to do something with taxonomy bundles or fields, or integrate with those administrative menu paths for vocabularies need to do some ugly hick-hacks to translate back and forth the vid into machine_name and possibly back:

function field_ui_admin_menu_map() {
  // @todo D7: Taxonomy still uses vid instead of machine name in administrative
  //   paths.
  if (module_exists('taxonomy')) {
    $vocabularies = taxonomy_vocabulary_get_names(); <---------- ugly, ugly, ugly
  }

  $map = array();
  foreach (entity_get_info() as $obj_type => $info) {
    foreach ($info['bundles'] as $bundle_name => $bundle_info) {
      switch ($obj_type) {
        case 'taxonomy_term':
          $fields = array();
          foreach (field_info_instances($obj_type, $bundle_name) as $field) { <------ $bundle_name is the real bundle name
            $fields[] = $field['field_name'];
          }
          // Map machine_name to vid.
          $vid = $vocabularies[$bundle_name]->vid; <------------- but the menu system needs translation from bundle name to voabulary to vid
          $arguments = array(
            '%taxonomy_vocabulary' => array($vid),
            '%field_ui_menu' => $fields,
          );
          break;
      }
    }
  }
  return $map;
Dries’s picture

Dries CreditAttribution: Dries commented

But the patch doesn't remove the ugly hick-hacks (love that term)? Would that be a follow-up patch? Right now, this patch adds more code than it removes.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

oh, that code snippet I pasted above is from a contributed module, admin_menu. It merely outlines the problem of other modules with the current paths.

But yes, this patch is also a preparation for #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc., which ultimately will revert the recently introduced changes of #614030: Cannot edit fields for taxonomy terms

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

OK, that all makes sense now. Thanks for the various clarifications -- that helps. Committed.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Fixed » Active

When you use our API, taxonomy_vocabulary_save(), there is no prevention of duplicate machine names. Nor is there any in the DB. I'm looking at several dupes right now, that I got just by installing same module few times.

We should at minimum add a UNIQUE index in the DB to match the assumption thats made here. At maximum, we add php checks when saving a vocab that the machine name is valid. Field API validates incoming data like this before it creates a field/instance.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs work
FileSize
744258-unique-key.patch922 bytes

Here's a patch to add the unique key.

The PHP check sounds good. We also likely need a safeguard when generated the machine names in the taxonomy update. I've not done either of those yet.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review

Reasonable follow-up

Status: Needs review » Needs work

The last submitted patch, 744258-unique-key.patch, failed testing.

bcn’s picture

bcn CreditAttribution: bcn commented
Status: Needs work » Needs review

Cycling bot... patch should still apply.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#15: 744258-unique-key.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 744258-unique-key.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

How could it break drupal install?

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented
FileSize
744258-22.txt937 bytes

I think you had the db_add_unique_key API wrong.

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented
FileSize
744258-23.patch937 bytes

Rename it .patch so bot picks it up?

jpmckinney’s picture

jpmckinney CreditAttribution: jpmckinney commented
FileSize
744258-24.patch944 bytes

Fix array returned by hook_schema, too. Sorry for the triple-post.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Seems related #762604: Taxonomy terms list broken, term add/edit forgets it's parent

yched’s picture

yched CreditAttribution: yched commented

Hm, one nasty consequence is that is becomes difficult to know the vid of a given vocab...
Before this patch, I used to go to admin/structure/taxonomy and hover the links to get the vid from the URL. What other way do we have, other than looking in the db ?

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Devel module adds a tab to all term pages (and node/user/comment) where the full entity can be explored. I think thats sufficient.

Status: Fixed » Closed (fixed)

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

YesCT’s picture

YesCT CreditAttribution: YesCT commented

this is the closest related issue to dealing with the vocab listing page I could find for #1891746: Make vocabulary label in administrative listing link to its list of terms. Is there a better issue more related I should link to?