Cannot intelligently target using the `get_terms_args` filter

[johnjamesjacoby]

I'm filtering get_terms_args in the get_terms() function, but because I cannot see what the original $args parameter was that was passed into get_terms(), I cannot make an educated decision about whether or not my plugin should override any default arguments.

Example code:

add_filter( 'get_terms_args', function( $args, $taxonomies ) {
    if ( 'id' !== $args['orderby'] ) {
        $args['orderby'] = 'id';
    }
    return $args;
} ), 10, 2 );

The above snippet will always force get_terms() to order by id which actually isn't what I want to do. I want to only order by id if the orderby passed into get_terms() was not id.

This happens because of the following code in get_terms()

$args = wp_parse_args( $args, $defaults );

Not an unusual line (over 80 other occurrences in core) but it's not ideal because it overwrites the original $args parameter, and filters cannot compare the two variables to make more intelligent decisions.

[johnjamesjacoby]

Pretty invasive $r vs. $args replacement to get_terms()

[johnjamesjacoby]

33369.01.patch proposes the following changes in get_terms():

• Prefer $r over $args so that the original $args parameter remains intact
• Pass both $r and $args into relevant filters
• Remove old byref $empty_array variable and just use array() instead
• Remove unused $single_taxonomy variable
• Existing terms tests all pass

A few things that I anticipate:

• Big ugly patch; will go stale quickly
• Taxonomy terms going through multi-cycle refactoring; touching this is scary for less important reasons
• On a long enough timeline, the guts of get_terms() should be moved to a modern WP_Term_Query class which would likely nullify the efforts in this patch anyways

[knutsp]

It could be argued that a function using defaults should return the same result when the argument was explicit and equal to the default, as when (deliberately) omitted. My trust in the documentation of a function would be damaged if I found this not to be true.

In other words, I think get_terms( 'mytax', array( 'ordery' => 'name' ) ) and get_terms( 'mytax' ) should return the same results when called in the same context, regardless of any filter.

That is why I'm not sure that filters really should be informed on how the arguments was given, only their value after merging with defaults. The way WordPress uses wp_parse_args() resembles that of PHP when using optional arguments.

Do you have a use case where this "knowledge" is the only way to do what you want?

Are there any other filters in core that reveals the arguments as given before defaulting?

[johnjamesjacoby]

It could be argued that a function using defaults should return the same result when the argument was explicit and equal to the default, as when (deliberately) omitted. My trust in the documentation of a function would be damaged if I found this not to be true.

If I understand you correctly, I don't think this is true. See get_tags() for a (maybe poor) example of this:

$r = apply_filters( 'get_media_item_args', $args );

In other words, I think get_terms( 'mytax', array( 'orderby' => 'name' ) ) and get_terms( 'mytax' ) should return the same results when called in the same context, regardless of any filter.

I disagree. The reason filters exist is to allow for overriding the environment and it's relative variables.

That is why I'm not sure that filters really should be informed on how the arguments was given, only their value after merging with defaults. The way WordPress uses wp_parse_args() resembles that of PHP when using optional arguments.

I understand. And I am positive that most of WordPress core operates this way.

Do you have a use case where this "knowledge" is the only way to do what you want?

See above. In this case, which might be unique, there is a default orderby of name in the defaults, which gets transferred to $args if none are passed. That funnels into the get_terms_orderby filter, which includes the query clause and the $args array, but orderby is never empty unless it's explicitly passed as such, so it's not possible to conditionally filter it.

Are there any other filters in core that reveals the arguments as given before defaulting?

get_media_item_args and wp_link_pages_args are the two that I found at a cursory. FWIW, most of BuddyPress's & bbPress's template tags works this way, to allow JIT manipulating of return values based on both the merged arguments and the original ones. Basically, the more relevant data that's passed into a filter is the more useful it is.

---

One other way to maybe approach this would be to remove all references to 'orderby' => 'name' from relative get_terms() usages, or remove the default t.name (if orderby somehow ends up empty) with some uniquely filterable value.

Ultimately, there's no way to know if t.name was the intended query argument passed into the function, or the default fallback because no orderby argument was passed. Which is also to say, there's no way to override t.name without accidentally doing it almost everywhere all the time.

[knutsp]

Replying to johnjamesjacoby:

Great answer, thank you.

The reason filters exist is to allow for overriding the environment and it's relative variables.

I can't tell how much I agree with that, especially as a plugin developer.

I have always, naively, believed that (well documented) default arguments could really be omitted and explicitly adding them seen as completely redundant, like a well implemented shorthand technique. This is from a template developers view, where one might use get_terms(). And as such, I fully accept the function return may be filtered. Filtered on basis of context and variables available, but not on the chosen programming technique.

All I want is complete predictability, and maintain that documented defaults allow for shorthand coding. Even if Core is not perfect at that, as it is.

Looking forward to more opinions.

[boonebgorges]

If I'm understanding knutsp's concern correctly, I think the concern is somewhat misplaced. Filters like 'get_terms_args' mean that it's *already* possible for plugins to modify the args that you pass to the function, whether you pass them explicitly or implicitly (by relying on defaults). Passing the pristine $args to the filter only provides additional info to the plugin developer. Granted, the specific example that johnjamesjacoby used in the original description may be what's scaring you. In real life, it'd be the callbacks responsibility to do something like this:

