Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#49118 closed enhancement (fixed)

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

Reported by: pbiron's profile pbiron Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.6
Component: Users Keywords: has-patch commit
Focuses: rest-api Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (1)

49118.diff (988 bytes) - added by pbiron 5 years ago.

Download all attachments as: .zip

Change History (10)

5 years ago

#1 @pbiron
5 years ago

  • Keywords has-patch added

#2 @TimothyBlynJacobs
5 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 5.4
  • Version set to 4.9.6

Patch looks good to me. Thanks @pbiron!

#3 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#4 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#6 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#7 follow-up: @pbiron
5 years ago

@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.

#8 in reply to: ↑ 7 @SergeyBiryukov
5 years ago

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:

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.

#9 @pbiron
5 years ago

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 :-)

Note: See TracTickets for help on using tickets.