Don't prepare the response body for HEAD requests #7970

anton-vlasenko · 2024-12-07T16:28:11Z

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…org/ticket/56481#comment:14).

github-actions · 2024-12-07T16:41:43Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

ironprogrammer

Thanks for putting this all together, @anton-vlasenko 🙌🏻 I've got a name tweak suggestion for some of the newly added methods to consider.

ironprogrammer · 2024-12-09T19:08:26Z

tests/phpunit/tests/rest-api/rest-categories-controller.php

+	 *
+	 * @param string $method HTTP method to use.
+	 */
+	public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {


Suggested change

public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {

public function test_get_items_returns_only_ids_for_head_requests( $method ) {

For this and other same-named tests in the updated test classes under this PR, returns_only_fetches has two verbs, but should only use one for clarity.

Scanning other test classes, "return" is used more extensively than "fetch", so IMHO these new methods should use returns_only_ids. What do you think?

Well spotted, @ironprogrammer! Having two verbs here doesn't make sense. This is likely the result of extensive copy-pasting between different test classes. :)

I'm leaning towards test_get_items_fetches_only_ids_for_head_requests, because what I meant is that the get_items() method fetches only the *id* column from the database for HEAD requests, as opposed to fetching all columns for GET requests.

Fixed in 9c91e45.

ironprogrammer · 2024-12-09T19:40:18Z

tests/phpunit/tests/rest-api/rest-comments-controller.php

+	 *
+	 * @param string $method HTTP method to use.
+	 */
+	public function test_get_items_only_fetches_ids_for_head_requests( $method ) {


Suggested change

public function test_get_items_only_fetches_ids_for_head_requests( $method ) {

public function test_get_items_returns_only_ids_for_head_requests( $method ) {

Suggestion per previous note.

#7970 (comment)

ironprogrammer · 2024-12-09T19:41:57Z

tests/phpunit/tests/rest-api/rest-posts-controller.php

+	 *
+	 * @param string $method HTTP method to use.
+	 */
+	public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {


Suggested change

public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {

public function test_get_items_returns_only_ids_for_head_requests( $method ) {

Suggestion per above.

#7970 (comment)

ironprogrammer · 2024-12-09T19:43:00Z

tests/phpunit/tests/rest-api/rest-tags-controller.php

+	 *
+	 * @param string $method HTTP method to use.
+	 */
+	public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {


Suggested change

public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {

public function test_get_items_returns_only_ids_for_head_requests( $method ) {

Suggestion per above.

#7970 (comment)

ironprogrammer · 2024-12-09T19:45:15Z

tests/phpunit/tests/rest-api/rest-users-controller.php

+	 *
+	 * @param string $method HTTP method to use.
+	 */
+	public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {


Suggested change

public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {

public function test_get_items_returns_only_ids_for_head_requests( $method ) {

Suggestion per above.

#7970 (comment)

@ironprogrammer

Context: WordPress#7970 (comment) Props @ironprogrammer.

Mamaduka · 2024-12-11T12:57:04Z

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.

anton-vlasenko · 2024-12-11T20:16:28Z

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.

tests/phpunit/tests/rest-api/rest-categories-controller.php

github-actions · 2024-12-12T15:22:38Z

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jonnynews.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props antonvlasenko, ironprogrammer, swissspidy, spacedmonkey, mamaduka, timothyblynjacobs.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

TimothyBJacobs · 2024-12-12T20:05:38Z

Yeah I'm planning on landing this this weekend.

spacedmonkey

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.

spacedmonkey · 2024-12-13T11:16:31Z

src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php

+		$is_head_request = $request->is_method( 'HEAD' );
+		if ( $is_head_request ) {
+			// Force the 'fields' argument. For HEAD requests, only post IDs are required to calculate pagination.
+			$prepared_args['fields'] = 'ids';


Should we avoid priming comment meta here as well?

Should we avoid priming comment meta here as well?

Yes, I think we should avoid priming comment meta here.
Fixed in 294584e

…t_Types_Controller.

anton-vlasenko · 2024-12-14T01:54:24Z

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 fixed these issues in 294584e and b80fd51.

TimothyBJacobs · 2024-12-16T20:09:29Z

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.

anton-vlasenko · 2024-12-17T15:49:16Z

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:

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.

jonnynews

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?

anton-vlasenko · 2024-12-17T18:19:52Z

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?

jonnynews · 2024-12-17T18:21:31Z

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.

jonnynews · 2024-12-17T22:11:45Z

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.

anton-vlasenko · 2024-12-17T22:14:38Z

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. Thank you for the review.

…hod to preserve BC.

…ler.

anton-vlasenko added 6 commits December 7, 2024 11:03

Short circuit the WP_REST_Posts_Controller controller.

c2c5547

Short circuit the WP_REST_Comments_Controller controller.

950a3aa

Short circuit the WP_REST_Terms_Controller controller.

fef4d1b

Short circuit the WP_REST_Users_Controller controller.

4ded075

Short circuit the WP_REST_Taxonomies_Controller controller.

07e462f

Use the upper-case http method name (per https://core.trac.wordpress.…

bd17cd9

…org/ticket/56481#comment:14).

anton-vlasenko added 2 commits December 7, 2024 18:01

Add PHPUnit tests for the WP_Request::is_method() method.

92d5278

Better test method name.

ba88c7f

ironprogrammer reviewed Dec 9, 2024

View reviewed changes

Rename the test methods.

9c91e45

Context: WordPress#7970 (comment) Props @ironprogrammer.

anton-vlasenko requested a review from ironprogrammer December 9, 2024 23:24

swissspidy requested review from SergeyBiryukov and spacedmonkey and removed request for ironprogrammer December 11, 2024 22:00

swissspidy reviewed Dec 11, 2024

View reviewed changes

tests/phpunit/tests/rest-api/rest-categories-controller.php Outdated Show resolved Hide resolved

Data providers should always be declared as static. Props @swisspidy.

2e17f2b

anton-vlasenko requested review from swissspidy and ironprogrammer December 12, 2024 00:08

WordPress deleted a comment from github-actions bot Dec 12, 2024

desrosj added the props-bot Adding this label triggers the Props Bot workflow for a PR. label Dec 12, 2024

github-actions bot removed the props-bot Adding this label triggers the Props Bot workflow for a PR. label Dec 12, 2024

spacedmonkey requested changes Dec 13, 2024

View reviewed changes

anton-vlasenko force-pushed the add/short-circuit-head-requests branch from 8a73466 to c7eee61 Compare December 14, 2024 00:54

Disable priming meta for HEAD requests to improve performance.

294584e

anton-vlasenko force-pushed the add/short-circuit-head-requests branch from c7eee61 to 294584e Compare December 14, 2024 00:58

anton-vlasenko added 2 commits December 14, 2024 02:22

Fix CS and typos.

3bd2742

Don't prepare the response body for HEAD requests in WP_Test_REST_Pos…

b80fd51

…t_Types_Controller.

anton-vlasenko force-pushed the add/short-circuit-head-requests branch from 9d1f623 to b80fd51 Compare December 14, 2024 01:51

anton-vlasenko requested a review from spacedmonkey December 14, 2024 01:54

jonnynews reviewed Dec 17, 2024

View reviewed changes

anton-vlasenko force-pushed the add/short-circuit-head-requests branch 2 times, most recently from c1dc6e3 to c9c4d20 Compare December 20, 2024 17:45

Move the check for HEAD requests to ::prepare_item_for_response() met…

8bf96cf

…hod to preserve BC.

anton-vlasenko force-pushed the add/short-circuit-head-requests branch from c9c4d20 to 8bf96cf Compare December 20, 2024 17:51

anton-vlasenko added 6 commits December 21, 2024 00:06

Short circuit the WP_REST_Block_Pattern_Categories_Controller control…

bc1a1ad

…ler.

Short circuit the WP_REST_Block_Types_Controller controller.

3acf69d

Fix CS.

cdb9397

Add unit tests.

6ef5d6a

Short circuit the WP_REST_Font_Collections_Controller controller.

2ef3664

Add unit tests.

f6963e1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't prepare the response body for HEAD requests #7970

Don't prepare the response body for HEAD requests #7970

anton-vlasenko commented Dec 7, 2024

github-actions bot commented Dec 7, 2024

ironprogrammer left a comment

ironprogrammer Dec 9, 2024

anton-vlasenko Dec 9, 2024 •

edited

Loading

ironprogrammer Dec 9, 2024

anton-vlasenko Dec 9, 2024

ironprogrammer Dec 9, 2024

anton-vlasenko Dec 9, 2024

ironprogrammer Dec 9, 2024

anton-vlasenko Dec 9, 2024

ironprogrammer Dec 9, 2024

anton-vlasenko Dec 9, 2024

Mamaduka commented Dec 11, 2024

anton-vlasenko commented Dec 11, 2024

github-actions bot commented Dec 12, 2024 •

edited

Loading

TimothyBJacobs commented Dec 12, 2024

spacedmonkey left a comment

spacedmonkey Dec 13, 2024

anton-vlasenko Dec 14, 2024 •

edited

Loading

anton-vlasenko commented Dec 14, 2024 •

edited

Loading

TimothyBJacobs commented Dec 16, 2024

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

jonnynews left a comment

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

jonnynews commented Dec 17, 2024

jonnynews commented Dec 17, 2024

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

	public function test_get_items_returns_only_fetches_ids_for_head_requests( $method ) {
	public function test_get_items_returns_only_ids_for_head_requests( $method ) {

	public function test_get_items_only_fetches_ids_for_head_requests( $method ) {
	public function test_get_items_returns_only_ids_for_head_requests( $method ) {

Don't prepare the response body for HEAD requests #7970

Are you sure you want to change the base?

Don't prepare the response body for HEAD requests #7970

Conversation

anton-vlasenko commented Dec 7, 2024

github-actions bot commented Dec 7, 2024

Test using WordPress Playground

Some things to be aware of

ironprogrammer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mamaduka commented Dec 11, 2024

anton-vlasenko commented Dec 11, 2024

github-actions bot commented Dec 12, 2024 • edited Loading

Unlinked Accounts

TimothyBJacobs commented Dec 12, 2024

spacedmonkey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

anton-vlasenko commented Dec 14, 2024 • edited Loading

TimothyBJacobs commented Dec 16, 2024

anton-vlasenko commented Dec 17, 2024 • edited Loading

jonnynews left a comment

Choose a reason for hiding this comment

anton-vlasenko commented Dec 17, 2024 • edited Loading

jonnynews commented Dec 17, 2024

jonnynews commented Dec 17, 2024

anton-vlasenko commented Dec 17, 2024 • edited Loading

anton-vlasenko Dec 9, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

anton-vlasenko Dec 14, 2024 •

edited

Loading

anton-vlasenko commented Dec 14, 2024 •

edited

Loading

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

anton-vlasenko commented Dec 17, 2024 •

edited

Loading