WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 6 months ago

#22212 reopened enhancement

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

Reported by: thomask Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Users Keywords: has-unit-tests needs-refresh
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 (7)

query.php.patch (842 bytes) - added by barrykooij 2 years ago.
Unit Test to test user.php.patch
user.php.patch (1.6 KB) - added by barrykooij 2 years ago.
user.php.2.patch (2.1 KB) - added by barrykooij 2 years ago.
22212.patch (5.6 KB) - added by barrykooij 19 months ago.
user.php & unit tests
23756.diff (3.9 KB) - added by sirbrillig 18 months ago.
Re-added @ to ticket number in test.
22212.2.patch (6.6 KB) - added by barrykooij 13 months ago.
22212.3.patch (6.7 KB) - added by mordauk 10 months ago.

Download all attachments as: .zip

Change History (47)

comment:1 @knutsp3 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.

comment:2 @scribu3 years ago

needs-patch indeed.

comment:3 @greenshady3 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.

comment:4 @ramiy2 years ago

  • Cc r_a_m_i@… added

comment:5 @SergeyBiryukov2 years ago

#23258 was marked as a duplicate.

comment:7 @helgatheviking2 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.

comment:8 @barrykooij2 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.

comment:9 @barrykooij2 years ago

  • Keywords needs-testing added

comment:10 follow-up: @ocean902 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.

comment:11 in reply to: ↑ 10 @barrykooij2 years ago

Replying to ocean90:

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

I'm sorry!

@barrykooij2 years ago

Unit Test to test user.php.patch

comment:12 @markoheijnen2 years ago

  • Keywords dev-feedback added; needs-testing removed

@barrykooij2 years ago

comment:13 @scribu2 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 2 years ago by scribu (previous) (diff)

@barrykooij2 years ago

comment:14 @barrykooij2 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 2 years ago by barrykooij (previous) (diff)

comment:15 @alex-ye2 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 .

comment:16 @markoheijnen2 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.

comment:17 @markoheijnen20 months ago

  • Milestone changed from Awaiting Review to 3.7

I would love to tackle this in 3.7

comment:18 @nacin20 months 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. :-)

comment:19 @nacin19 months 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.

@barrykooij19 months ago

user.php & unit tests

comment:20 @barrykooij19 months 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.

comment:21 @nacin19 months ago

  • Milestone changed from Future Release to 3.8

comment:22 @nacin19 months 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.

@sirbrillig18 months ago

Re-added @ to ticket number in test.

comment:23 @sirbrillig18 months ago

Argh. Please ignore that last patch. Wrong ticket.

comment:24 @barrykooij17 months 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.

comment:25 @wonderboymusic17 months ago

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

@barrykooij13 months ago

comment:26 @barrykooij13 months ago

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

comment:27 @ircbot13 months ago

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

comment:28 @mordauk10 months ago

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

comment:29 @samuelsidler10 months ago

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

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

comment:30 @wonderboymusic10 months ago

  • Keywords needs-refresh added

Patch blows up, whitespace needs rehabilitation as well.

@mordauk10 months ago

comment:31 @mordauk10 months ago

  • Keywords needs-refresh removed

22212.3.patch refreshed patch attached that applies cleanly.

comment:32 @barrykooij10 months ago

Thanks for the refresh @mordauk

So, is this ready to go?

comment:33 @ramiy10 months ago

@mordauk, you also need to update the phpDods.

comment:34 @DrewAPicture10 months ago

  • Keywords needs-docs added

comment:35 follow-up: @mordauk9 months ago

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

comment:36 in reply to: ↑ 35 @DrewAPicture9 months 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.

comment:37 @mordauk9 months ago

Thanks Drew!

comment:38 @slackbot6 months ago

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

comment:39 @johnbillion6 months ago

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

comment:40 @boonebgorges6 months 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.

Note: See TracTickets for help on using tickets.