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

[thomask]

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.

[knutsp]

+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.

[scribu]

needs-patch indeed.

[greenshady]

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

[SergeyBiryukov]

#23258 was marked as a duplicate.

[SergeyBiryukov]

Related: #23045

[helgatheviking]

+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.

[barrykooij]

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.

[ocean90]

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

[barrykooij]

Replying to ocean90:

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

I'm sorry!

[barrykooij]

Unit Test to test user.php.patch

[scribu]

$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.

[barrykooij]

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.

[alex-ye]

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 .

[markoheijnen]

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.

[markoheijnen]

I would love to tackle this in 3.7

[nacin]

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. :-)

[nacin]

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]

user.php & unit tests

[barrykooij]

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.

[nacin]

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]

Re-added @ to ticket number in test.

[sirbrillig]

Argh. Please ignore that last patch. Wrong ticket.

[barrykooij]

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.

[barrykooij]

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

[mordauk]

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

[samuelsidler]

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

[wonderboymusic]

Patch blows up, whitespace needs rehabilitation as well.

[mordauk]

22212.3.patch refreshed patch attached that applies cleanly.

[barrykooij]

Thanks for the refresh @mordauk

So, is this ready to go?

[ramiy]

@mordauk, you also need to update the phpDods.

[mordauk]

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

[DrewAPicture]

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:

• ​https://codex.wordpress.org/Class_Reference/WP_User_Query
• ​https://codex.wordpress.org/Function_Reference/get_users

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

[mordauk]

Thanks Drew!

[boonebgorges]

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.

[knutsp]

Can we move this into the 4.4 milestone?

[DrewAPicture]

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.

[tifosi]

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

[swissspidy]

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.

[johnbillion]

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.

[johnbillion]

Related: #22993

[swissspidy]

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​.

[boonebgorges]

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.

[boonebgorges]

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.

[Otto42]

[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.

[jorbin]

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

[ocean90]

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

[Otto42]

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

[boonebgorges]

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]

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?

[thomask]

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

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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]

[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.

[boonebgorges]

In 35101:

Don't match partial roles in WP_User_Query.

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

Props bocean901, ocean90.
Fixes #22212.