taxonomy_get_term_by_name()
is a helper to load taxonomy term entities.
Because multiple terms can match a passed in $name, it always returns more than one (unlike user_load_by_name()
for example).
The function name is completely custom, it should be taxonomy_term_load_multiple_by_name()
Comment | File | Size | Author |
---|---|---|---|
#14 | 1377628-rename-taxonomy_get-functions-1.patch | 22.15 KB | chris.leversuch |
#3 | 1377628-rename-taxonomy_get-functions.patch | 28.29 KB | chris.leversuch |
Comments
Comment #1
xjmThere are several other
taxonomy_get_*
functions as well.taxonomy_get_vocabularies()
at least is also a load function.Comment #2
sunSo let's rename them all? Would be good to achieve some consistency.
Actually sounds like a nice Novice issue, no? :)
Comment #3
chris.leversuch CreditAttribution: chris.leversuch commentedI searched through taxonomy.module for all taxonomy_get_* functions -
taxonomy_get_term_by_name() -> taxonomy_term_load_multiple_by_name()
taxonomy_get_vocabularies() -> taxonomy_vocabulary_load_all()
taxonomy_get_parents() -> taxonomy_term_load_parents()
taxonomy_get_parents_all() -> taxonomy_term_load_parents_all()
taxonomy_get_children() -> taxonomy_term_load_children()
taxonomy_get_tree() -> taxonomy_vocabulary_load_tree()
Does _taxonomy_get_tid_from_term() need changing?
I also grep'd for each function and changed any occurances of the old functions names.
Comment #4
xjmHmm, I'm the most uncertain about renaming
taxonomy_get_tree()
totaxonomy_vocabulary_load_tree()
--we're loading term objects there, not vocabs, and plus by default it doesn't do full loads.On the fence about some of the other names. @sun, thoughts?
See also: #1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience
Comment #5
chris.leversuch CreditAttribution: chris.leversuch commentedI was thinking that the tree represents a vocabulary not a term, but I guess it is a tree of terms :) taxonomy_vocabulary_load_term_tree()?
Comment #6
droplet CreditAttribution: droplet commentedtaxonomy_get_parents() & taxonomy_get_parents_all() are confused me.
I thought taxonomy_get_parents() is return all parents, ONE or more than ONE ( = taxonomy_get_parents_all() )
but in fact taxonomy_get_parents() is only return an array contains ONE term object ??
bug or document error or my problem:
http://api.drupal.org/api/drupal/core--modules--taxonomy--taxonomy.modul...
both returned value are not consistent.
Comment #7
chris.leversuch CreditAttribution: chris.leversuch commentedA term can have multiple parents so taxonomy_get_parents() can return more than 1 term.
The description for taxonomy_get_parents_all() is 'Find all ancestors of a given term ID' so it's actually returning parents, grandparents etc. It should probably be called taxonomy_term_load_ancestors().
Comment #8
droplet CreditAttribution: droplet commented@chris,
any example? how to assign 2 parents ?
EDIT: I found it. Relations -> Parents
Comment #9
sun1) taxonomy_get_vocabularies() should be taxonomy_vocabulary_load_multiple(), using the same function signature as node_load_multiple() (or taxonomy_term_load_multiple()); i.e., all previous taxonomy_get_vocabularies() calls turn into taxonomy_vocabulary_load_multiple(FALSE).
EDIT: taxonomy_vocabulary_load_multiple() already exists, so we're effectively dropping this obsolete helper function and merely change all callers instead.
2) taxonomy_get_parents() & Co. are loading full term entities, so the suggested renames look very fine to me, as they are loading terms and use a similar function signature like taxonomy_term_load():
3) taxonomy_get_tree(), as @xjm already mentioned, gets terms not vocabularies, and only loads full terms when passing a function argument. Tough one. Perhaps rename to taxonomy_term_get_tree(). But perhaps it's not worth to rename this one... yes, let's leave that alone.
Thus, in total:
Comment #10
xjmI actually have for awhile wanted to rename
whatever_parents_all()
towhatever_ancestors()
since it's more easily understood. That's likely out of scope here, though.I think sun's list in #9 looks good.
Comment #11
sun_parents() is to _children() what _ancestors() is to ?
In other words: I could only get behind _ancestors, if there was an equivalent to _children for the opposite. In terms of clarity (as a non-native English speaker), I like _parents and _children though. _ancestors is kind of a synonym of _parents in my book, so having both _parents and _ancestors would be highly confusing for me.
Comment #12
chris.leversuch CreditAttribution: chris.leversuch commentedThe opposite of ancestor is descendant - first google result i found http://www.elearnenglishlanguage.com/difficulties/ancestordescendant.html
Ancestors include parents but goes further, grandparents, great-grandparents etc.
Descendants include children but goes further, grandchildren, great-grandchildren etc.
Comment #13
xjmCurrently there is no
_children_all()
function. It would be_descendants()
if we went with_ancestors()
.Edit: see #1207326: Refactor taxonomy hierarchy API for performance, consistency, and convenience and #251595: Add taxonomy_term_load_descendents().
Edit 2: I think the use of the term "descendants" is pretty common. The issue with
_all()
is that it is ambiguous as to what we are getting "all" of.Comment #14
chris.leversuch CreditAttribution: chris.leversuch commentedNew patch with functions listed in #9.
Comment #15
sunThanks! This looks good to go for me.
Comment #16
catchWorks for me. Committed/pushed to 8.x, this will need a change notification.
Comment #17
reprogrammer CreditAttribution: reprogrammer commentedI assigned the issue to myself to write a change notification for it.
Comment #18
reprogrammer CreditAttribution: reprogrammer commentedI drafted an API change notification for this issue. Please review my change notification.
Comment #19
xjmChange notice looks great. Thanks @reprogrammer!
Comment #21
chx CreditAttribution: chx commentedI filed one for the removal separately. http://drupal.org/node/1972410
Comment #22
PanchoFiled #2043163: Rename taxonomy_term_load_parents_all() to taxonomy_term_load_ancestors() as a followup per #7 and #10 ff.