Currently, the available API for selecting taxonomy hierarchies is very inconsistent. For example:
-
taxonomy_get_tree()
makes the loading of term entities optional, but seemingly related functions --taxononomy_get_children()
,taxonomy_get_parents()
,taxonomy_get_parents_all()
, andtaxonomy_get_parents_all()
-- all force the entities to be loaded. -
taxonomy_get_tree()
has data about the direct children of each term, but does not return it. So, callers have to choose between rearranging the array to find the children of each term based on what claims it as a parent, or usingtaxonomy_get_children()
, which only allows the fetching of one term at a time and forces the loading of term entities. -
taxonomy_get_parents_all()
is provided, but notaxonomy_get_children_all()
(ortaxonomy_get_descendents()
as in #251595: Add taxonomy_term_load_descendents()) -
taxonomy_get_parents_all()
in particular runs a lot of queries in addition to loading all the parent entities. It could probably be refactored to improve performance. -
It would be better to have
taxonomy_get_parents_multiple()
andtaxonomy_get_children_multiple()
to reduce the number of queries needed, and for the existing functions to be single-value wrappers of these. (One is a special case of many.) -
If #5 above were implemented, it might be best to rename
taxonomy_get_parents_all()
totaxonomy_get_ancestors()
for clarity.
A refactor might potentially leverage some of the flexibility in the new database API, perhaps with a single base taxonomy hierarchy query that is modified to fit the particular case. (It might also be very useful to allow callers add contextual query tags to the queries.)
API functions:
Possibly relevant issues (some are old and parts may no longer be applicable since #556842: taxonomy_get_tree() memory issues went into 7.x):
- #344019: New implementation for database tree parsing ( taxonomy / comment )
- #106015: [performance] DB caching for \Drupal\taxonomy\TermStorageController::loadTree()
- #698918: Reduce memory usage from \Drupal\taxonomy\TermStorageInterface::loadTree() when parent is specified
- #499144: taxonomy_get_tree() generates data almost, but not quite, useful in taxonomy_save_term()
- #251595: Add taxonomy_term_load_descendents()
Comments
Comment #1
giorgio79 CreditAttribution: giorgio79 commentedAs I understand the entire core taxonomy plus node and user references will be scrapped since http://drupal.org/project/relation is coming. Not sure how much effort will go into the obsolete taxonomy stuff.
Comment #2
xjmI think it's a bit hasty to call the entirety of taxonomy "obsolete" and not worth working on based on one initiative. You make it sound as if taxonomy would be eliminated from core. Even if the relational model changes, a refactor is worthwhile. All the more worthwhile, actually, because a cleanly factored API is much easier to port to a new model than a poorly factored one.
Comment #3
catchRelation ought to be able to replace (or at least provide the basis of) the term reference field in core, but it isn't going to provide the taxonomy term entity type, and currently doesn't have support for hierarchical relationships at all yet (unless I missed something).
This issue is a good one, things I'd personally like to see us do:
- Nice external facing API for requesting hierarchy.
- Make the implementation in taxonomy module (and possibly elsewhere) pluggable.
- Try to get something better than what we have (shouldn't be hard) into core as the default implementation.
Comment #4
xjmPossible first steps:
taxonomy_get_tree()
, or more likely sometaxonomy_magic_hierarchy_bunnies()
factored out of it, becomes the base for all these functions.(Note to the extremely literal-minded: that is not an actual proposal for a function name.)
Address #698918: Reduce memory usage from \Drupal\taxonomy\TermStorageInterface::loadTree() when parent is specified.
Add
$term->children
analogous to$term->parents
for convenience. If the memory use of this duplication is a concern, we can make it optional with a developer-friendly set of flags or additional wrappers.Add the possibility to additionally add
$term->ancestors
and$term->descendants
, also for convenience. These would not be included by default, because of the memory footprint. I am not immediately sure whether it would be better to pass a flag totaxonomy_magic_hierarchy_bunnies()
and add the descendants or ancestors in real time in the parent processing loop as it exists now, or to provide separate API to loop over the results again and assemble the ancestors/descendants based on the parents/children. (Benchmarking could probably tell us which method is better, if nothing else.)taxonomy_get_parents()
and brethren are then special-case wrappers (the parent is the term tid and the max depth is the term's depth), with entity loading optional.Handle multiple cases in the base function for reduced queries.
Maybe add additional API to return the tree as a nested array (with child term objects as actual children of the parent term object).
And we improve progressively from there!
Comment #5
xjmOther notes from IRC:
Comment #6
catchWe don't currently load $term->parents into the term object on entity_load(), that means there is a nasty hack in taxonomy_term_save() to stop them getting wiped away if you try to save a term you just loaded (in D6 there is no such check and it just kills them iirc). Would be good to fix that as well.
Not sure about adding term->children automatically - although I think the menu system may use a nested structure for its trees so if we were actually nesting maybe that would work?
Comment #7
giorgio79 CreditAttribution: giorgio79 commented#3 "currently doesn't have support for hierarchical relationships at all yet (unless I missed something)"
Taken from http://drupal.org/project/relation page
"And for 2-ary relations, relations can be directional, ie:
bruno -> isChildOf -> boglarka"
Already in D6, I pretty much replaced all my taxonomies by setting up a content type as "taxonomy" and creating node references for it in "tagged" nodes. This way I got all the benefits of nodes such as moderation, i18n etc etc. Sayonara taxonomy...
Comment #8
xjmRegarding
$term->parents
and$term->children
, I was referring to their use intaxonomy_get_tree()
(without entities).Also: catch pointed out that we could also backport an improved API as D7 contrib.
Comment #9
catch@giorgio79 - while you can make a relation like that, it doesn't mean there are any functions for generating trees of relationships etc.
Comment #10
giorgio79 CreditAttribution: giorgio79 commentedcatch - I guess it is just a question of little Views magic :) SQL is perfectly enough, no need for fancy functions.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedI think we need to go one step back here and implement hierarchical support for any entity types. Once we have done that, we could *also* refactor the menu link system to use that.
Comment #12
JeremyFrench CreditAttribution: JeremyFrench commentedI think it is a great idea to look at refactoring, in fact I was looking to post something along these lines last week. I have been badly bitten by taxonomy_get_tree() in the past.
A fundamental question, which I don't think has been covered here. Do we want to allow a function to return a whole vocabulary. It may be handy in some cases, and from a DX perspective in those cases it is quite handy. But if you get a lot of terms (tens of thousands) in your vocabulary it can snarl up your site.
Do we want to keep the DX simple for most cases, or make it easier to scale to very large vocabularies. I feel that the two are mutually exclusive.
We allow for querying of entities with EntityFieldQuery, could we not make taxonomy functions an extension of that. Rather than having a separate API for it? I'm not advocating that we change the storage here, just the underlying method of accessing.
If we want to make upgrading and DX, we could provide a limited API which uses EntityFieldQuery under the hood.
Comment #13
jantoine CreditAttribution: jantoine commentedTo improve the performance/functionality of this new API, are database structure discussions on the table? I like Damien Tournoud's idea of implementing this for all entities.
Comment #14
jibranThis is an API change and a task so we can't do that in 8.0.x now so moving it to 8.1.x
Comment #15
jibran