Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#35495 closed enhancement (fixed)

Allow `get_terms()` to return all terms, make `$taxonomy` parameter optional

Reported by: flixos90's profile flixos90 Owned by: boonebgorges's profile boonebgorges
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Now that term IDs are unique, we can continue work on making term functions more flexible. One thing I would consider useful would be to make the $taxonomy parameter of get_terms() optional, furthermore allowing to not specify any taxonomy, resulting in all terms of any taxonomy being returned.

I will upload an initial patch which provides this functionality. There might be areas in the function that require additional focus to apply this change, but in general it works with this approach.

Attachments (4)

35495.diff (2.7 KB) - added by flixos90 8 years ago.
Initial patch allowing an empty $taxonomies parameter
35495.2.diff (6.8 KB) - added by boonebgorges 8 years ago.
35495.3.diff (6.7 KB) - added by flixos90 8 years ago.
35495.4.diff (6.2 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (21)

@flixos90
8 years ago

Initial patch allowing an empty $taxonomies parameter

#1 @flixos90
8 years ago

  • Keywords has-patch needs-unit-tests dev-feedback added

#2 follow-up: @swissspidy
8 years ago

I guess we can't just use function get_terms( $args = array() ) {…} and func_get_args() to maintain backwards compatibility? Would look more clean than get_terms( null, $args ).

#3 in reply to: ↑ 2 @flixos90
8 years ago

Replying to swissspidy:

I guess we can't just use function get_terms( $args = array() ) {…} and func_get_args() to maintain backwards compatibility? Would look more clean than get_terms( null, $args ).

+1 for this idea, it's definitely cleaner

Version 0, edited 8 years ago by flixos90 (next)

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

#5 @boonebgorges
8 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

I like @swissspidy's approach more. Could we see a patch for this? And unit tests that demonstrate the conflict between 'taxonomies' parameters? (For the record, I am not concerned about this; if anyone is passing an array of args to the first param, then they clearly intend for the second param to be ignored.)

@boonebgorges
8 years ago

#6 @boonebgorges
8 years ago

  • Keywords has-patch added; needs-unit-tests needs-patch removed

35495.2.diff is a first pass at get_terms( $args ). When a non-empty second param is passed to the function, or when an array is passed to the first param that doesn't look like an $args array, the function falls back on the old get_terms( $taxonomies, $args ) format.

I don't see the possibility for conflicts between legacy and new arg formats. If the second param is present, the first will be parsed as a taxonomy array. $legacy_args['taxonomy'] is effectively ignored. This should result in total backward compatibility for anyone who happens to be passing $args['taxonomy'] in an existing plugin.

What do people think? Especially regarding the documentation. (ping @DrewAPicture)

#7 @swissspidy
8 years ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

Thanks for the patch! I was already mentally preparing myself to dive into this, glad I don't have to now :-)

The logic looks good, though $args = array(), $legacy_args = '' sounds a bit confusing. Can't we just name this parameter $deprecated, so that it's clear you should really only use get_terms( $args )?

return new WP_Error( 'invalid_taxonomy', __( 'Invalid taxonomy' ) );

Is it possible to pass $taxonomy here, to make it obvious which taxonomy is invalid?

Also, could we perhaps use array_filter() for these taxonomy_exists() and is_taxonomy_hierarchical() checks? It should be just as fast and a bit cleaner to read.


Note: We may still change the milestone to 4.5 if we get a commit-ready patch

@flixos90
8 years ago

#8 @flixos90
8 years ago

@boonebgorges:
The patch looks good to me as well, 35495.3.diff fixes two issues I found when querying terms without specifying any taxonomy (a PHP notice thrown and an invalid SQL query).

@swissspidy:
Sounds like a good idea to me, this will provide more clarity, especially passing which taxonomy is invalid. Should I write another patch with your proposal included? I'm on it right now anyway :)

#9 @swissspidy
8 years ago

By the way, what about making the same changes to get_term()?

#10 @boonebgorges
8 years ago

  • Milestone changed from Future Release to 4.5

Stripping the leading AND off the clause drives me nuts. Let's assemble the WHERE clause in get_terms() properly so that we don't have to do this. (We've already made the corresponding change in other query classes.)

#11 @boonebgorges
8 years ago

In 36598:

In get_terms(), assemble WHERE conditions in an array instead of concatenating.

This method is more reliable when adding new WHERE conditions.

See #35495.

#12 @boonebgorges
8 years ago

Is it possible to pass $taxonomy here, to make it obvious which taxonomy is invalid?

Yes, let's add this, in a separate changeset.

Also, could we perhaps use array_filter() for these taxonomy_exists() and is_taxonomy_hierarchical() checks? It should be just as fast and a bit cleaner to read.

OK, but I would want to verify that there's full test coverage before making changes like this. May be worth a separate ticket.

By the way, what about making the same changes to get_term()?

get_term() hasn't required the $taxonomy param since [34997].

Can't we just name this parameter $deprecated, so that it's clear you should really only use get_terms( $args )?

I didn't do this originally because I didn't want to throw a _deprecated_argument() notice. This would be extremely annoying for developers, especially this late in the cycle. (Maybe after 4.5 comes out.) But yes, I'm OK with the variable $deprecated.

35495.4.diff is an updated patch.

@boonebgorges
8 years ago

#13 @TimothyBlynJacobs
8 years ago

I don't think this should throw a deprecated argument for a few releases. For anyone who has to support less than 4.5 it will liter the codebase with WP version checks. Additionally, the prior syntax is more clean when doing a simple query for only one taxonomy.

#14 @DrewAPicture
8 years ago

Talked with @boonebgorges a bit about approach and I think this is realistically the best we're going to get in terms of documentation. It's definitely parameter gymnastics.

Initially I was thinking adding the $deprecated parameter might be overkill but I think we need to do it in case we ever decide to add a third parameter (yay future-proofing). In fact, I would even suggest adding an @internal comment explaining the presence of the second "dead" parameter.

#15 @boonebgorges
8 years ago

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

In 36614:

Allow get_terms() to fetch terms regardless of taxonomy.

get_terms() has historically required that a taxonomy be specified when
querying terms. This requirement is related to the fact that terms could
formerly be shared between taxonomies, making $taxonomies critical for
disambiguation. Since terms can no longer be shared as of 4.4, it'
s desirable to be able to query for terms regardless of what taxonomy they're in.

Because it's now optional to pass taxonomies, it's no longer necessary to have
$taxonomies as the first (and required) parameter for get_terms(). The new
function signature is get_terms( $args ), where 'taxonomy' can (optionally) be
passed as part of the $args array. This syntax is more consistent with
functions like get_users() and get_posts().

We've maintained backward compatibility by always giving precedence to the old
argument format. If a second parameter is detected, or if it's detected that
the first parameter is a list of taxonomy names rather than an $args array,
get_terms() will parse the function arguments in the legacy fashion.

Props flixos90, swissspidy, DrewAPicture, boonebgorges.
Fixes #35495.

#16 @johnbillion
8 years ago

  • Version changed from trunk to 4.4

#17 @boonebgorges
5 years ago

In 45888:

Taxonomy: Ensure consistency of hide_empty in term queries when taxonomy is excluded.

When querying for terms in hierarchical categories using hide_empty=true,
results have historically included parent terms which are themselves
unattached to any objects (are "empty") but which have non-empty descendent
terms. Because this process involves walking the descendant tree, we avoid it
when we detect that the queried taxonomies are not hierarchical. (This
behavior was introduced in [5525].)

When the taxonomy parameter of get_terms() was made optional - see #35495,
[36614] - it affected the mechanism for avoiding unneccessary tree walks,
since there may not be any explicitly declared taxonomies to run through
is_taxonomy_hierarchical(). As a result, term queries excluding taxonomy
did not check descendants, and empty parents with non-empty children were not
included in hide_empty results.

We correct the behavior by crawling term descendants when the taxonomy
argument is absent, which means that we're querying for terms in all taxonomies.

Props smerriman.
Fixes #37728.

Note: See TracTickets for help on using tickets.