taxonomy_get_tree() returns inocrect depth on terms with multiple parents and diffrent depth levels.

Schema

The depth is the same, 3, for both terms instead of 2 and 3.

CommentFileSizeAuthor
#27 taxonomy-tree-1330554-27.patch786 bytesoriol_e9g
#24 taxonomy-tree-1330554-24.patch3.12 KBoriol_e9g
#20 taxonomy-tree-1330554-20.patch3.12 KBrichthegeek
#15 taxonomy-tree-1330554-15.patch3.07 KBoriol_e9g
#15 taxonomy-tree-onlytests-1330554-15.patch2.29 KBoriol_e9g
#12 taxonomy-tree-1330554-12.patch3.17 KBoriol_e9g
#11 taxonomy-tree-onlytests-1330554-11.patch2.29 KBoriol_e9g
#11 taxonomy-tree-1330554-11.patch3.73 KBoriol_e9g
#8 taxonomy-vocabulary-tree-tests.patch2.36 KBoriol_e9g
#9 taxonomy-tree-tests-1330554-10.patch2.34 KBoriol_e9g
#3 taxonomy_get_tree_multiple_parents_correct_depth-1330554.patch1.42 KBSilviuChingaru
taxonomy_get_tree.jpg39.53 KBSilviuChingaru
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SilviuChingaru’s picture

SilviuChingaru CreditAttribution: SilviuChingaru commented

I think is something wrong after the clone $term part because if i put the print_r($tree) like this:

        if (count($parents[$vid][$term->tid]) > 1) {
          // We have a term with multi parents here. Clone the term,
          // so that the depth attribute remains correct.
          $term = clone $term;
        }
        
        $term->depth = $depth;
        unset($term->parent);
        $term->parents = $parents[$vid][$term->tid];
        $tree[] = $term;
if($term->tid == 3) print_r($tree);         
        if (!empty($children[$vid][$term->tid])) {
          $has_children = TRUE;

The first output is the right one with depth 2 but the second one when the term with depth 3 is added to the tree the first one gets depth 3 also.

Why is this happening?

I mention that this anomaly makes uc_catalog from ubercart module unusable using taxonomy with multiple parents and different levels. See the issue here:
#1313874: PHP Fatal error: Call to a member function add_child() on a non-object in vocab with multiple parents

SilviuChingaru’s picture

SilviuChingaru CreditAttribution: SilviuChingaru commented
Version: 7.9 » 7.x-dev

I can't unerstand how is overwriting already set $tree var with the last $term?!
The problem is not from clone because I tried also with something like this:

if (count($parents[$vid][$term->tid]) > 1) {
  //$term = clone $term;
  $term_copy = $term;
  $term_copy->depth = $depth;
  unset($term_copy->parent);
  $term_copy->parents = $parents[$vid][$term_copy->tid];
  $tree[] = $term_copy ;
}
else {
  $term->depth = $depth;
  unset($term->parent);
  $term->parents = $parents[$vid][$term->tid];
  $tree[] = $term;
}

and the $tree output still got value from last loop on both terms depth => 3 instead of 2 and 3.

SilviuChingaru’s picture

SilviuChingaru CreditAttribution: SilviuChingaru commented
Version: 7.x-dev » 7.9
Status: Active » Needs review
FileSize
taxonomy_get_tree_multiple_parents_correct_depth-1330554.patch1.42 KB

I fixed this by ading a clone flag and if this flag is set to true all child are cloned so they have individual depth like should.

SilviuChingaru’s picture

SilviuChingaru CreditAttribution: SilviuChingaru commented
Status: Needs review » Patch (to be ported)
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Patch (to be ported) » Needs review
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 7.9 » 7.x-dev
Issue tags: +Needs tests
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

We have the same code in Drupal 8, so first fix this in D8 and add tests to ensure that we never repeat this bug.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 7.x-dev » 8.x-dev
FileSize
taxonomy-vocabulary-tree-tests.patch2.36 KB

Tests for Drupal 8 but doesn't seems that this is failing, NR for testbot.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
taxonomy-tree-tests-1330554-10.patch2.34 KB

Trying the tests in Drupal 7.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 8.x-dev » 7.x-dev
Status: Needs review » Postponed (maintainer needs more info)

I can't reproduce the error, we need a programatically failing test for this.

@fiftyz Any additional steps to reproduce the error?

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
FileSize
taxonomy-tree-1330554-11.patch3.73 KB
taxonomy-tree-onlytests-1330554-11.patch2.29 KB

Yeah! I have reproduced the error with a failing test :) This fails only with some tree structures.

