REST API: minor performance improvement for /wp/v2/users?who=authors

[pbiron]

Related: #42202

Changeset [43001]/[43137] added a who=authors parameter to the list users endpoint.

When that parameter is present, WP_REST_Users_Controller::get_items_permissions_check() checks that the current user has the capability to edit_posts for at least 1 post type that has show_in_rest = true and supports author. That check loops through all post types with show_in_rest = true.

A minor performance improvement could be had by breaking out of that loop as soon one post type is found that meets the requirements.

[TimothyBlynJacobs]

Patch looks good to me. Thanks @pbiron!

[SergeyBiryukov]

In 47034:

REST API: Synchronize permission checks in ::get_items_permissions_check() methods for post types, post statuses, and users:

• Only query post types with 'show_in_rest' => true instead of looping over all post types and checking the show_in_rest property separately.
• Return from the foreach() loop as soon as the permission check succeeded.

Props pbiron, TimothyBlynJacobs, SergeyBiryukov.
Fixes #49118.

[pbiron]

@SergeyBiryukov One comment about the change you made to immediately return instead of just breading out the loop: that might make it harder to add other checks to WP_REST_Users_Controller::get_items_permissions_check() in the future.

The basic pattern of all those checks seems to be: early return (with a WP_Error) when the permission check fails and do a single "success" return at the end.

The change you made kind of breaks that pattern...and might make it harder for a contributor submitting a future patch that adds a new check: they'd have to realize that any new checks need to go before the 'authors' === $request['who' check.

Of course, that's something that a committer would need to check before committing such future patches, but I'm thinking it would be better to make it easier for contributors to not have to worry about noticing the early "success" return.

Just a thought.

[SergeyBiryukov]

Replying to pbiron:

The basic pattern of all those checks seems to be: early return (with a WP_Error) when the permission check fails and do a single "success" return at the end.

Thanks for bringing this up! That was my initial thought as well, but after a closer look it turned out that a return true inside a foreach() loop is actually more common in the existing code:

• WP_REST_Post_Statuses_Controller::get_items_permissions_check()​
• WP_REST_Post_Statuses_Controller::check_read_permission()​
• WP_REST_Post_Types_Controller::get_items_permissions_check()​
• WP_REST_Posts_Controller::check_assign_terms_permission()​
• WP_REST_Taxonomies_Controller::get_items_permissions_check()​

I went back and forth for a bit, as I also find a single "success" return at the end more readable, but ultimately chose to be consistent with most of the existing checks, since refactoring them seemed out of scope for this ticket.

[pbiron]

That's fine by me. The users controller is the only one I've looked at in any detail and didn't realize the pattern in the others was different.

Also agree that refactoring the similar methods in the other controllers is out-of-scope for this ticket. I'll leave it to some other enterprising soul to maybe open another ticket on that :-)