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()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

There are several other taxonomy_get_* functions as well. taxonomy_get_vocabularies() at least is also a load function.

sun’s picture

Issue tags: +Novice

So let's rename them all? Would be good to achieve some consistency.

Actually sounds like a nice Novice issue, no? :)

chris.leversuch’s picture

Assigned: Unassigned » chris.leversuch
Status: Active » Needs review
FileSize
28.29 KB

I 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.

xjm’s picture

Hmm, I'm the most uncertain about renaming taxonomy_get_tree() to taxonomy_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

chris.leversuch’s picture

I 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()?

droplet’s picture

taxonomy_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.

chris.leversuch’s picture

A 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().

droplet’s picture

@chris,
any example? how to assign 2 parents ?

EDIT: I found it. Relations -> Parents

sun’s picture

1) 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():

taxonomy_get_parents()     -> taxonomy_term_load_parents()
taxonomy_get_parents_all() -> taxonomy_term_load_parents_all()
taxonomy_get_children()    -> taxonomy_term_load_children()

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:

taxonomy_get_term_by_name() -> taxonomy_term_load_multiple_by_name()
taxonomy_get_vocabularies() -> - DROP - [taxonomy_vocabulary_load_multiple(FALSE)]

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() -> - RETAIN -
xjm’s picture

I actually have for awhile wanted to rename whatever_parents_all() to whatever_ancestors() since it's more easily understood. That's likely out of scope here, though.

I think sun's list in #9 looks good.

sun’s picture

_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.

chris.leversuch’s picture

The 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.

xjm’s picture

Currently 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.

chris.leversuch’s picture

New patch with functions listed in #9.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +API change

Thanks! This looks good to go for me.

catch’s picture

Title: taxonomy_get_term_by_name() should be taxonomy_term_load_multiple_by_name() » Change notification for: taxonomy_get_term_by_name() should be taxonomy_term_load_multiple_by_name()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Works for me. Committed/pushed to 8.x, this will need a change notification.

reprogrammer’s picture

I assigned the issue to myself to write a change notification for it.

reprogrammer’s picture

I drafted an API change notification for this issue. Please review my change notification.

xjm’s picture

Title: Change notification for: taxonomy_get_term_by_name() should be taxonomy_term_load_multiple_by_name() » taxonomy_get_term_by_name() should be taxonomy_term_load_multiple_by_name()
Priority: Critical » Normal
Status: Active » Fixed

Change notice looks great. Thanks @reprogrammer!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

chx’s picture

I filed one for the removal separately. http://drupal.org/node/1972410

Pancho’s picture