| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | taxonomy.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
Issue Summary
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? ;)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| taxonomy_get_tree_indexed_by_tid.d7.patch | 1.56 KB | Idle | Failed: 7073 passes, 108 fails, 6640 exceptions | View details | Re-test |
Comments
#1
This has bothered me too for a long time. This, among other things, is necessary to clean up the Taxonomy module.
#2
This is a great idea, but it breaks drag and drop in taxonomy administration.
* notice: Undefined offset: 0 in /home/catch/www/7/modules/taxonomy/taxonomy.admin.inc on line 463.* notice: Undefined index: tid in /home/catch/www/7/modules/taxonomy/taxonomy.admin.inc on line 464.
* notice: Undefined index: parents in /home/catch/www/7/modules/taxonomy/taxonomy.admin.inc on line 465.
* notice: Undefined index: weight in /home/catch/www/7/modules/taxonomy/taxonomy.admin.inc on line 465.
* notice: Undefined index: parents in /home/catch/www/7/modules/taxonomy/taxonomy.admin.inc on line 471.
#3
Was 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
#4
Patch 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.
#5
1.) 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):PDOException: INSERT INTO {term_hierarchy} (tid, parent) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1) - Array ( [:db_insert_placeholder_0] => 4 [:db_insert_placeholder_1] => ) SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'parent' at row 1 in taxonomy_term_save() (line 359 of modules/taxonomy/taxonomy.module).#6
[:db_insert_placeholder_1] should equal 0. So that's probably an error in the #submit callback.
#7
damn, 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]
#8
The last submitted patch failed testing.
#9
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#10
The last submitted patch failed testing.
#11
re-rolled against current head. Passes current set of taxonomy tests.
#12
Marking as needs review
#13
Note 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.
#14
The last submitted patch failed testing.
#15
bugger. Still learning how to use git, looks like my patch didn't actually apply. Should work now.
#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.
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.
#17
Also, indexing terms means we lose the order in which we fetch them from the DB.
#18
@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.
SELECT t.tid, t.*, parent FROM {taxonomy_term_data} t INNER JOIN {taxonomy_term_hierarchy} h ON t.tid = h.tid WHERE t.vid = %d ORDER BY weight, nameWe 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.
#19
You 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.
#20
Moments 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.
#21
We need this for #295395: DX: convert Taxonomy selectors to FAPI widgets.
#22
Between 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
#23
Crossposting #1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience.
#24
http://api.drupal.org/api/drupal/core%21modules%21taxonomy%21taxonomy.mo...
Looks like this has already been added.