Problem/Motivation

  • The Taxonomy module uses the numbers 0, 1, and 2 to denote flat, single, and multiple taxonomy vocabulary hierarchy, respectively. See taxonomy_check_vocabulary_hierarchy() and taxonomy_schema().
  • It is not clear from the code what these numbers mean or why they are used.

Proposed resolution

  • Define constants for the three types of hierarchy.
    • TAXONOMY_HIERARCHY_DISABLED = 0
    • TAXONOMY_HIERARCHY_SINGLE = 1
    • TAXONOMY_HIERARCHY_MULTIPLE =2
  • Convert the integers to these values where appropriate. I grepped and found the following instances for 2, which should help:
    taxonomy.admin.inc:371:    if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
taxonomy.admin.inc:405:  if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
taxonomy.install:151:        'description' => 'The type of hierarchy allowed within the vocabulary. (0 = disabled, 1 = single, 2 = multiple)',
taxonomy.module:530: * will be given a hierarchy of 2.
taxonomy.module:551:      $hierarchy = 2;
taxonomy.admin.inc:705:    '#collapsed' => $vocabulary->hierarchy < 2,
taxonomy.admin.inc:843:  elseif ($current_parent_count > $previous_parent_count && $form['#vocabulary']->hierarchy < 2) {
taxonomy.admin.inc:844:    $form['#vocabulary']->hierarchy = $current_parent_count == 1 ? 1 : 2;
taxonomy.module:530: * will be given a hierarchy of 2.
taxonomy.module:551:      $hierarchy = 2;

    Also possibly in taxonomy.test, so check for a hierarchy test there as well.

Remaining tasks

  • TBD

User interface changes

  • None.

API changes

  • Constants added:
    • TAXONOMY_HIERARCHY_DISABLED
    • TAXONOMY_HIERARCHY_SINGLE
    • TAXONOMY_HIERARCHY_MULTIPLE
CommentFileSizeAuthor
#14 1369200-d7-taxonomy-hierarchy-constants.patch7.76 KBchris.leversuch
#11 1369200-taxonomy-hierarchy-constants-5.patch7.8 KBchris.leversuch
#9 1369200-taxonomy-hierarchy-constants-4.patch8.57 KBchris.leversuch
#7 1369200-taxonomy-hierarchy-constants-3.patch8.04 KBchris.leversuch
#5 1369200-taxonomy-hierarchy-constants-2.patch7.79 KBacouch
#5 interdiff.txt2.59 KBacouch
#3 1369200-taxonomy-hierarchy-constants.patch7.1 KBchris.leversuch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Novice, +Needs backport to D7

Tagging.

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
Assigned: Unassigned » chris.leversuch

I'll have a go at this one.

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
Status: Active » Needs review
FileSize
1369200-taxonomy-hierarchy-constants.patch7.1 KB

First attempt

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented

Forgot to include grep results for review. I don't think the remaining occurances need changing

chris@pc051:~/htdocs/drupal8/core/modules/taxonomy$ grep -in "hierarchy.*0" *
taxonomy.admin.inc:471:    $hierarchy = $term['parents'][0] != 0 ? TAXONOMY_HIERARCHY_SINGLE : $hierarchy;
taxonomy.admin.inc:498:      $hierarchy = $term['parent'] != 0 ? TAXONOMY_HIERARCHY_SINGLE : $hierarchy;
taxonomy.admin.inc:511:    $hierarchy = $term['parents'][0] != 0 ? TAXONOMY_HIERARCHY_SINGLE : $hierarchy;
taxonomy.install:151:        'description' => 'The type of hierarchy allowed within the vocabulary. (0 = disabled, 1 = single, 2 = multiple)',
taxonomy.module:19:const TAXONOMY_HIERARCHY_DISABLED = 0;
taxonomy.module:474:    $result = db_query('SELECT t.tid FROM {taxonomy_term_data} t INNER JOIN {taxonomy_term_hierarchy} th ON th.tid = t.tid WHERE t.vid = :vid AND th.parent = 0', array(':vid' => $vid))->fetchCol();
chris@pc051:~/htdocs/drupal8/core/modules/taxonomy$ grep -in "hierarchy.*1" *
taxonomy.admin.inc:371:    if ($vocabulary->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE && count($tree) > 1) {
taxonomy.admin.inc:405:  if ($vocabulary->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE && count($tree) > 1) {
taxonomy.admin.inc:844:    $form['#vocabulary']->hierarchy = $current_parent_count == 1 ? TAXONOMY_HIERARCHY_SINGLE : TAXONOMY_HIERARCHY_MULTIPLE;
taxonomy.install:151:        'description' => 'The type of hierarchy allowed within the vocabulary. (0 = disabled, 1 = single, 2 = multiple)',
taxonomy.module:24:const TAXONOMY_HIERARCHY_SINGLE = 1;
taxonomy.test:240:    // Set up hierarchy. term 2 is a child of 1 and 4 a child of 1 and 2.
chris@pc051:~/htdocs/drupal8/core/modules/taxonomy$ grep -in "hierarchy.*2" *
taxonomy.install:151:        'description' => 'The type of hierarchy allowed within the vocabulary. (0 = disabled, 1 = single, 2 = multiple)',
taxonomy.module:29:const TAXONOMY_HIERARCHY_MULTIPLE  = 2;
taxonomy.test:240:    // Set up hierarchy. term 2 is a child of 1 and 4 a child of 1 and 2.
chris@pc051:~/htdocs/drupal8/core/modules/taxonomy$
acouch’s picture

acouch CreditAttribution: acouch commented
FileSize
interdiff.txt2.59 KB
1369200-taxonomy-hierarchy-constants-2.patch7.79 KB

Awesome start @chris.leversuch. I noticed a couple of small things. There was an extra space before the the multiple constant, I added the constants for the cases in the help section, and have a question about the following:

$result = db_query('SELECT t.tid FROM {taxonomy_term_data} t INNER JOIN {taxonomy_term_hierarchy} th ON th.tid = t.tid WHERE t.vid = :vid AND th.parent = :thparent', array(':vid' => $vid, ':thparent'     => TAXONOMY_HIERARCHY_DISABLED))->fetchCol();

Shouldn't that be left out since it is referring to the value of the parent?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

I think @acouch is right about #5; that query is checking whether the term has any parents rather than the hierarchy of the vocab. I also noticed one other thing:

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -368,7 +368,7 @@ function taxonomy_overview_terms($form, &$form_state, $vocabulary) {
-    if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
+    if ($vocabulary->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE && count($tree) > 1) {

@@ -402,7 +402,7 @@ function taxonomy_overview_terms($form, &$form_state, $vocabulary) {
-  if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
+  if ($vocabulary->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE && count($tree) > 1) {

@@ -702,7 +702,7 @@ function taxonomy_form_term($form, &$form_state, $edit = array(), $vocabulary =
-    '#collapsed' => $vocabulary->hierarchy < 2,
+    '#collapsed' => $vocabulary->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE,

@@ -840,8 +840,8 @@ function taxonomy_form_term_submit($form, &$form_state) {
-  elseif ($current_parent_count > $previous_parent_count && $form['#vocabulary']->hierarchy < 2) {
-    $form['#vocabulary']->hierarchy = $current_parent_count == 1 ? 1 : 2;
+  elseif ($current_parent_count > $previous_parent_count && $form['#vocabulary']->hierarchy < TAXONOMY_HIERARCHY_MULTIPLE) {
+    $form['#vocabulary']->hierarchy = $current_parent_count == 1 ? TAXONOMY_HIERARCHY_SINGLE : TAXONOMY_HIERARCHY_MULTIPLE;

For these, should we avoid relying on the literal value of the disabled and multiple constants being less than the multiple? I think it might be better to check for:

($foo == TAXONOMY_HIERARCHY_DISABLED) || ($foo == TAXONOMY_HIERARCHY_SINGLE)
chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
FileSize
1369200-taxonomy-hierarchy-constants-3.patch8.04 KB

Here's a new patch based on #5 with the changes from #6

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Needs work 
+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -368,7 +368,7 @@ function taxonomy_overview_terms($form, &$form_state, $vocabulary) {
-    if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
+    if (($vocabulary->hierarchy == TAXONOMY_HIERARCHY_DISABLED) || ($vocabulary->hierarchy == TAXONOMY_HIERARCHY_SINGLE) && count($tree) > 1) {

@@ -402,7 +402,7 @@ function taxonomy_overview_terms($form, &$form_state, $vocabulary) {
-  if ($vocabulary->hierarchy < 2 && count($tree) > 1) {
+  if (($vocabulary->hierarchy == TAXONOMY_HIERARCHY_DISABLED) || ($vocabulary->hierarchy == TAXONOMY_HIERARCHY_SINGLE) && count($tree) > 1) {

Hm, for these two, let's make the order of operations more explicit:
if (($vocabulary->hierarchy == TAXONOMY_HIERARCHY_DISABLED || $vocabulary->hierarchy == TAXONOMY_HIERARCHY_SINGLE) && count($tree) > 1)

Edit: Actually, how about this (not sure why I didn't think of this earlier):
if (($vocabulary->hierarchy != TAXONOMY_HIERARCHY_MULTIPLE) && (count($tree) > 1))

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
Status: Needs work » Needs review
FileSize
1369200-taxonomy-hierarchy-constants-4.patch8.57 KB

Changed all 4 occurences from #6 to match the new logic in #8

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Thanks @chris.leversuch. Codewise, the patch looks correct to me now. Two minor questions:

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -449,7 +449,7 @@ function taxonomy_overview_terms_submit($form, &$form_state) {
+  $hierarchy = TAXONOMY_HIERARCHY_DISABLED; // Update the current hierarchy type as we go.

Sorry that I did not notice this before, but we need to move this comment up to the preceding line since it's now over 80 chars. I'd just reroll/RTBC with that change myself, except...

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -543,11 +559,11 @@ function taxonomy_check_vocabulary_hierarchy($vocabulary, $changed_term) {
@@ -1210,6 +1226,8 @@ function taxonomy_vocabulary_load_multiple($vids = array(), $conditions = array(

@@ -1210,6 +1226,8 @@ function taxonomy_vocabulary_load_multiple($vids = array(), $conditions = array(
  * @return
  *   The vocabulary object with all of its metadata, if exists, FALSE otherwise.
  *   Results are statically cached.
+ *
+ * @see taxonomy_vocabulary_machine_name_load()
  */
 function taxonomy_vocabulary_load($vid) {
   $vocabularies = taxonomy_vocabulary_load_multiple(array($vid));
@@ -1225,6 +1243,8 @@ function taxonomy_vocabulary_load($vid) {

@@ -1225,6 +1243,8 @@ function taxonomy_vocabulary_load($vid) {
  * @return
  *   The vocabulary object with all of its metadata, if exists, FALSE otherwise.
  *   Results are statically cached.
+ *
+ * @see taxonomy_vocabulary_load()
  */
 function taxonomy_vocabulary_machine_name_load($name) {
   $vocabularies = taxonomy_vocabulary_load_multiple(NULL, array('machine_name' => $name));

These two additions are new to the latest patch in #9. Is that intentional? They don't seem to be related to the issue.

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
FileSize
1369200-taxonomy-hierarchy-constants-5.patch7.8 KB

Ha, oops. They're from #1370060: Add cross-references between taxonomy_vocabulary_load() and taxonomy_vocabulary_machine_name_load(). I obviously didn't have a clean working dir when I created the patch.
Try this one.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

Alright, this looks good now. Thanks @chris.leversuch!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks good. Committed/pushed to 8.x.

chris.leversuch’s picture

chris.leversuch CreditAttribution: chris.leversuch commented
Status: Patch (to be ported) » Needs review
FileSize
1369200-d7-taxonomy-hierarchy-constants.patch7.76 KB

Here's a D7 version

acouch’s picture

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

I reviewed this and there are no instances of hierarchy that I could find that were missed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Define constants for taxonomy hierarchy » Change notice for Define constants for taxonomy hierarchy
Version: 7.x-dev » 8.x-dev
Priority: Minor » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Needs backport to D7 +Needs change record

This seems like a good improvement, but not sure it really makes sense for Drupal 7. It's not fixing a bug or adding a new capability, merely cleaning things up. If you feel strongly the opposite, we can discuss, though.

Moving back to 8.x and we should get a change notice for this.

izus’s picture

izus CreditAttribution: izus commented
Priority: Critical » Normal
Status: Active » Fixed

Added change notice http://drupal.org/node/1385926

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Change notice for Define constants for taxonomy hierarchy » Define constants for taxonomy hierarchy
Issue tags: -Novice, -Needs change record
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface

Follow-up for this issue: #2807263: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface.