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.

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.

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

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

	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

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

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