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.

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, janusdev, ironprogrammer, swissspidy, spacedmonkey, mukesh27, 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.

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

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.

…troller for HEAD requests.

…emplate_Revisions_Controller controllers.

anton-vlasenko · 2025-01-15T21:29:25Z

~~I'm working on fixing the merge conflicts.~~

UPD: Done.

…ons-controller.php Don't use a PHPDoc block because it doesn't contain any PHPDoc tags. Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

spacedmonkey

Looks good to me

github-actions · 2025-03-02T22:06:34Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 59899
GitHub commit: 1c2a87b

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

ironprogrammer reviewed Dec 9, 2024

View reviewed changes

anton-vlasenko added a commit to anton-vlasenko/wordpress-develop that referenced this pull request Dec 9, 2024

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

anton-vlasenko requested review from ironprogrammer and swissspidy 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

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

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

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

anton-vlasenko and others added 16 commits January 15, 2025 22:03

Short circuit the WP_REST_Sidebars_Controller controller.

bdd97cf

Remove non-existent test method parameter.

6772204

Add unit tests.

e0c6424

Fix backward compatibility issues in the WP_REST_Font_Collections_Con…

f79d0dd

…troller for HEAD requests.

Short circuit the WP_REST_Templates_Controller controller.

be26c39

Add unit tests.

8c384d4

Short circuit the WP_REST_Widget_Types_Controller controller.

eb7d4d2

Fix the unit test.

25b39a3

Add unit tests.

792128f

Short circuit the WP_REST_Widgets_Controller controller.

bd9bae3

Add unit tests.

e6ed30f

Unify inline comments.

61010ec

Short circuit the WP_REST_Autosaves_Controller controller.

dccc30d

Add unit tests.

1fbaeb4

Short-circuit the WP_REST_Template_Autosaves_Controller and WP_REST_T…

85ce233

…emplate_Revisions_Controller controllers.

Rename test methods.

9d4b831

anton-vlasenko and others added 4 commits January 15, 2025 22:50

Add unit tests.

cb42502

Add unit tests.

021c2c5

Short circuit the WP_REST_Pattern_Directory_Controller controller.

72a3a1e

Update src/wp-includes/rest-api/endpoints/class-wp-rest-font-collecti…

4765127

…ons-controller.php Don't use a PHPDoc block because it doesn't contain any PHPDoc tags. Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

anton-vlasenko force-pushed the add/short-circuit-head-requests branch 2 times, most recently from 8d13489 to fd60b44 Compare January 15, 2025 23:51

spacedmonkey approved these changes Jan 16, 2025

View reviewed changes

anton-vlasenko added 2 commits January 17, 2025 14:33

Fix CS.

d4da2d8

Fix the failing PHPUnit tests.

48b4c48

anton-vlasenko force-pushed the add/short-circuit-head-requests branch from fd60b44 to 48b4c48 Compare January 17, 2025 13:33

github-actions bot closed this Mar 2, 2025

anton-vlasenko mentioned this pull request Mar 25, 2025

FIX: Add REST API filters to the Global Styles Revisions controller. #8062

Open

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

Conversation

anton-vlasenko commented Dec 7, 2024

Uh oh!

github-actions bot commented Dec 7, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

ironprogrammer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anton-vlasenko Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mamaduka commented Dec 11, 2024

Uh oh!

anton-vlasenko commented Dec 11, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unlinked Accounts

Uh oh!

TimothyBJacobs commented Dec 12, 2024

Uh oh!

spacedmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anton-vlasenko commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyBJacobs commented Dec 16, 2024

Uh oh!

anton-vlasenko commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anton-vlasenko commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacedmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

anton-vlasenko Dec 9, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

anton-vlasenko commented Dec 14, 2024 •

edited

Loading

anton-vlasenko commented Dec 17, 2024 •

edited

Loading

anton-vlasenko commented Jan 15, 2025 •

edited

Loading