Opened 9 years ago
Closed 9 years ago
#33369 closed enhancement (fixed)
Cannot intelligently target using the `get_terms_args` filter
Reported by: | johnjamesjacoby | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | 2.2 |
Component: | Taxonomy | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
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.
Attachments (4)
Change History (29)
#1
@
9 years ago
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 usearray()
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 modernWP_Term_Query
class which would likely nullify the efforts in this patch anyways
#3
@
9 years ago
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( 'orderby' => '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?
#4
follow-up:
↓ 5
@
9 years ago
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' ) )
andget_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.
#5
in reply to:
↑ 4
@
9 years ago
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.
#6
@
9 years ago
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?
#7
@
9 years ago
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.
#8
@
9 years ago
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.
#9
@
9 years ago
No there is a semantical 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 :).
#10
@
9 years ago
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.
#11
follow-up:
↓ 13
@
9 years ago
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?
#12
@
9 years ago
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
.
#13
in reply to:
↑ 11
;
follow-up:
↓ 19
@
9 years ago
- Milestone changed from Awaiting Review to 4.4
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.
This ticket was mentioned in Slack in #core by sergey. View the logs.
9 years ago
#17
follow-up:
↓ 18
@
9 years ago
- Keywords needs-docs removed
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 :)
#18
in reply to:
↑ 17
@
9 years ago
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 :-)
#19
in reply to:
↑ 13
@
9 years ago
- Keywords needs-patch added; has-patch removed
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.
#21
@
9 years ago
- Keywords has-patch commit added; 2nd-opinion needs-patch removed
- Milestone changed from Future Release to 4.4
- Type changed from feature request to enhancement
33369.3.diff adds 'get_terms_defaults'
filter, seems like a quick win.
Pretty invasive $r vs. $args replacement to
get_terms()