WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 14 months ago

Last modified 12 months ago

#22212 closed enhancement (fixed)

WP_User_Query( array('role' => $role) ) should accept array or not return anything if array

Reported by: thomask Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: major Version:
Component: Users Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

I got a small problem with WP_User_Query - i wanted to find users from more then one roles to send them e-mail, so i did

$role = array('role-1','role-2);
$wp_user_search = WP_User_Query( array('role' => $role) );
$users = $wp_user_search->get_results()
foreach ($user) ... send wp_mail

problem is, that role param accepts only string. It would not be a problem if it would return nothing, problem was, that it actually returns ALL users, so i have send my author-only emails to all thousands users in database :-(

I know that it is my fault, that i have to read documentation, but i thought, that it works as other wp queries, so if the parame do not fit, it throws error, but it does not.

Attachments (12)

query.php.patch (842 bytes) - added by barrykooij 4 years ago.
Unit Test to test user.php.patch
user.php.patch (1.6 KB) - added by barrykooij 4 years ago.
user.php.2.patch (2.1 KB) - added by barrykooij 4 years ago.
22212.patch (5.6 KB) - added by barrykooij 3 years ago.
user.php & unit tests
23756.diff (3.9 KB) - added by sirbrillig 3 years ago.
Re-added @ to ticket number in test.
22212.2.patch (6.6 KB) - added by barrykooij 3 years ago.
22212.3.patch (6.7 KB) - added by mordauk 2 years ago.
22212.4.patch (10.4 KB) - added by swissspidy 14 months ago.
22212.5.patch (11.1 KB) - added by swissspidy 14 months ago.
22212.diff (14.3 KB) - added by boonebgorges 14 months ago.
22212.2.diff (14.9 KB) - added by swissspidy 14 months ago.
22212.3.diff (1.8 KB) - added by ocean90 14 months ago.

Download all attachments as: .zip

Change History (78)

#1 @knutsp
4 years ago

  • Cc knut@… added
  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement

+1. I use array_merge on two or more queries. It would be nice to pass an array, as long as the result is one effective query.

#2 @scribu
4 years ago

needs-patch indeed.

#3 @greenshady
4 years ago

  • Cc justin@… added

+1 to allowing an array of roles. I have to run multiple user queries and merge the arrays on a regular basis.

#4 @ramiy
4 years ago

  • Cc r_a_m_i@… added

#5 @SergeyBiryukov
4 years ago

#23258 was marked as a duplicate.

#7 @helgatheviking
4 years ago

+1 From me. Needs to accept an array of roles.

Of course, I'd also like to see a default template for a list of all users. The archive for an individual author exists, but I think there should also be an archive of all the authors, but I guess that should be a new ticket.

#8 @barrykooij
4 years ago

  • Cc wpcore@… added
  • Keywords has-patch added; needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

Please see attached patch for fix. I've set the default value of 'role' to array with backwards support for give a String as argument.

#9 @barrykooij
4 years ago

  • Keywords needs-testing added

#10 follow-up: @ocean90
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Tickets are marked as fixed when a changeset is commited to trunk which fixes the issue.

#11 in reply to: ↑ 10 @barrykooij
4 years ago

Replying to ocean90:

Tickets are marked as fixed when a changeset is commited to trunk which fixes the issue.

I'm sorry!

@barrykooij
4 years ago

Unit Test to test user.php.patch

#12 @markoheijnen
4 years ago

  • Keywords dev-feedback added; needs-testing removed

@barrykooij
4 years ago

#13 @scribu
4 years ago

  • Keywords dev-feedback removed
$qv['meta_query']['relation'] = 'OR';

So, this won't work as expected:

$users = get_users( array(
  'role' => array( 'foo', 'bar' ),
  'meta_query' => array(
    'relation' => 'AND',
    array(
      'key' => 'some_other_key',
      'compare' => 'EXISTS'
    ),
    array(
      'key' => 'another_key',
      'compare' => 'EXISTS'
    )
  )
) );

OTOH, 'relation' => 'OR' already doesn't work as expected when you pass any value to 'role'.

Maybe use a separate WP_Meta_Query instance for querying the roles and definitely add tests for both of these corner cases.

Last edited 4 years ago by scribu (previous) (diff)

#14 @barrykooij
4 years ago

  • Keywords needs-unit-tests needs-testing added

Thanks for your feedback. I've tried to setup a separate instance of WP_Meta_Query but this resulted in a database error because the same meta table was joined without an alias multiple times (WP_Meta_Query.get_sql). As far as I could see it's not possible to use WP_Meta_Query to do multiple meta queries from separate WP_Meta_Query objects on the same meta table.

I did not want to change or extends the WP_Meta_Query class so I've extended the WP_User_Query.prepare_query function to let it create the SQL statements required to do a proper user fetch.

Attached patch (user.php.2.patch) solves more then reported bug/enhancement, it also solves:

  • using the 'meta_query' in combination with the 'OR' relation returns all users (with all roles)
  • using the 'meta_query' in combination with the 'OR' relation in multisite returns all users (of all blogs)

I will add Unit Tests tomorrow.

Last edited 4 years ago by barrykooij (previous) (diff)

#15 @alex-ye
4 years ago

I think it should be work like this

WP_User_Query( array('role__in' => array( 'role-1', 'role-2' ) ) )

And when array is used in 'role' arg , it should return null .

#16 @markoheijnen
4 years ago

Alex, I disagree with that kind of markup since you have two ways to select role. So what happens when you pass the rolein and role arguments? That can trow confusion.

#17 @markoheijnen
3 years ago

  • Milestone changed from Awaiting Review to 3.7

I would love to tackle this in 3.7

#18 @nacin
3 years ago

As users are allowed to have multiple roles, role => array( role1, role2 ) does seem confusing — is it an AND, or an OR? What if users have role1, role2, and *also* role 3? The patch assumes an OR, which might make sense if you are querying editor OR administrator, but not necessarily so for custom roles.

I would think that AND is more obvious for an array passed to role. A role__in as an OR also fits similar patterns elsewhere, such as many vars in WP_Query.

Sidenote: This going to be a fairly nasty meta query, at least until #10201 is ever tackled. If we do this API enhancement, let's not trumpet it too loudly. :-)

#19 @nacin
3 years ago

  • Milestone changed from 3.7 to Future Release

No feedback in 5 weeks. Also noticing this above:

OTOH, 'relation' => 'OR' already doesn't work as expected when you pass any value to 'role'.

It feels like there are deeper issues here.

@barrykooij
3 years ago

user.php & unit tests

#20 @barrykooij
3 years ago

Updated the user.php to latest trunk and fixed a small bug in it. Also added 6 unit tests.

Talked this over with nacin, role => array( role1, role2 ) will still result in returning both role1 and role2 (OR relation), no role__in will be added for now.

#21 @nacin
3 years ago

  • Milestone changed from Future Release to 3.8

#22 @nacin
3 years ago

  • Keywords needs-unit-tests needs-testing removed

Just needs some coding style cleanup, but this is good. Also could use some sanity checks on the SQL, but the tests look solid.

@sirbrillig
3 years ago

Re-added @ to ticket number in test.

#23 @sirbrillig
3 years ago

Argh. Please ignore that last patch. Wrong ticket.

#24 @barrykooij
3 years ago

Can I please get an update on this, is this good to go for 3.8? I've asked nacin multiple times on IRC to please take a look, would be great if we can get this in.

#25 @wonderboymusic
3 years ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

@barrykooij
3 years ago

#26 @barrykooij
3 years ago

I fixed my patch so it matches latest WP coding standards. Fingers crossed we can finally get this patch in :)

This ticket was mentioned in IRC in #wordpress-dev by barrykooij. View the logs.


3 years ago

#28 @mordauk
2 years ago

Can we milestone this for 4.0? I've just come across a case where this would save so much time and headache.

#29 @samuelsidler
2 years ago

  • Keywords 3.9-early removed
  • Milestone changed from Future Release to 4.0

@nacin or @wonderboymusic: Is this good? Ready to go?

#30 @wonderboymusic
2 years ago

  • Keywords needs-refresh added

Patch blows up, whitespace needs rehabilitation as well.

@mordauk
2 years ago

#31 @mordauk
2 years ago

  • Keywords needs-refresh removed

22212.3.patch refreshed patch attached that applies cleanly.

#32 @barrykooij
2 years ago

Thanks for the refresh @mordauk

So, is this ready to go?

#33 @ramiy
2 years ago

@mordauk, you also need to update the phpDods.

#34 @DrewAPicture
2 years ago

  • Keywords needs-docs added

#35 follow-up: @mordauk
2 years ago

Any specifics on the docs that should be updated with this? I don't see any besides perhaps some inline comments.

#36 in reply to: ↑ 35 @DrewAPicture
2 years ago

  • Keywords has-unit-tests 4.1-early added; needs-docs removed
  • Milestone changed from 4.0 to Future Release

Replying to mordauk:

Any specifics on the docs that should be updated with this? I don't see any besides perhaps some inline comments.

I was thinking a notation that the type on $role had been changed to string|array, though I notice there's actually no documentation anywhere other than the Codex. Let's handle documenting the defaults over in #28298

Following commit, the Codex could use some updates though for the accepted type adjustment:

And unfortunately, it's too late for this change. Let's pick it up in 4.1 early.

#37 @mordauk
2 years ago

Thanks Drew!

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


2 years ago

#39 @johnbillion
2 years ago

  • Keywords 4.1-early removed
  • Milestone changed from Future Release to 4.1

#40 @boonebgorges
2 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from 4.1 to Future Release

Patch no longer applies cleanly. See [30094].

I would strongly prefer to see this rewritten to use WP_Meta_Query. It will avoid a lot of these horrific table joins against a potentially gargantuan wp_usermeta table.

This is also a good time to take a step back and remind ourselves of the general terribleness of doing a LIKE query against usermeta to get role information. Not a problem that has to be solved by this patch, but something for us all to think about in those quiet moments.

If we can get a refresh in time for 4.1 beta, we can move this back to the 4.1 milestone.

#41 follow-up: @knutsp
15 months ago

  • Keywords dev-feedback added

Can we move this into the 4.4 milestone?

#42 in reply to: ↑ 41 @DrewAPicture
15 months ago

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

Replying to knutsp:

Can we move this into the 4.4 milestone?

This will need a new patch based on feedback from comment:40 before 4.4 consideration.

#43 @DrewAPicture
15 months ago

  • Keywords dev-feedback removed

#44 @tifosi
15 months ago

Was considering logging an enhancement for the 'role' arg in WP_User_Query until I found this and saw that you were considering the 'role_in'argument for 4.4+.

I'm looking for the reverse where I can get all users those with a particular role, e.g. all except administrators. Useful for custom plugin user listing screens.

Would welcome also the role_not_in argument as well. Would bring it in line with WP_Query and it's _in & _not_in query parameters.

Damn editor doesn't like double underscores!

Thanks

Last edited 15 months ago by tifosi (previous) (diff)

#45 @swissspidy
14 months ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Milestone changed from Future Release to 4.4

22212.4.patch uses the already existing meta query structure and does the following:

  • Allows multiple roles to be passed for role, either as a comma-separated string or as an array. (AND relation)
  • WP_User_Query now takes a role__in array. (OR relation)
  • Also takes a role__not_in array. (AND relation)
  • Adds unit tests

It'd be great if this could be considered for 4.4.

#46 follow-up: @johnbillion
14 months ago

On both single site and multisite, users can exist without a role. The role__not_in argument could therefore be ambiguous, unless its behaviour is strictly defined. Does (and should) a query that uses role__not_in return users without a role? We'll need tests to cover this.

#48 in reply to: ↑ 46 @swissspidy
14 months ago

Replying to johnbillion:

Does (and should) a query that uses role__not_in return users without a role? We'll need tests to cover this.

It currently does, and IMHO also should. If you want users without a specific role then it doesn't matter if they have another role or none at all. I just added a test for this in 22212.5.patch.

#49 @boonebgorges
14 months ago

  • Owner set to boonebgorges
  • Status changed from reopened to assigned

swissspidy - Thanks for the dynamite patch, and especially for the tests <3

If you want users without a specific role then it doesn't matter if they have another role or none at all.

Agreed.

I think this looks good to go. One fix I'm going to make is that you're nesting one level too deep when appending $role__in_query.

#50 @boonebgorges
14 months ago

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

In 34875:

Improve role-related arguments in WP_User_Query.

  • 'role' now accepts an array or comma-separated list of role names. When passing multiple values for 'role', WP_User_Query will only match users that have all of the specified roles.
  • 'rolein' accepts an array of role names, and allow the filtering of matched users to those with at least one of the specified roles.
  • 'rolenot_in' accepts an array of role names, and allows the filtering of matched users to those who have none of the specified roles.

Props swissspidy, mordauk, barrykooij, sirbrillig.
Fixes #22212.

#51 @Otto42
14 months ago

  • Priority changed from normal to highest omg bbq
  • Resolution fixed deleted
  • Severity changed from normal to blocker
  • Status changed from closed to reopened

[34875] just took WordPress.org down because of the breaking change to how the meta query is built on multisite.

The previous version of this function added the query piece for the meta key = blog_id.capabilities, regardless of whether or not a role was requested. This limits the users returned to those with some defined role on this particular multisite instance.

Here in the old version: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-user-query.php?rev=34804#L269

The new version only sets the meta query if one is actually asked for. It never sets a query for the key by itself, only for key and value.

New version, here: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-user-query.php?rev=34875#L290

This breaks sites with extremely large numbers of users, like WordPress.org and the 8 million+ users we have.

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


14 months ago

#53 @jorbin
14 months ago

In 34880:

Revert [34875] due to unintentional breaking change

WordPress.org was taken down due to as Otto42 describes:

The previous version of this function added the query piece for the meta key = blog_id.capabilities, regardless of whether or not a role was requested. This limits the users returned to those with some defined role on this particular multisite instance.

See #22212

#54 @Otto42
14 months ago

  • Priority changed from highest omg bbq to normal
  • Severity changed from blocker to normal

#55 @ocean90
14 months ago

A query which was broken/not limited anymore:

$user_query = new WP_User_Query( array(
    'blog_id' => get_current_blog_id(),
    'fields'  => 'ID',
    'number' => 2
) );
return 1 === (int) $user_query->get_total();

SQL: SELECT SQL_CALC_FOUND_ROWS wp_users.ID FROM wp_users WHERE 1=1 ORDER BY user_login ASC LIMIT 0, 2

#56 @Otto42
14 months ago

To simplify the problem a little bit: the patch breaks the blog_id parameter. Fix that, and it's likely solved.

@boonebgorges
14 months ago

#57 @boonebgorges
14 months ago

Whoops. Sorry about the debacle.

To be more specific, the $blog_id parameter was broken only when not combined with one of the 'role' parameters. 22212.diff fixes this by doing a key-only lookup (which will match any role) when no role has been specified. There's also a unit test in there.

Feeling a tad gun-shy, so I'd appreciate a look from one of my esteemed colleagues.

@swissspidy
14 months ago

#58 @swissspidy
14 months ago

I just tested the new patch and it looks good so far. What's it with these sanity checks though?

// Sanity check: this clause may already have been added to the meta_query. 
if ( empty( $this->meta_query->clauses ) || ! in_array( $roles_clauses, $this->meta_query_clauses, true ) ) { 
    $role_queries[] = $roles_clauses; 
}
  1. This should probably be if ( empty( $this->meta_query->queries ) || ! in_array( $roles_clauses, $this->meta_query->queries, true ) ) { (clauses is protected)
  2. Either we add this check to all role clauses (see my patch) or remove it (what breaks?).
  3. What about a test for this sanity check?

#59 @thomask
14 months ago

pardon my layman question, but the wordpress.com debacle was interesting story for me. I have thought, that this should cover the unit tests, and i would have expect, that the unit test enviroment does have setup, that covers the extreme variants, so e.g. milions of posts, taxonomies, users ... so that the test can then reveal not just the boolean truth/false of the tested operation correctness, but also possible negative performance impact.
So when reading the followup and patches, i miss one more patch reqest - patch of the unit tests, so that such problem would not appear in the future.
But I am laic (just the author of this ticket), so excuse me if this my comment is completely unrelevant

#60 @boonebgorges
14 months ago

In 34956:

Store SQL query string as a property on WP_User_Query.

In addition to better parity with other WP query classes, this also allows
testing of SQL strings, should anyone want to do something so foolish.

See #22212.

#61 @boonebgorges
14 months ago

But I am laic (just the author of this ticket), so excuse me if this my comment is completely unrelevant

It's an interesting thought, but outside the scope of this ticket :)

What's it with these sanity checks though?

I was hoping we could get through this ticket without addressing the issue :-/ The check dates to [28087] #21119. When prepare_query() was made into a public method, it was found that it wasn't completely idempotent: calling $q->prepare_query() on an already-prepared query could cause additional processing. The only problem that was noted in #21119 was the cap-check meta_query, so [28087] was kind of a slap-dash way of taking care of it. At some point, the bug was organically refactored out of WP_User_Query, so the in_array() check stopped doing anything. I knew this backstory when going into the current ticket, but decided to ignore it and leave the check rather than unravel the history of WordPress in order to verify that it could be removed :) But I have now done the unraveling, and I've determined that the check is no longer necessary, and the commit will contain a unit test that demonstrates this fact.

#62 @boonebgorges
14 months ago

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

In 34959:

WP_User_Query role improvement redux.

It's back, and it's better than ever: an overhaul of role-related arguments
in WP_User_Query. This updated version of the previously-reverted [34875]
includes support for the use of $blog_id without specifying a $role, for
a 99.7% reduced chance of breaking wordpress.org and other large sites.

Props boonebgorges, swissspidy.
Fixes #22212.

@ocean90
14 months ago

#63 @ocean90
14 months ago

  • Resolution fixed deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

[34959] introduced a bug with similiar role names, like "editor" and "foo-editor". That's because the quotes for the role value are missing.

Query before [34959]:

SELECT wptests_users.* FROM wptests_users INNER JOIN wptests_usermeta ON ( wptests_users.ID = wptests_usermeta.user_id ) WHERE 1=1 AND (
  ( wptests_usermeta.meta_key = 'wptests_capabilities' AND CAST(wptests_usermeta.meta_value AS CHAR) LIKE '%\"editor\"%' )
) ORDER BY user_login ASC

Query after [34959]:

SELECT wptests_users.* FROM wptests_users INNER JOIN wptests_usermeta ON ( wptests_users.ID = wptests_usermeta.user_id ) WHERE 1=1 AND (
  (
    ( wptests_usermeta.meta_key = 'wptests_capabilities' AND CAST(wptests_usermeta.meta_value AS CHAR) LIKE '%editor%' )
  )
) ORDER BY user_login ASC

22212.3.diff includes a test and the fix.

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


14 months ago

#65 @boonebgorges
14 months ago

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

In 35101:

Don't match partial roles in WP_User_Query.

Because 'role=ocean90' shouldn't match 'role=bocean901'.

Props bocean901, ocean90.
Fixes #22212.

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


12 months ago

Note: See TracTickets for help on using tickets.