WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 2 weeks 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-patch 3.9-early
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 (6)

query.php.patch (842 bytes) - added by barrykooij 13 months ago.
Unit Test to test user.php.patch
user.php.patch (1.6 KB) - added by barrykooij 13 months ago.
user.php.2.patch (2.1 KB) - added by barrykooij 13 months ago.
22212.patch (5.6 KB) - added by barrykooij 7 months ago.
user.php & unit tests
23756.diff (3.9 KB) - added by sirbrillig 6 months ago.
Re-added @ to ticket number in test.
22212.2.patch (6.6 KB) - added by barrykooij 5 weeks ago.

Download all attachments as: .zip

Change History (33)

comment:1 knutsp18 months 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 scribu18 months ago

needs-patch indeed.

comment:3 greenshady18 months 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 ramiy17 months ago

  • Cc r_a_m_i@… added

comment:5 SergeyBiryukov15 months ago

#23258 was marked as a duplicate.

comment:7 helgatheviking15 months 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 barrykooij13 months 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 barrykooij13 months ago

  • Keywords needs-testing added

comment:10 follow-up: ocean9013 months 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 barrykooij13 months ago

Replying to ocean90:

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

I'm sorry!

barrykooij13 months ago

Unit Test to test user.php.patch

comment:12 markoheijnen13 months ago

  • Keywords dev-feedback added; needs-testing removed

barrykooij13 months ago

comment:13 scribu13 months 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 13 months ago by scribu (previous) (diff)

barrykooij13 months ago

comment:14 barrykooij13 months 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 13 months ago by barrykooij (previous) (diff)

comment:15 alex-ye13 months 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 markoheijnen13 months 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 markoheijnen8 months ago

  • Milestone changed from Awaiting Review to 3.7

I would love to tackle this in 3.7

comment:18 nacin8 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 nacin7 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.

barrykooij7 months ago

user.php & unit tests

comment:20 barrykooij7 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 nacin7 months ago

  • Milestone changed from Future Release to 3.8

comment:22 nacin7 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.

sirbrillig6 months ago

Re-added @ to ticket number in test.

comment:23 sirbrillig6 months ago

Argh. Please ignore that last patch. Wrong ticket.

comment:24 barrykooij5 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 wonderboymusic5 months ago

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

barrykooij5 weeks ago

comment:26 barrykooij5 weeks ago

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

comment:27 ircbot2 weeks ago

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

Note: See TracTickets for help on using tickets.