function wp33369_filter_get_terms_args( $args, $taxonomies, $original_args ) {
    // Only override the value of 'orderby' if it was explicitly passed.
    if ( isset( $original_args['orderby'] ) && 'id' !== $original_args['orderby'] ) {
        $args['orderby'] = 'id';
    }

    return $args;
}
add_filter( 'get_terms_args', 'wp33369_filter_get_terms_args', 10, 3 );

It may sometimes be appropriate for a plugin to override the implicit defaults, and it may sometimes be inappropriate for it to do so. It's up to the plugin developer to decide.

In any case, I think the suggestion of passing the pristine arguments to the filters is a reasonable one. To reduce the churn of changing the variable name in a hundred places, 33369.diff​ copies the explicits args to $_args. johnjamesjacoby, does this capture the spirit of what you're asking for?

[TimothyBlynJacobs]

I really don't like this for the same reason knutsp.

Whether or not I've explicitly passed a parameter, should not have any effect on the output. We commonly rely on the defaults being the defaults. Excluding a default parameter from the args I'm passing to the function is not an implicit waiver that I don't care about that arg. I'm merely trying to make my code more sane, and not the list 20 or so default arguments to a function whenever I call it.

This feels a lot like #22673.

[boonebgorges]

Excluding a default parameter from the args I'm passing to the function is not an implicit waiver that I don't care about that arg.

Agreed, but again, any plugin can currently do the following:

add_filter( 'get_terms_args', function( $args, $taxonomies ) {
    $args['orderby'] = 'id';
    return $args;
} );

If you don't like the idea of plugins running roughshod over your parameters, then what you're really arguing against is the idea of having filters on argument arrays in the first place. What's being proposed here is simply the passing of additional data to an existing filter.

[TimothyBlynJacobs]

No there is a semantic difference. I'm fine with a plugin filtering the args array. What I'm saying is that I don't think that the original args I pass to the function are a valid source of data for making that decision.

Also, FWIW, the get_media_item_args doesn't seem to pass the original args to a filter anywhere. And the wp_link_pages passing the original args array instead of the merged args _seems_ like more of a bug caused by renaming the variable three times :).

[SergeyBiryukov]

Adding more context to existing filters is generally fine, but I have the same concerns as @knutsp and @TimothyBlynJacobs.

I'd expect get_terms( 'mytax', array( 'orderby' => 'name' ) ) and get_terms( 'mytax' ) to return consistent results, whether 'orderby' => 'name' was passed explicitly or not.

[johnjamesjacoby]

Sorry that I haven't been able to more clearly describe what's wrong here.

The problem is there is no way, in a plugin, to know if name was explicitly passed, or was fallen back to as the default.

I wrote a term-order plugin. I want to orderby order by default instead of name and still want name to be available as an option if it's explicitly passed. As the code stands today, I cannot tell the difference.

Does that help clarify?

[TimothyBlynJacobs]

I understand the use case. But what I'm saying is, it that just because I don't pass the orderby parameter, does not mean I don't expect the return value to be name.

[SergeyBiryukov]

Replying to johnjamesjacoby:

Does that help clarify?

Yes, I guess changing the default value unless it was explicitly passed makes sense.

33369.diff​ looks good, but we should also add a changelog entry to the hook docs:

@since 4.4.0 The `$_args` parameter was added.

[iamfriendly]

Refreshed the patch as the functions have moved files, also added the @since docs

[iamfriendly]

The patch needed a refresh as the location of the functions has moved in trunk. 33369.2.diff does just that as well as adding the @since doc strings for the added parameter. I know this is owned by boone and I'm sorry if me doing this has stepped on someone else's toes (is that bad form? If so, I apologise, let me know and it won't happen again) I'm just keen to see this go in for 4.4 :)

[DrewAPicture]

Replying to iamfriendly:

The patch needed a refresh as the location of the functions has moved in trunk. 33369.2.diff does just that as well as adding the @since doc strings for the added parameter. I know this is owned by boone and I'm sorry if me doing this has stepped on someone else's toes (is that bad form? If so, I apologise, let me know and it won't happen again) I'm just keen to see this go in for 4.4 :)

Nope, totally fine to contribute patches even if the ticket has an owner. We're all here to make WordPress better together :-)

[SergeyBiryukov]

Replying to SergeyBiryukov:

Yes, I guess changing the default value unless it was explicitly passed makes sense.

I've just realized this should be a new filter on the $defaults array, no need to modify other filters.

[SergeyBiryukov]

33369.3.diff​ adds 'get_terms_defaults' filter, seems like a quick win.

[SergeyBiryukov]

In 35319:

Docs: Correct duplicate hook reference for get_terms.

See #33369.

[SergeyBiryukov]

In 35320:

Docs: Correct function name in get_terms_args parameter description.

See #33369.

[SergeyBiryukov]

In 35321:

Reorder default arguments in get_terms() for consistency with the hash notation added in [29128].

See #33369.

[SergeyBiryukov]

In 35322:

Add get_terms_defaults filter for the default arguments of get_terms().

Fixes #33369.