Short-circuit HEAD methods in Core controllers

[TimothyBlynJacobs]

The REST API has built-in support for responding to a HEAD request. If no callback is specifically registered for the method, the server will fallback to the associated GET handler. However, this means that the server is throwing away work when no response body is needed.

GET routes can be adapted to check if the WP_REST_Request::get_method is HEAD and if so skip preparing the response body. For single item routes, this is pretty straightforward.

It is a little more nebulous for collection routes. We still need to provide any headers like pagination which are currently prepared after the response body is generated.

I think we'd want to extract the pagination logic to it's own method. Then, before making the database query,
adjust the query to only return ids. This will give us enough information to build the pagination headers and allow us to send the HEAD response.

See ​https://github.com/WordPress/gutenberg/pull/43703

[antonvlasenko]

I’m working on a patch to explore implementing this for both single and collection routes. If it’s challenging to do both, I’ll start with single routes first, as this should provide better insight into how to implement it for collection routes later.

Update (19/8): I will be unable to work on this issue this week, but I will resume next week.

U̶p̶d̶a̶t̶e̶ (̶2̶8̶/8̶)̶:̶ r̶e̶s̶u̶m̶e̶d̶ w̶o̶r̶k̶i̶n̶g̶ o̶n̶ t̶h̶i̶s̶ i̶s̶s̶u̶e̶.
Update (2/9): resumed working on this issue.

[Mamaduka]

Thanks for the updates, @antonvlasenko!

Let me know if I can help with this in any way.

[antonvlasenko]

Let me know if I can help with this in any way.

@Mamaduka, that's very kind of you. Thanks!
I think you can help with testing and code reviews once the first PR is ready.
I'm currently working on the posts controller.

Trac ticket: https://core.trac.wordpress.org/ticket/56481

[antonvlasenko]

I've just submitted a first PR to address the issue for the WP_REST_Posts_Controller controller: ​https://github.com/WordPress/wordpress-develop/pull/7349.

I think we'd want to extract the pagination logic to it's own method.

For the WP_REST_Posts_Controller, the adjustments required for HEAD requests compared to standard GET requests are minimal, so I found it unnecessary to extract the pagination logic into a separate method.

As suggested by @TimothyBlynJacobs, the proposed PR optimizes performance by adjusting the query to return only ids. If the direction of my PR is considered correct, I am prepared to make similar adjustments for other controllers in future PRs.

My PR also adds several unit tests for the HEAD method to ensure that the WP_REST_Posts_Controller functions correctly.

Please note that I will be unavailable until Sep 23, but I can resume work on subsequent PRs upon my return.

Trac ticket: https://core.trac.wordpress.org/ticket/56481

Trac ticket: https://core.trac.wordpress.org/ticket/56481

[antonvlasenko]

I'd like to share the performance improvement data I measured after applying the three submitted PRs to the relevant controllers:

• ​Posts controller: 122ms improvement (100 posts per page);
• ​Comments controller: 179ms improvement (100 comments per page);
• ​Terms controller: 124ms improvement (100 categories per page);

@TimothyBlynJacobs, any feedback on these PRs would be greatly appreciated!

[TimothyBlynJacobs]

Thanks for sharing! That sounds very promising. Will take a deeper look this weekend.

Trac ticket: https://core.trac.wordpress.org/ticket/56481

Trac ticket: https://core.trac.wordpress.org/ticket/56481

[antonvlasenko]

I'd like to share the performance improvement data I measured after submitting two additional PRs and applying them to the relevant controllers:

• ​Users controller: 199 ms improvement (100 users);
• ​Taxonomies controller: 0.45 ms improvement.

[TimothyBlynJacobs]

Yeah, I think we can commit basically this. Can you roll it into one patch @antonvlasenko?

The only other thing, is I'd rather us pass the uppercase form of the methods to the is_method(). That's what we use everywhere in the REST API. I think it makes sense to allow someone to pass the lowercased named. But I think Core should be consistent.

Trac ticket: https://core.trac.wordpress.org/ticket/56481

[antonvlasenko]

@TimothyBlynJacobs I've consolidated all the PRs into one, updated the string cases passed to is_method() to uppercase, and added unit tests for the is_method() method.
Thank you for the review!

PR: ​https://github.com/WordPress/wordpress-develop/pull/7970

Fantastic work, @anton-vlasenko!

Let me know how I can help to land this in WP 6.8. I think it would improve editors' loading performance.

Fantastic work, @anton-vlasenko!

Thank you, @Mamaduka.

Let me know how I can help to land this in WP 6.8. I think it would improve editors' loading performance.

Thank you for offering your help!
A code review and/or testing report could help with landing this in WordPress 6.8.
This ticket is likely on @TimothyBJacobs's radar, so hopefully, he'll be able to approve and commit it when he has the time, provided there’s no additional feedback from the code review.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/7970.
Context: https://core.trac.wordpress.org/ticket/56481#comment:14.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/7970.
Context: https://core.trac.wordpress.org/ticket/56481#comment:14.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/7970.
Context: https://core.trac.wordpress.org/ticket/56481#comment:14.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/7970.
Context: https://core.trac.wordpress.org/ticket/56481#comment:14.

