Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36878 closed defect (bug) (fixed)

get_term_by returns random result with multiple terms

Reported by: cogmios's profile cogmios Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.5.2
Component: Taxonomy Keywords: needs-docs
Focuses: docs Cc:

Description

current situation: Taxonomy term names are not unique. GET_TERM_BY allows a developer to add a query for name. If multiple names are present e.g.:

\legal\public\contract
\finance\leasing\contract

it will return a random one of the two because of LIMIT 1. A developer who creates code without the content yet in place, could create a bug by only realizing much later that multiple entries exist in the content or never even face the bug since sometimes it delivers the correct one and sometimes it does not. So in the end, the end-user will have to notify the developer something strange is going on...

proposal: the code should protect the developer from introducing bugs. So proposal to either

  • return multiple (remove limit 1) (can't see why not) or
  • return WP_ERROR if multiple so it is at least clear there is a bug
  • or not allow names

Change History (6)

#1 @boonebgorges
9 years ago

  • Focuses docs added
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Hi @cogmios - Thanks very much for the ticket and for the clear explanation of the issue.

A quick note about the history of this issue. Before WP 4.1 or thereabouts, this problem would only arise in cases where two terms were in different branches of a hierarchical taxonomy, because it wasn't possible (through WP's functions, anyway) to create two terms with the same names in any other way, at least not within the same taxonomy. And since get_term_by() always required a $taxonomy argument, the problem would arise fairly rarely.

Versions 4.1 through 4.3 eased some of these requirements - it's now possible for duplicate names, and even slugs, to exist in certain cases, both within and between taxonomies. And in WP 4.4, the $taxonomy param was made optional, meaning that the possibility for ambiguity arose even more - not just with names, but also slugs, since terms in different taxonomies can share a slug.

I'm not sure that we can change the default return value of get_term_by() to be either a WP_Error object or an array. Consider the following:

$term = get_term_by( 'name', 'foo' );
if ( $term ) {
    echo $term->name;
}

This will break if we return a non-falsey value that is not a WP_Term. Even in core, we do this in many spots. Plugins probably do it in thousands of ways.

We could potentially add another parameter (or another value of the $output parameter) that tells get_term_by() to return an array of terms. But in that case, we're really just reproducing get_terms(), which people ought to be using anyway when they might be expecting multiple results.

I think the best we can do here is a note in the documentation that warns people of this danger, and suggests that they use get_terms() (or at least to specify the $taxonomy) when there's the potential for conflict.

#2 @bcworkz
9 years ago

  • Keywords needs-docs needs-codex added; needs-patch removed

I also don't see a way to improve this function without impacting reverse compatibility. There is thus nothing to fix except for improving the documentation.

#3 @bcworkz
9 years ago

  • Keywords needs-codex removed

I added a note to the Codex, and submitted the same to the Code Reference, but it's stuck in the moderation queue there.

#4 follow-up: @DrewAPicture
9 years ago

@bcworkz Your submission on DevHub was approved a couple of weeks ago.

@boonebgorges What's left here?

#5 in reply to: ↑ 4 @boonebgorges
9 years ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.6

Replying to DrewAPicture:

@bcworkz Your submission on DevHub was approved a couple of weeks ago.

@boonebgorges What's left here?

I'm going to add a note to the doc block.

#6 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 37656:

Add a note about uniqueness to the doc block for get_term_by().

get_term_by() always returns a single term, even when more than one term
matches the query parameters. The new note warns developers to use
get_terms() when such ambiguity may result.

Fixes #36878.

Note: See TracTickets for help on using tickets.