Attached two files, the first only the tests and should fail, the second the tests and the fix, should pass.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
FileSize
taxonomy-tree-1330554-12.patch3.17 KB

Easier!!! Why not simply do this?

SilviuChingaru’s picture

SilviuChingaru CreditAttribution: SilviuChingaru commented
Version: 7.x-dev » 8.x-dev
Status: Postponed (maintainer needs more info) » Needs review

@oriol_e9g You mean clone every term?

I don't know much about drupal test system but i'll try your patch #12 and let you know if it works.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Issue tags: +Needs tests

Yes, it's not correct to only clone the terms with multiple parents because if you have an structure like this:

term1 | depth: 0
-- term2 | depth: 1
---- term3 | depth: 2
term4 | depth: 0
-- term5 | depth: 1
---- term2 | depth: 2
------ term3 | depth: 3

term3 needs to be cloned to correct the depth attribute, but term3 only have one parent (term2). So, if we do this:

if (count($parents[$vid][$term->tid]) > 1) {
  $term = clone $term;
}

term3 is not cloned and it's needed as proven the tests.

In fact we do not need to clone all terms. We need to clone the terms with multiple parents (as term2->term1,term5) and the terms with a single parent that have a parent with a multiple parents (term3->term2->term1,term5), etc... So, I think that the easier and faster way is clone all terms with parents (single or multiple) to correct the depth attributes, instead of recursively see all parents in all depth levels for searching a multiple parent.

if (isset($parents[$vid][$term->tid])) {
  $term = clone $term;
}
oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Issue tags: -Needs tests
FileSize
taxonomy-tree-onlytests-1330554-15.patch2.29 KB
taxonomy-tree-1330554-15.patch3.07 KB

Only tests patch should fail to prove the bug, complete patch should pass.

Status: Needs review » Needs work

The last submitted patch, taxonomy-tree-onlytests-1330554-15.patch, failed testing.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs work » Needs review
omitsis’s picture

omitsis CreditAttribution: omitsis commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

We've tested the patch and seems to work correctly.
Thankyou!

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1058,9 +1058,8 @@ function taxonomy_get_tree($vid, $parent = 0, $max_depth = NULL, $load_entities
+          // Clone the term to remains correct the depth attribute.

This comment is a bit wonky. And I can't comment on the code. Don't know the internals.

-16 days to next Drupal core point release.

richthegeek’s picture

richthegeek CreditAttribution: richthegeek commented
FileSize
taxonomy-tree-1330554-20.patch3.12 KB

Fixed a few spelling and phrasing issues. I don't understand the approach enough to comment on it's correctness.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs work » Needs review

Changing the status to attract some eyes to the approach.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, taxonomy-tree-1330554-20.patch, failed testing.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
FileSize
taxonomy-tree-1330554-24.patch3.12 KB
omitsis’s picture

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

Works great and well documented, thx!

Dries’s picture

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

Good catch, and nice fix. Thanks for the tests too. Committed to 7.x and 8.x. Thanks!

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Version: 8.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
taxonomy-tree-1330554-27.patch786 bytes

Same fix for Drupal 6.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Needs review » Reviewed & tested by the community

Technically I can't put the RTBC but it's the same fix as D7 & D8, testbot it's happy, harmless change, so...

jtwalters’s picture

jtwalters CreditAttribution: jtwalters commented

The committed patch results in the taxonomy term object no longer having $term->parents returned from taxonomy_get_tree.

Can you move the line $term->parents = $parents[$vid][$term->tid]; before $term = clone $term; without changing the desired behavior? I tried it and the taxonomy tests still pass.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Reviewed & tested by the community » Needs work

You cannot use clone() in Drupal 6, as it is a PHP 5 construct and D6 still supports PHP 4. Use drupal_clone() instead. See http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_clo...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.