Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
taxonomy_get_tree() returns inocrect depth on terms with multiple parents and diffrent depth levels.
The depth is the same, 3, for both terms instead of 2 and 3.
Comment | File | Size | Author |
---|---|---|---|
#27 | taxonomy-tree-1330554-27.patch | 786 bytes | oriol_e9g |
#24 | taxonomy-tree-1330554-24.patch | 3.12 KB | oriol_e9g |
#20 | taxonomy-tree-1330554-20.patch | 3.12 KB | richthegeek |
#15 | taxonomy-tree-1330554-15.patch | 3.07 KB | oriol_e9g |
#15 | taxonomy-tree-onlytests-1330554-15.patch | 2.29 KB | oriol_e9g |
Comments
Comment #1
SilviuChingaru CreditAttribution: SilviuChingaru commentedI think is something wrong after the clone $term part because if i put the print_r($tree) like this:
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
Comment #2
SilviuChingaru CreditAttribution: SilviuChingaru commentedI 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:
and the $tree output still got value from last loop on both terms depth => 3 instead of 2 and 3.
Comment #3
SilviuChingaru CreditAttribution: SilviuChingaru commentedI 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.
Comment #4
SilviuChingaru CreditAttribution: SilviuChingaru commentedComment #5
oriol_e9gComment #6
oriol_e9gComment #7
oriol_e9gWe have the same code in Drupal 8, so first fix this in D8 and add tests to ensure that we never repeat this bug.
Comment #8
oriol_e9gTests for Drupal 8 but doesn't seems that this is failing, NR for testbot.
Comment #9
oriol_e9gTrying the tests in Drupal 7.
Comment #10
oriol_e9gI can't reproduce the error, we need a programatically failing test for this.
@fiftyz Any additional steps to reproduce the error?
Comment #11
oriol_e9gYeah! 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.
Comment #12
oriol_e9gEasier!!! Why not simply do this?
Comment #13
SilviuChingaru CreditAttribution: SilviuChingaru commented@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.
Comment #14
oriol_e9gYes, it's not correct to only clone the terms with multiple parents because if you have an structure like this:
term3 needs to be cloned to correct the depth attribute, but term3 only have one parent (term2). So, if we do this:
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.
Comment #15
oriol_e9gOnly tests patch should fail to prove the bug, complete patch should pass.
Comment #17
oriol_e9gComment #18
omitsis CreditAttribution: omitsis commentedWe've tested the patch and seems to work correctly.
Thankyou!
Comment #19
aspilicious CreditAttribution: aspilicious commentedThis 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.
Comment #20
richthegeek CreditAttribution: richthegeek commentedFixed a few spelling and phrasing issues. I don't understand the approach enough to comment on it's correctness.
Comment #21
oriol_e9gChanging the status to attract some eyes to the approach.
Comment #22
oriol_e9gComment #24
oriol_e9gComment #25
omitsis CreditAttribution: omitsis commentedWorks great and well documented, thx!
Comment #26
Dries CreditAttribution: Dries commentedGood catch, and nice fix. Thanks for the tests too. Committed to 7.x and 8.x. Thanks!
Comment #27
oriol_e9gSame fix for Drupal 6.
Comment #28
oriol_e9gTechnically I can't put the RTBC but it's the same fix as D7 & D8, testbot it's happy, harmless change, so...
Comment #29
jtwalters CreditAttribution: jtwalters commentedThe committed patch results in the taxonomy term object no longer having
$term->parents
returned fromtaxonomy_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.Comment #30
longwaveYou 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...