Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#16841: Capability Queries #286

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@swissspidy
Copy link
Collaborator

commented Oct 16, 2017

This PR adds capability options to WP_User_Query.

It leverages the previous work we've done on querying the capability meta fields, so it doesn't contain too much magic sauce.

Basically, when querying for capabilities, it checks which roles do have these caps and queries for the roles instead. Plus, it queries for the capability as well in case it was added directly to a user.

To do:

  • More tests, ideally those that match the ones from #22212.
  • Add support for these queries in wp_dropdown_users().
    See #38135 on Trac for a precedent.
  • Add tests for the current behaviour of who queries in wp_dropdown_users().
  • Figure out how/if passing an array for capability could work.
  • Multisite: Use $wp_roles->for_site( $blog_id ) to make sure the roles are fetched from the right site. See [41625]
  • Multisite: Add tests accordingly.

See #16841 on Trac for more information.

swissspidy added some commits Oct 16, 2017

@boonebgorges
Copy link
Collaborator

left a comment

General technique gets a +1 from me - as you note, it relies on the awful role logic, but doesn't make it any worse. I'd think about modifying the logic so that it's possible to do an AND query, as I suggest in my comment above.

$capabilities = array();
if ( isset( $qv['capability'] ) ) {
// Todo: Add support for arrays? How should these queries be resolved?

This comment has been minimized.

Copy link
@boonebgorges

boonebgorges Jan 31, 2018

Collaborator

The way role is handled, vs role__in, might be helpful to look at. role can be an array, and is handled as an AND list: role=foo,bar means only users who have both the foo and bar role will be returned. role__in is obviously an IN / OR check. I think it's like this for legacy reasons, though it might be worth emulating for consistency. Additionally, while it's not super useful to be able to match multiple Roles with an AND, it's potentially more relevant for caps: show me all users who have manage_users AND manage_options, or whatever.

@felixarntz

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2018

No additonal comments from me except from the multisite support regarding roles possibly need to be switched temporarily before retrieving them.

I like the approach, I definitely agree that thorough tests are needed. Those will uncover possible issues in the logic, but so far I haven't been able to detect any problems. Happy to have another look related to tests in the near future.

@swissspidy swissspidy self-assigned this Apr 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.