Repository navigation
Abilities API: Add core/users-query ability - #10775
jorgefilipecosta wants to merge 46 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
(Sorry I missed this one. Wish there was a way we could actually tag other #core-ai folks for review. Will take it for a spin hopefully tomorrow or wednesday) |
0756a9a to
37e01db
Compare
|
This PR was rebases and the conflicts were fixed. cc: @justlevine |
| 'include_capabilities' => array( | ||
| 'type' => 'boolean', | ||
| 'description' => __( 'Whether to include the user capabilities in the response.' ), | ||
| 'default' => false, | ||
| ), |
There was a problem hiding this comment.
Why do we need include_capabilities at all?
If there's reason not to get rid of it entirely then
- It should probably be on a different hierarchical level than the
oneOffor our ID types. - It should probably be done more generically so we can grow this to handle other properties. e.g.
fields: string[]
There was a problem hiding this comment.
Hi @justlevine,
Why do we need include_capabilities at all?
You mean you prefer that we include the capabilities by default all the time? The downside of that is that an agent may not need capability information and returning it may just be wasting tokens,
It should probably be done more generically so we can grow this to handle other properties. e.g. fields: string[]
Although fields[] is versatile it forces an agent to think about what fields it even needs. On abilities context something like optional_fields[] where we only specify optional fields may make more sense, what do you think?
There was a problem hiding this comment.
Hi @justlevine,
Why do we need include_capabilities at all?
You mean you prefer that we include the capabilities by default all the time? The downside of that is that an agent may not need capability information and returning it may just be wasting tokens,
I mean if we aren't sure the holistic way to query it
It should probably be done more generically so we can grow this to handle other properties. e.g. fields: string[]
Although fields[] is versatile it forces an agent to think about what fields it even needs. On abilities context something like optional_fields[] where we only specify optional fields may make more sense, what do you think?
Sorry let me be more clear - the suggestion of fields here was just meant to parallel how REST handles lazy-loading. I'm not sure an agent needing to request its fields is any different than it needed to intuit any other args - except that because of the schema it doesn't even need to slurp docs or source. Wouldn't suggest exclude_fields: string[] because we follow the principle of least privilege, but i'm good with whatever holistic strategy that can encompass fields that don't natively live on WP_User.
I don't know what an optional_fields[] means in this context, so I'm assuming an llm will struggle to intuit this too. What constitutes whether a field is optional? It sounds like an api implementation detail, while a holistic schema imo should focus on usage and semantics.
There was a problem hiding this comment.
Maybe I'm overthinking and fields[] is ok. I guess we can give a try to include capabilities by default, and have an optional fields[] offering granularity. Would you be ok with that approach?
On an unrelated non-blocking question but to try to understand your vision regarding encompass fields that don't natively live on WP_User. For now my thinking is that the fields are fixed, any other information besides the fields WordPress offers by default would be on user meta, if not possible to cover by user meta, it would be covered by a custom ability something like core/user/get-total-order-ammunt, or core/get-user-total-order-ammount. What use cases do you think should be considered for expanding the fields a user has? (not blocking since all options are open).
There was a problem hiding this comment.
On an unrelated non-blocking question but to try to understand your vision regarding encompass fields that don't natively live on WP_User.
Not an unrelated note lol. That's basically the underlying question I've been poorly trying to clarify from you 😭.
Capabilities doesnt live on WP_User, neither does locale etc. But this PR is including those in the input/output schemas but not other metadata. (Input schema is more disruptive to future-compat - is why I'm focusing more on include_capabilities but the question's about everything that isnt more than a normalizer / hook wrapper around direct WP_User::$prop ). If there's intentional reason that sets them apart from metadata, then I want to know what it is so the schema can reflect that intuitively. If there isnt, then we want to make sure our approach scales to other metadata - and if we're unsure and want to do things iteratively, then reducing down to WP_User is the most basic guaranteed delineation.
For now my thinking is that the fields are fixed, any other information besides the fields WordPress offers by default would be on user meta
Just to confirm, this would mean removing everything that's not on WP_User currently, like capabilities (?). If so then yeah, thats the safest change we can make.
Or are all of these extra fields like caps in that theyre neither WP_User props nor meta? If the latter, I'd want to make sure that we've thought how both the input and output schemas look alongside the WP_User props and metadata.{metadata_key}:{metadata_schema} || {infer/cast from type like in settings}.
I'm also assuming that posts/terms/etc have similiar intermediary data alongside their meta for a quick gut check.
if not possible to cover by user meta, it would be covered by a custom ability something like core/user/get-total-order-ammunt, or core/get-user-total-order-ammount.
We're in alignment here.
There was a problem hiding this comment.
I opted to not include capabilities as they are very connected to meta, let's try to get the minimum viable ability ready first to avoid increasing the PR size.
| private static function get_user_output_schema(): array { | ||
| return array( | ||
| 'type' => 'object', | ||
| 'required' => array( |
There was a problem hiding this comment.
What makes these "required"? It's clearly not a promise of content since we're not coercing empty values at all...
There was a problem hiding this comment.
It means the keys will always be on the returned object even if they may be empty. But maybe we don't need to make them required. I guess we could consider only id and username as required. Will do an update.
There was a problem hiding this comment.
Is that helpful on output schema?
Input I understand, if it's required then a consumer minimally needs to an explicit null, but is knowing a key exists but may be null|undefined|empty-string| beneficial?
I could see the benefit of using required to choreograph if an output field will always have a value (like id/username) whereas fields that are coerced to ( (cast-type ) $value ) ?: null are "optional".
( Otherwise, then we should put all our output keys as required and let it be a guarantee that the prop exists. Don't see any Core use case of optional if it's just about us guaranteeing key presence)
There was a problem hiding this comment.
The more complete the schema is the better, in this case we know any valid user must have at least an id and username, so we should provide that information in the schema.
There was a problem hiding this comment.
Works for me. If we want to promise that then we need to enforce it though and not just that a {key} is present
|
My biggest concern with this PR is that it's not automatically scalable or extendable. It's not obvious (to me or an LLM) why these are the particular set of user data we're returning, we're not exposing user meta that's registered with |
Hi @justlevine the plan is to expose user meta. The only reason we are not doing it yet is because I learned the lesson on #10867 😅 and PR's rapidly become very huge and hard to review if we include the full features from the beginning. So the plan is to merge a simple user ability first, and then follow up right away with the meta expansion. |
100% agree. The nuance here is that "simple" means something a bit different for abilities than traditional progressive programming, since it's the schema that needs to scale, and we have a lot more leeway with the internals. My focus isn't why not metadata, but why yes to e.g. Like @/aaronjorbin notes in https://core.trac.wordpress.org/ticket/64596#comment:9 , once things are in core back and forward compat take on a mind on its own, so we need to make sure we can iterate the schema in the holistic direction we want, like how we intentionally prepped for the possibility of nested namespaces |
29c36cc to
4312b3f
Compare
|
This question came up tangentially elsewhere - I couldn't find a trac ticket for this ability , so sharing it here for completeness: what are we doing with the existing
|
9963eaa to
a8218c3
Compare
a8218c3 to
e846a9c
Compare
Use the core helpers the class wrapped: rest_sanitize_object() for the input of every callback, get_user_by() for every lookup type, wp_parse_id_list() for include, absint() for integers, and the paged and count_total handling of WP_User_Query. Inline the one-line membership, default fields and role name helpers, and drop the casts of WP_User::$ID, which is always an integer. get_user_by() takes the lookup value as a string, because validation also accepts a float ID, like 5.0. format_user() always includes id, so the fallback for an empty result is removed. Roles are listed with array_values(), as the REST users controller does. Ported from WordPress/wordpress-develop#10775.
Start the label with a verb, like the other core abilities, such as "Get Site Information" and "Get User Information".
…query`. Without `per_page`, an `include` request used the default page size of 10, so a caller loading a known set of users silently received only the first 10. Page such a request to the number of included IDs instead, and cap `include` at the maximum page size so the IDs always fit on one page. An explicit `per_page` still wins. This matches `core/content-query`.
The note said that leaving the include order out of `orderby` lets WP_User_Query share cached results with other queries. It does not: WP_User_Query keys its cache on the SQL, and `ID IN (...)` lists the IDs in the order given, so lists in a different order never share an entry either way. Say what the code does instead: like the REST users controller, the include list filters the results without ordering them. Also say so in the `include` description, and fix the test docblock that repeated the note.
…s-query`. Together, rest_is_boolean() and rest_sanitize_boolean() match exactly the forms of `true` that the hand-written check matched: `true`, `1`, and the strings `'1'` and `'true'` in any case. Schema validation reads booleans with the same helpers.
…ry`. Parse the value with wp_parse_list() and keep only its strings, like the list parser of `core/content-query`. The loop and array_unique() that dropped empty and duplicate items are not needed, because schema validation has already rejected them.
WordPress/wordpress-develop#10775 now labels the ability "Query Users" too, so the label no longer differs from core.
Without `per_page`, an `include` request used the default page size of 10, so a caller loading a known set of users silently received only the first 10. Page such a request to the number of included IDs instead, and cap `include` at the maximum page size so the IDs always fit on one page. An explicit `per_page` still wins. Ported from WordPress/wordpress-develop#10775.
The note said that leaving the include order out of `orderby` lets WP_User_Query share cached results with other queries. It does not: WP_User_Query keys its cache on the SQL, and `ID IN (...)` lists the IDs in the order given, so lists in a different order never share an entry either way. Say what the code does instead: like the REST users controller, the include list filters the results without ordering them. Also say so in the `include` description, and fix the test docblock that repeated the note. Ported from WordPress/wordpress-develop#10775.
WordPress/wordpress-develop#10775 now reads the forms of `true` with rest_is_boolean() and rest_sanitize_boolean(). For a mixed value, PHPStan cannot resolve the template type that the WordPress stubs give rest_sanitize_boolean(), so the plugin keeps listing the same forms by hand. Mark the difference.
Parse the value with wp_parse_list() and keep only its strings. The loop and array_unique() that dropped empty and duplicate items are not needed, because schema validation has already rejected them. Ported from WordPress/wordpress-develop#10775.
gziolo
left a comment
There was a problem hiding this comment.
I compared this PR with core/content-query (#12195). Content-query returns author_slug, and agents will likely pass it here as slug. I left a few inline notes where the two abilities behave differently. I'm not sure what the reasoning is on each side, so these are questions rather than requests.
Treat `fields: []` like an omitted `fields` argument and return the default fields, as `normalize_fields()` already did, instead of rejecting the list in the input schema. `core/content-query` accepts an empty list too, so clients can use one convention for both abilities. Also say in the `fields` description that `id` is always included, and that fields the current user cannot view are omitted rather than causing an error.
A single-user lookup that the execute callback could not resolve returned `ability_invalid_permissions` without an HTTP status. Return `users_not_found` with a 404 instead, like the `content_not_found` error of `core/content-query`. Gated transports still never reach it, because the permission callback denies the same lookups first. A user the current user cannot read is reported like a missing one. Report a page past the last one as `users_invalid_page_number` with a 400, like `core/content-query` and the REST posts controller, instead of returning an empty list. An empty result set still returns zero totals on any page.
On multisite, a super admin can publish posts on a site without being a member of it. The posts show them as an author on the front end, and `core/content-query` returns their `author_slug`, but a lookup of that slug was refused, because every lookup of another user required membership of the site. Only require membership for the capability checks, which should reveal users of the site, not of the whole network. Lookups by ID or slug now find public authors who are not members, with the fields of a public author. Lookups by email or username still only find users of the site.
`core/content-query` lets a user who can edit others' posts resolve any user of the site as an `author_slug`, since they can make any of them the author of a post. A lookup of that slug here was refused when the user had no public posts, such as an author whose only post is pending review. Let a caller who can edit others' posts of a post type that supports authors look up any user of the site by ID or slug, and return only the fields that identify the user: `id`, `name`, `link`, `slug`, and `avatar_urls`. The profile fields, `description` and `url`, and the sensitive fields are left out, and their descriptions now say they are present when the current user can view them. Lookups by email or username still require permission to list or edit users, and collections still only list public authors to callers who cannot list users. The permission and execute callbacks share the decision through `get_readable_fields()`, which replaces `can_read_user_for_lookup()`.
Treat `fields: []` like an omitted `fields` argument and return the default fields, as normalize_fields() already did, instead of rejecting the list in the input schema. `core/content-query` accepts an empty list too, so clients can use one convention for both abilities. The create, update, and delete abilities share the `fields` schema, so they accept an empty list too. Also say in the `fields` descriptions that `id` is always included, and that fields the current user cannot view are omitted rather than causing an error. Ported from WordPress/wordpress-develop#10775.
A single-user lookup that the execute callback could not resolve returned `ability_invalid_permissions` without an HTTP status. Return `users_not_found` with a 404 instead, like the `content_not_found` error of `core/content-query`. Gated transports still never reach it, because the permission callback denies the same lookups first. A user the current user cannot read is reported like a missing one. The update and delete abilities give the same error for a user who does not exist or does not belong to the site, so they return `users_not_found` too. Report a page past the last one as `users_invalid_page_number` with a 400, like `core/content-query` and the REST posts controller, instead of returning an empty list. An empty result set still returns zero totals on any page. Ported from WordPress/wordpress-develop#10775.
On multisite, a super admin can publish posts on a site without being a member of it. The posts show them as an author on the front end, and `core/content-query` returns their `author_slug`, but a lookup of that slug was refused, because every lookup of another user required membership of the site. Only require membership for the capability checks, which should reveal users of the site, not of the whole network. Lookups by ID or slug now find public authors who are not members, with the fields of a public author. Lookups by email or username still only find users of the site. Ported from WordPress/wordpress-develop#10775.
`core/content-query` lets a user who can edit others' posts resolve any user of the site as an `author_slug`, since they can make any of them the author of a post. A lookup of that slug here was refused when the user had no public posts, such as an author whose only post is pending review. Let a caller who can edit others' posts of a post type that supports authors look up any user of the site by ID or slug, and return only the fields that identify the user: `id`, `name`, `link`, `slug`, and `avatar_urls`. The profile fields, `description` and `url`, and the sensitive fields are left out, and their descriptions now say they are present when the current user can view them. Lookups by email or username still require permission to list or edit users, and collections still only list public authors to callers who cannot list users. The permission and execute callbacks share the decision through get_readable_fields(), which replaces can_read_user_for_lookup(), and find_user() replaces resolve_readable_user(). Ported from WordPress/wordpress-develop#10775.
core/users-query abilitycore/users-query ability
- Report a page past the last one as not found (404) instead of as a caller error. - Fail closed on collection filters that cannot be honored instead of dropping them, which silently widened the query: an `include` list with no valid ID, an empty `roles` list, and a `has_published_posts` value that is neither true nor a list of post types are rejected, and a role filter is refused to a caller who cannot list users. - Read `id`, `page`, and `per_page` with the content query's integer parser, so a fraction or a value beyond 2 ** 53 cannot be cast onto another user, and paging that cannot be parsed falls back to the defaults. - Default the input schema to an empty array, like the other core abilities, and say in the description that the ability requires an authenticated user. - Tests: set up the fixtures in wpSetUpBeforeClass() and clean up in wpTearDownAfterClass(), and cover the rejected filters and the pagination fallback.
gziolo
left a comment
There was a problem hiding this comment.
All my feedback is addressed. The permission handling, field-selection behavior, and errors now align with the agreed contracts, with regression coverage and green CI. Ready to land from my side.
…-info`. `core/get-user-info` only returns the current user's own profile, while `core/users-query` reads any user the current user is allowed to see. Note this on `register_users_query()`, so the two abilities are not mistaken for duplicates.
Only `register()` stays public. The permission and execute callbacks are now private methods, registered through closures defined in the class, so callers run the ability through the Abilities API, such as `wp_get_ability( 'core/users-query' )->execute()`, which validates the input and checks permissions first. This replaces the class docblock's note that the class is not part of the public API. Public methods are covered by Core's backward compatibility commitment whatever the docblock says. Tests that call the callbacks directly now capture them from the registration arguments, through the `wp_register_ability_args` filter.
…nt query - Map the filter to the message under `params` in the `users_invalid_filter` error data, as the REST API's `rest_invalid_param` errors do, so callers can tell which filter failed without parsing the translated message. - Mark `id`, which is always returned, as required in the user output schema.
`core/get-user-info` only returns the current user's own profile, while `core/users-query` reads any user the current user is allowed to see. Note this on register_users_query(), so the two abilities are not mistaken for duplicates. Ported from WordPress/wordpress-develop#10775.
Only init() and register() stay public. The permission and execute callbacks of `core/users-query`, `core/user-create`, `core/user-update`, and `core/user-delete` are now private methods, registered through closures defined in the class, so callers run the abilities through the Abilities API, such as `wp_get_ability( 'core/users-query' )->execute()`, which validates the input and checks permissions first. Tests that call the callbacks directly now capture them from the registration arguments, through the `wp_register_ability_args` filter. Ported from WordPress/wordpress-develop#10775.
What?
core/users-queryability registered through Core ability registration.id,email,username, orslug; a successful lookup returns the user object directly.roles,has_published_posts,include,fields,page, andper_page; collection responses containusers,total, andtotal_pages.id,name,description,url,link,slug,avatar_urls,username,email,first_name,last_name,nickname,locale,registered_date, androles.id, and omits fields that the caller cannot view.Ticket: https://core.trac.wordpress.org/ticket/64657
Testing
php -l src/wp-includes/abilities.phpphp -l src/wp-includes/abilities/class-wp-abilities-users.phpphp -l tests/phpunit/tests/abilities-api/wpRegisterCoreUsersAbility.phpcomposer lint -- src/wp-includes/abilities/class-wp-abilities-users.php tests/phpunit/tests/abilities-api/wpRegisterCoreUsersAbility.phpcomposer phpstan -- src/wp-includes/abilities/class-wp-abilities-users.phpphp ./vendor/bin/phpunit --filter Tests_Abilities_API_WpRegisterCoreUsersAbilityphp ./vendor/bin/phpunit -c tests/phpunit/multisite.xml --filter Tests_Abilities_API_WpRegisterCoreUsersAbilityphp ./vendor/bin/phpunit --group abilities-apiphp ./vendor/bin/phpunit -c tests/phpunit/multisite.xml --group abilities-apigit diff --check