Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37568 closed defect (bug) (fixed)

`get_terms()` falsely assumes that legacy arguments are used

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

Description

As stated by @wonderboymusic on Slack (https://wordpress.slack.com/archives/core/p1470289963002086), get_terms() under some conditions assumes that legacy arguments are used when they actually aren't. This can cause the taxonomy argument to have some weird values.

Attachments (3)

37568.diff (1.5 KB) - added by flixos90 8 years ago.
37568.2.diff (2.6 KB) - added by flixos90 8 years ago.
37568.3.diff (1.6 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (11)

@flixos90
8 years ago

#1 @flixos90
8 years ago

  • Keywords has-patch added; needs-patch removed

37568.diff fixes that problem by introducing a new static method to WP_Meta_Query which returns the default values for it. These default values are then included in the query var defaults in WP_Term_Query. They could alternatively be placed directly in WP_Term_Query, but I think this approach makes more sense since several classes use WP_Meta_Query and could use its new method.

#2 @ocean90
8 years ago

  • Keywords needs-unit-tests added

Related: [36614]

@flixos90
8 years ago

#3 @flixos90
8 years ago

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

In 37568.2.diff I added a unit tests to verify the issue is fixed.

@boonebgorges
8 years ago

#4 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 4.7

@flixos90 - Thanks for the debugging and the patches.

Adding the meta-related query params to the $query_var_defaults array seems like the right approach to me.

They could alternatively be placed directly in WP_Term_Query, but I think this approach makes more sense since several classes use WP_Meta_Query and could use its new method.

This feels off to me. First, the naming is weird: what's really being provided here is the "default top-level query params that are supported by parse_query_vars()". (IMO, meta_query is a different kind of thing from meta_key, meta_type, etc.) This is hard to capture in a method name, which suggests to me that it's not a discrete enough concept to abstract in the way you've suggested. I'd like to explore a better way to centralize the definition and processing of WP_*_Query parameters, but this should be tackled as a larger project. Since get_terms() is the only place where the defaults are being used in this way, let's hold off on the additional abstraction. See 37568.3.diff.

As @ocean90 notes, this dates to [36614], which means it's a regression in 4.5, not 4.6. For this reason, I don't think it's necessary to put it into the 4.6 RC, but I'll let @ocean90 make the call there. @wonderboymusic In the meantime, you should be able to work around by passing any additional parameter to get_terms() - something redundant like:

$terms = get_terms( [
    'meta_key' => 'foo',
    'meta_value' => 1,
    'orderby' => 'name',
] );

#5 @wonderboymusic
8 years ago

  • Milestone changed from 4.7 to 4.6
  • Owner set to boonebgorges
  • Priority changed from normal to high
  • Severity changed from normal to critical
  • Status changed from new to assigned

#6 @wonderboymusic
8 years ago

  • Milestone changed from 4.6 to 4.7
  • Owner changed from boonebgorges to wonderboymusic
  • Priority changed from high to normal
  • Severity changed from critical to normal

#7 @wonderboymusic
8 years ago

that was an accident

#8 @wonderboymusic
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 38337:

Taxonomy: in get_terms(), do not assume that legacy args are being passed when the only params are top-level meta_* values. Add keys in WP_Term_Query::__construct().

Adds unit tests.

Props flixos90, boonebgorges.
Fixes #37568.

Note: See TracTickets for help on using tickets.