Status:
A partial fix has been committed to D7 in #346156-14: term delete fails because of misnamed function.
However, per #13 and a duplicate issue against D7 (see #19) this still doesn't work correctly.
A new patch against D7 is available in #16
Related Issues:
#1347542: [META] Taxonomy API improvements
#251595: Add taxonomy_term_load_descendents()
#346156: term delete fails because of misnamed function
#394422: Taxonomy terms should be listed in order they are entered
#1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience
Original report by Pancho
Suppose you have a multiple hierarchy vocabulary with the following terms:
Politics (tid=1)
-- Event (tid=2)
Sports (tid=3)
-- Tennis (tid=4)
---- Event (tid=2)
Calling taxonomy_get_parents_all(2), you would expect an array with all ancestors of the Event term.
Instead, the array contains (in this order) only the terms Event, Politics and Tennis, while Sports is missing.
If you add a positive weight to Politics, you receive the following vocabulary:
Sports (tid=3)
-- Tennis (tid=4)
---- Event (tid=2)
Politics (tid=1)
-- Event (tid=2)
Calling taxonomy_get_parents_all(2) now gives you the expected array with all ancestors of the Event term: Event, Tennis, Politics and Sports.
So the search for ancestors starts with the branch that has the least weight and then bounces between the branches, searching for another generation of ancestors. But as soon as any ancestor has no more parents, the complete search is stopped.
Instead it would be correct to keep on analyzing the other ancestors if they have more parents.
In the first example: Politics has no more parents, so the algorithm still spits out Tennis, but stops then. Correct would be to abandon the Politics branch, but keep on crawling the Tennis branch. Then we would have found Sports as well.
I also think it would make a lot of sense to add parents to the output array. Maybe we also want some control over how many parent generations are displayed. But these are nice features for D7, not the D6 bug itself.
Comment | File | Size | Author |
---|---|---|---|
#52 | drupal-taxonomy_get_parents_all-multiple_parents-217676-47_0.patch | 1.92 KB | mgifford |
#47 | drupal-taxonomy_get_parents_all-multiple_parents-217676-47.patch | 1.92 KB | ckng |
#38 | interdiff.txt | 1.96 KB | andypost |
#37 | 217676-37.patch | 2.99 KB | andypost |
Comments
Comment #1
cburschkaVery straightforward. Patch is attached below.
Example vocabulary was created according to the original post. This command was executed pre- and post-patch:
Pre-patch:
Post-patch:
As you said in the other issue, taxonomy breadcrumbs need to work around the multiple hierarchies. This patch doesn't fix that, it only deals with the bug above.
Comment #2
Wim LeersSubscribing. This would need to be backported to Drupal 5 as well.
Comment #3
toemaz CreditAttribution: toemaz commentedSubscribing. This bug appeared to me when using the hierarchical_select module.
Comment #4
PanchoI'd call this a critical bug.
It needs to be fixed in D7 first, then backported to D6 and D5.
Still applies to HEAD. Please test.
Comment #5
pp CreditAttribution: pp commentedI have a new copy of 7.x-dev. I used the patch. It worked good. Example vocabulary was created according to the original post. I saw what Arancaytar saw.
SimpleTest taxonomy modul:
62 passes, 0 fails, 0 exceptions
pp
Comment #6
catchThis still applies and looks good, but IMO we need a test for it.
Comment #7
catchRolled in an E_ALL fix from #351669: taxonomy_get_parents_all() is not E_ALL compliant which also needs backporting to Drupal 6. Also added a test which covers both bugs..
Comment #9
catchHere it is without an infinite loop, but in this case the (very basic) test fails.
I'm more interested in getting #344019: New implementation for database tree parsing ( taxonomy / comment ) in than trying to fix these recursive queries.
Comment #10
catchComment #11
catchSplit the tests off to #352479: Tests for taxonomy_get_parents_all()
Also demoting from critical.
Comment #12
Wim Leers.
Comment #13
Summit CreditAttribution: Summit commentedHi,
This patch is not working correct. I need it to get breadcrumbs correct for weblinks on D6: http://drupal.org/node/735874#comment-2690350
I tried the latest patch and got WSOD.
I added the D7 functions taxonomy_term_load and taxonomy_term_load_multiple, but still WSOD.
Edit: Will implement this module: http://drupal.org/project/taxidermy help?
Thanks a lot in advance for working on this!
greetings,
Martijn
Comment #14
Stevel CreditAttribution: Stevel commentedThis part got committed in #346156: term delete fails because of misnamed function, so this function might actually be working now. Lets try out the tests from #352479: Tests for taxonomy_get_parents_all().
Comment #15
Stevel CreditAttribution: Stevel commentedRolled a new patch with a different way of searching. Also included the test as it is the only way of telling whether the function actually works correct.
Comment #16
Stevel CreditAttribution: Stevel commentedLast patch wasn't correct, changed some details.
Comment #17
PanchoStill applies for D8. The function is just called taxonomy_term_load_parents_all() now.
Comment #18
PanchoAlso closed #873414: taxonomy_get_parents_all() doesn't work correctly with multiple hierarchy terms as an exact duplicate. Note that this issue contains a patch against D6.
Comment #19
PanchoClosed #1982032: taxonomy_get_parents_all doesn't work as (I) expected as yet another duplicate of this. It contains a patch against D7 that basically resembles these one here.
Comment #19.0
PanchoUpdate current status
Comment #20
PanchoPorted the patch in #16 to D8.
Also added Unit Tests for both taxonomy_term_load_parents() and taxonomy_term_load_parents_all(), which currently aren't tested at all.
Test results are as expected: taxonomy_term_load_parents() passes the tests, while taxonomy_term_load_parents_all() only does so with the patch applied.
From test duration, performance should remain the same.
Comment #22
PanchoTests-only patch correctly failed, so this is ready for review.
Comment #22.0
Panchomention D7 patch, add link to meta issue
Comment #23
mr.baileysI confirmed that the bug is still present in 8.x HEAD, and that this patch solves the issue.
The code looks sound to me, just two small remarks:
Comment #24
mr.baileysComment #26
mr.baileys24: 217676-24-taxonomy-load-all-ancestors.patch queued for re-testing.
Comment #27
mr.baileysThe same test-only patch that fails in #20 is now passing in #24. Since all terms share the same weight, the order in which parents for a given term are returned is defined by the term names (see
TermStorageController::loadParents()
). Since weight of terms plays a role in whether or the result fromtaxonomy_term_load_parents_all()
is correct, this causes the tests to fail in only half of the test runs.Attached is a new test-only patch, which now forces weight on $term[1] to trigger the bug, and a new complete patch (fix + tests)
Comment #29
mr.baileys27: 217676-27-taxonomy-load-all-ancestors.patch queued for re-testing.
Comment #34
mgiffordSeems like this function taxonomy_term_load_parents_all() has been rewritten
Pointing everything to core/modules/taxonomy/src/TermStorage.php and
public function loadAllParents($tid) {
This will need to be re-rolled with someone understanding TermStorage.php.
Comment #35
andypostproper re-roll, still does not get how to work with "cycled" terms
Comment #36
jibranWe can update this.
Comment #37
andypostFixed
Comment #38
andypostinterdiff
Comment #39
jibranThanks
Comment #42
larowlan+1 thanks
Comment #43
jibranTest bot fluke.
Comment #44
alexpottCommitted 99876e6 and pushed to 8.0.x. Thanks!
Comment #46
andypostComment #47
ckngThis is a reroll of #26 for D7.
Comment #51
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer and commentedAny update when this is going to be committed to 7.x?
Comment #52
mgiffordRe-posting patch from 2 years ago to trigger the bot.