Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#36724 closed defect (bug) (fixed)

who=>authors breaks meta arguments in user queries

Reported by: rarst's profile Rarst Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Users Keywords:
Focuses: Cc:

Description

When using who=>authors in user query it silently overrides meta_key, meta_value, and meta_compare arguments.

Among other things this makes it impossible to orderby=>meta_value such query, since it orders by that key.

This behavior is neither explicitly documented or desirable.

Attachments (4)

class-wp-user-query.diff (775 bytes) - added by adrianosilvaferreira 9 years ago.
WP User Class diff
class-wp-user-query.2.diff (975 bytes) - added by adrianosilvaferreira 9 years ago.
class-wp-user-query.3.diff (1012 bytes) - added by adrianosilvaferreira 9 years ago.
class-wp-user-query.4.diff (2.3 KB) - added by adrianosilvaferreira 9 years ago.

Download all attachments as: .zip

Change History (14)

#1 @boonebgorges
9 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added

Good call. It should be possible to rearrange this so that 'who' is added as a 'meta_query' clause instead of overriding 'meta_key' and 'meta_value'. The roles/role__in/role__not_in params work this way.

@adrianosilvaferreira
9 years ago

WP User Class diff

#2 @adrianosilvaferreira
9 years ago

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

Following instructions from @boonebgorges I've added "who" lines to the "meta_query".

#3 @boonebgorges
9 years ago

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

Thanks for the patch, @adrianosilvaferreira! Unfortunately, this approach won't work for a couple reasons. It will create a meta_query parameter that is not well-formed: meta queries must be multi-dimensional arrays, and each key/value pair must be part of its own array:

'meta_query' => array(
    array(
        'key' => 'foo',
        'value' => 'bar',
    )
)

You're missing a level of nesting. Also, if you simply append a clause to the existing meta_query, you may cause problems if the top-level 'relation' is 'OR'.

To make this work right, a new clause will need to be created for the 'who' parameter, and that clause will need to be joined to the existing meta_query using relation=AND.

#4 @adrianosilvaferreira
9 years ago

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

Ups @boonebgorges, you are right! I've fixed this and attached a new diff. Please check it out.

#5 @adrianosilvaferreira
9 years ago

  • Keywords 2nd-opinion added

#6 @boonebgorges
9 years ago

  • Keywords needs-patch added; dev-feedback has-patch 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.6

Thanks, @adrianosilvaferreira. This patch is looking closer. However, the way you've structured it, a 'who' parameter will override any 'meta_query' passed to the function. You will need to assemble a meta query clause for the 'who' stuff, and then append it to the value of 'meta_query', in the same way that we do with the $role_queries clause in the same method.

We will also need unit tests that demonstrate that 'meta_query' and 'who' work together properly.

#7 @adrianosilvaferreira
9 years ago

  • Keywords has-patch dev-feedback 2nd-opinion added; needs-patch removed

Thanks @boonebgorges for looking into it. I've made a few changes and attached a new patch, I hope that it's correct now, fingers crossed :)

#8 @boonebgorges
9 years ago

  • Keywords needs-unit-tests good-first-bug has-patch dev-feedback 2nd-opinion removed
  • Owner set to boonebgorges
  • Status changed from new to assigned

Thank you, @adrianosilvaferreira. This patch is looking pretty good.

In writing a unit test, I found a previous bug that sorta needs to be addressed in order to fix this ticket.

Short version: because of an accident of implementation, it was possible to pass a malformed meta_query when using the who parameter and still have the meta_query recognized.

Long version: https://core.trac.wordpress.org/browser/tags/4.5.1/tests/phpunit/tests/user/query.php?annotate=blame&marks=690-693#L670 shows that a passing test is using improper syntax for meta_query:

'meta_query' => array(
    'key' => 'foo',
    'value' => 'bar',
)

is missing a level of nesting. It should be:

'meta_query' => array(
    array(
        'key' => 'foo',
        'value' => 'bar',
    ),
)

The offending test was introduced in [32207]. In that changeset, 'who' params were translated to meta_value and meta_key before WP_Meta_Query::parse_query_vars() was called. In parse_query_vars(), the presence of a "primary meta query" and "existing meta query" resulted in the two being concatenated in such a way that the invalid meta_query was inadvertantly nested in an array, accidentally turning it into proper syntax. However, I think that the bug was probably originally introduced in [30094], which caused parse_query_vars() to be run both before and after the role parameter.

In the vast majority of cases, the improperly nested syntax described above will simply be ignored by WP_Meta_Query. It's only when combined with who in the context of WP_User_Query that it will accidentally work. So, while it's technically a compatibility break to remove this support (which inadvertently happens with class-wp-user-query.4.diff), I think it's an acceptable break that is very unlikely to affect anyone.

#9 @boonebgorges
9 years ago

In 37359:

Tests: Correct 'meta_query' syntax in test related to WP_User_Query 'who' param.

The test, introduced in [32207], used the incorrect syntax for 'meta_query' -
one fewer level of array-nesting than what WP_Meta_Query requires. This
slip uncovered a bug introduced into WP_User_Query in [30094], whereby
an incorrectly formatted 'meta_query' parameter would be properly parsed by
WP_User_Query when passed alongside who=authors.

We need to fix the inconsistent syntax in order to resolve #36724.

See #36724, #32019, #23849, #27026.

#10 @boonebgorges
9 years ago

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

In 37360:

Users: The 'who' parameter should not interfere with 'meta_key' + 'meta_value' in WP_User_Query.

Props adrianosilvaferreira.
Fixes #36724.

Note: See TracTickets for help on using tickets.