Closed in favour of ​https://github.com/WordPress/wordpress-develop/pull/7970.
Context: https://core.trac.wordpress.org/ticket/56481#comment:14.

Yeah I'm planning on landing this this weekend.

I think we need to also disable any meta and term cache priming as well, as this results in database queries.

Also if there are changes to be made for taxononmy endpoint, those changes should also be made for post type endpoint. Otherwise this looks like great work.

Thank you for the review, @spacedmonkey.
Well spotted.
I've addresses these issues in 294584e3a25df251df985b2c28525ac264b9adff and b80fd51601bb95ea2ddc04ea98b7e4d79f68ac8c.

I think we have a BC issue with the single item route. Right now, if I use the rest_prepare_ filters, I can set headers on the returned response object. After these changes, this no longer happens.

I think we might want to move the is_method( 'HEAD' ) cehcks into the prepare_item_for_response methods, and prepare the empty response object there. That way, if someone adds a header, it is maintained. If they add a property, that's fine because it'll get omitted by the server.

I think we have a BC issue with the single item route. Right now, if I use the rest_prepare_ filters, I can set headers on the returned response object. After these changes, this no longer happens.

I think we might want to move the is_method( 'HEAD' ) cehcks into the prepare_item_for_response methods, and prepare the empty response object there. That way, if someone adds a header, it is maintained. If they add a property, that's fine because it'll get omitted by the server.

Thank you for the review, @TimothyBJacobs.
While I agree that this is a BC break, I tried searching for any plugins that add headers using the rest_prepare_ filters and couldn’t find any:

• ​https://www.wpdirectory.net/search/01JFAMD046JGJRWHWQ1QNMZAW9
• ​https://www.wpdirectory.net/search/01JFAMDVT9ZWJ0TS45ZG5B1D1E

It could theoretically happen, but I’m not aware of any such cases.

That said, I also agree that GET and HEAD requests should behave similarly, with the exception that HEAD requests don’t include a response body.

I’m working on a fix.

---

There are 42 files in the endpoints directory. Some of these are sub classes. But there are other endpoints that are not sub classes of the files edited in this PR. Is this applied all the endpoints that need this change?

No, it hasn’t been applied to all the classes that need editing, as this PR focuses only on the most commonly used REST controllers.
Do you think the other controllers can be addressed in a follow-up PR, or would you prefer to fix everything in a single PR?

There are 42 files in the endpoints directory. Some of these are sub classes. But there are other endpoints that are not sub classes of the files edited in this PR. Is this applied all the endpoints that need this change?

@jonnynews No, it hasn’t been applied to all the classes that need editing, as this PR focuses only on the most commonly used REST controllers. Do you think the other controllers can be addressed in a follow-up PR, or would you prefer to fix everything in a single PR?

In core, we normally do these things one commit.

Some controllers might benefit from this work.

WP_REST_Block_Pattern_Categories_Controller
WP_REST_Block_Types_Controller
WP_REST_Font_Collections_Controller
WP_REST_Global_Styles_Revisions_Controller
WP_REST_Pattern_Directory_Controller
WP_REST_Search_Controller
WP_REST_Sidebars_Controller
WP_REST_Templates_Controller
WP_REST_Widget_Types_Controller
WP_REST_Widgets_Controller

To clear, as I am asking, I believe it just a simply copy and paste for most of these endpoints, but correct me if I am wrong.

Some controllers might benefit from this work.

WP_REST_Block_Pattern_Categories_Controller
WP_REST_Block_Types_Controller
WP_REST_Font_Collections_Controller
WP_REST_Global_Styles_Revisions_Controller
WP_REST_Pattern_Directory_Controller
WP_REST_Search_Controller
WP_REST_Sidebars_Controller
WP_REST_Templates_Controller
WP_REST_Widget_Types_Controller
WP_REST_Widgets_Controller

To clear, as I am asking, I believe it just a simply copy and paste for most of these endpoints, but correct me if I am wrong.

Yes, I believe it is. PHPUnit tests would need to be added as well (which can also be partially copy-pasted). I'll work on this tomorrow.

[spacedmonkey]

Approved PR. This is good to commit. @antonvlasenko @TimothyBlynJacobs

[antonvlasenko]

Approved PR. This is good to commit.

Thank you, @spacedmonkey.

I haven't fixed the following controllers from your list yet:

• WP_REST_Search_Controller
• WP_REST_Sidebars_Controller
• WP_REST_Templates_Controller
• WP_REST_Widget_Types_Controller
• WP_REST_Widgets_Controller

I should be able to fix them by the end of the day.

I believe this PR is ready for review.

Additionally, I've updated some REST controllers beyond the ones ​listed, including:

• WP_REST_Revisions_Controller;
• WP_REST_Template_Revisions_Controller
• WP_REST_Autosaves_Controller;
• WP_REST_Template_Autosaves_Controller.

[flixos90]

@TimothyBlynJacobs Just checking in to get a status update. Based on the feedback above, it looks like this is ready to commit. Are you planning to commit it, or are there any outstanding concerns from your end?

[TimothyBlynJacobs]

Thanks for the ping @flixos90. I'll take another look and plan on committing shortly.

I'm working on fixing the merge conflicts.

[spacedmonkey]

I have reviewed @TimothyBlynJacobs and the PR looks good to go now.