WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 21 months ago

#35495 closed enhancement (fixed)

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

Reported by: flixos90 Owned by: 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 22 months ago.
Initial patch allowing an empty $taxonomies parameter
35495.2.diff (6.8 KB) - added by boonebgorges 21 months ago.
35495.3.diff (6.7 KB) - added by flixos90 21 months ago.
35495.4.diff (6.2 KB) - added by boonebgorges 21 months ago.

Download all attachments as: .zip

Change History (20)

@flixos90
22 months ago

Initial patch allowing an empty $taxonomies parameter

#1 @flixos90
22 months ago

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

#2 follow-up: @swissspidy
22 months 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
22 months 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. So I assume that would also mean we would need to add a taxonomies key in the $args array. If a user uses both parameters, but still adds taxonomies in the $args array (makes no sense to me, but it would be possible), what takes precedence there? Or do you mean that the syntax with 1 parameters is only if you don't need a specific taxonomy while the syntax with 2 parameters should be used otherwise?

Last edited 22 months ago by flixos90 (previous) (diff)

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


22 months ago

#5 @boonebgorges
22 months 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.)

#6 @boonebgorges
21 months 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
21 months 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
21 months ago

#8 @flixos90
21 months 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
21 months ago

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

#10 @boonebgorges
21 months 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
21 months 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
21 months 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.

#13 @TimothyBlynJacobs
21 months 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
21 months 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
21 months 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
21 months ago

  • Version changed from trunk to 4.4
Note: See TracTickets for help on using tickets.