taxonomy_get_tree() returns all the terms in an array, but you have no way to find the data for a given term inside that array without iterating through the whole thing. This is silly (I'm tempted to call this a bug). It's been bothering me for a while, and I've had to work-around it by wastefully iterating through the tree once to make a new array indexed by tid, which I can then actually use. We had a very similar problem with node_revision_list() over at #114703: minor node revisions fixes.
Here's a patch for D7's taxonomy.module. I can't see how there'd be objections to this.
However, meanwhile, I'd like to see if there's any chance this will be committed to D6 and/or D5. This patch applies (with minor offsets) to D6, and would be trivial to backport to D5 if that'd fly. Given the outcome of #114703, I doubt this will go into D5, but maybe D6? ;)
Comments
Comment #1
wim leersThis has bothered me too for a long time. This, among other things, is necessary to clean up the Taxonomy module.
Comment #2
catchThis is a great idea, but it breaks drag and drop in taxonomy administration.
Comment #3
eojthebraveWas playing patch bingo tonight and came across this one and agree that it would be a good improvement.
This should fix the problem.
Steps to test.
1. Navigate to the "list terms" page for a Vocabulary that has terms.
2. Reorder the terms.
3. Save
Comment #4
catchPatch was reversed, here's a re-roll which should apply cleanly.
We need at least a basic test for taxonomy_get_tree really. I don't think the terms overview page is tested either.
Comment #5
cburschka1.) Applies
2.) Patched site installs.
3.) Created basic tag hierarchy
4.) Shuffled around, works.
5.) Saved with the new order, works.
I encountered a nasty PDO error while creating one of the terms, but that's probably unrelated and I'll file a separate issue for it.
Specifically, when creating a term and selecting the
<root>item as a parent (which should be the same as selecting nothing at all):Comment #6
wim leers[:db_insert_placeholder_1] should equal 0. So that's probably an error in the #submit callback.
Comment #7
catchdamn, the term objects page also converted a bunch of queries to dbtng, and that doesn't do things like silently case empty strings to 0/NULL for you or whatever this code was relying on. Patch and test posted at #332145: UNSTABLE-3 blocker: taxonomy_form_term_submit passes empty string as parent [dbtng conversion regression]
Comment #9
lilou commentedSee: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
Comment #11
eojthebravere-rolled against current head. Passes current set of taxonomy tests.
Comment #12
eojthebraveMarking as needs review
Comment #13
catchNote that taxonomy_get_tree gets a bit more coverage than it does already in #144969: taxonomy_term_count_nodes returns wrong count (+ tests). I'll see about adding an explicit test to this issue later on though if no-one beats me to it.
Comment #15
eojthebravebugger. Still learning how to use git, looks like my patch didn't actually apply. Should work now.
Comment #16
catchWe can't do this. taxonomy_get_tree() returns the entire taxonomy tree - in which a term might appear more than once when multiple parents are enabled, with different depth for example.
However, in a lot of cases you don't need all that information, so we might consider something like taxonomy_get_descendants() which indexed by tid, or indexing by tid for vocabularies without multiple parents inside taxonomy_get_tree() itself.
Comment #17
xanoAlso, indexing terms means we lose the order in which we fetch them from the DB.
Comment #18
dww@catch #16: "We can't do this. taxonomy_get_tree() returns the entire taxonomy tree - in which a term might appear more than once when multiple parents are enabled, with different depth for example."
Argh. :( What a pain.
@xano #17: "indexing terms means we lose the order in which we fetch them from the DB"
Ugh.
We lose that ORDER BY. I suppose there are cases where you need that.
Guess this is a feature request for a brand new API function that gives you all terms in a vocab indexed by tid or something.
Comment #19
catchYou mean like taxonomy_term_load_multiple(array(), array('vid' => $vid)) ?
Having said that though, we could definitely add an API function like taxonomy_get_descendents() returning an array of all children indexed by tid - since we just want the terms in that case, not a representation of their depth in the hierarchy. So leaving this open.
Comment #20
xanoMoments after my last reply I realised that although the query used by taxonomy_get_tree() sorts terms by weight and name, this order gets lost later on in the function. We might want to figure out a way to make sure this order remains, so contrib can use Taxonomy's function to display terms in the same order at all times.
Comment #21
xanoWe need this for #295395: DX: convert Taxonomy selectors to FAPI widgets.
Comment #22
catchBetween taxonomy_get_tree() and taxonomy_load_multiple() we have most of this, I don't think this'll get done in the next day, so moving to D8
Comment #23
xjmCrossposting #1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience.
Comment #24
jody lynnhttp://api.drupal.org/api/drupal/core%21modules%21taxonomy%21taxonomy.mo...
Looks like this has already been added.
Comment #25
panchoRe #24:
Nope, taxonomy_term_load_children() has existed before and is not the same as taxonomy_term_load_descendents().
Therefore reopening.
Also, adapting title after the #1377628-9: taxonomy_get_term_by_name() should be taxonomy_term_load_multiple_by_name() rename.
Comment #26
panchoPostponed on #1976298: Move taxonomy_get_tree() and associated functions to Taxonomy storage, deprecate procedural wrappers..
Comment #27
mgiffordComment #41
smustgrave commentedThank you for sharing your idea for improving Drupal.
We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #42
smustgrave commentedWanted to bump 1 more time before closing.
Comment #43
dwwYeah, I need to look again at what's possible in modern Drupal. Probably this can be closed as outdated, but I'll try to make time to look closely and decide.
Comment #45
smustgrave commented@dww just wanted to see if you had a chance to take a look?