Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't prepare the response body for HEAD requests in WP_REST_Taxonomies_Controller #7925

Closed
wants to merge 6 commits into from
+119 −12
Closed

Don't prepare the response body for HEAD requests in WP_REST_Taxonomies_Controller #7925

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e1692a4
Don't prepare response for a single taxonomy.
anton-vlasenko Nov 26, 2024
8bec4d1
Short-circuit the get_items method.
anton-vlasenko Nov 27, 2024
362cdb1
Add more unit tests.
anton-vlasenko Dec 1, 2024
8378c83
Add more unit tests.
anton-vlasenko Dec 1, 2024
4c12f48
Fix the tests.
anton-vlasenko Dec 1, 2024
f119b28
Fix code style.
anton-vlasenko Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/wp-includes/rest-api/class-wp-rest-request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,18 @@ public function get_headers() {
return $this->headers;
}

/**
* Determines if the request is the given method.
*
* @since 6.8.0
*
* @param string $method HTTP method.
* @return bool Whether the request is of the given method.
*/
public function is_method( $method ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function is_method( $method ) {
public function is_method( $method ): bool {

MInimum required PHP version allows the use of type hints, and for bool it is already in use at other places, so imho we should introduce this new function with a type hint.

Copy link
Author

@anton-vlasenko anton-vlasenko Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.
There is currently no agreement on using return type declarations in WordPress Core, so I’d refrain from using them for now. I’m fine with using them as soon as the Community reaches a consensus.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML API for example uses it, so there are already some examples for return types in core. But I get your point, so waiting for a consensus is fine with me.

return $this->get_method() === strtoupper( $method );
}

/**
* Canonicalizes the header name.
*
Expand Down
9 changes: 9 additions & 0 deletions src/wp-includes/rest-api/endpoints/class-wp-rest-taxonomies-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ public function get_items_permissions_check( $request ) {
*/
public function get_items( $request ) {

if ( $request->is_method( 'head' ) ) {
// Return early as this method doesn't add any headers.
return new WP_REST_Response();
}

// Retrieve the list of registered collection query parameters.
$registered = $this->get_collection_params();

Expand Down Expand Up @@ -191,6 +196,10 @@ public function get_item( $request ) {
);
}

if ( $request->is_method( 'head' ) ) {
return new WP_REST_Response();
}

$data = $this->prepare_item_for_response( $tax_obj, $request );

return rest_ensure_response( $data );
Expand Down
112 changes: 100 additions & 12 deletions tests/phpunit/tests/rest-api/rest-taxonomies-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,24 @@ public function test_get_items() {
$this->assertSame( 'tags', $data['post_tag']['rest_base'] );
}

/**
* @ticket 56481
*/
public function test_get_items_with_head_request_should_not_prepare_taxonomy_data() {
$request = new WP_REST_Request( 'HEAD', '/wp/v2/taxonomies' );
$hook_name = 'rest_prepare_taxonomy';
$filter = new MockAction();
$callback = array( $filter, 'filter' );
add_filter( $hook_name, $callback );
$response = rest_get_server()->dispatch( $request );
remove_filter( $hook_name, $callback );
Comment on lines +67 to +73
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I know this is "just a test", but my inner monk wants to sort this. so that this is better readable.

Copy link
Author

@anton-vlasenko anton-vlasenko Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f119b28.


$this->assertSame( 200, $response->get_status(), 'The response status should be 200.' );

$this->assertSame( 0, $filter->get_call_count(), 'The "' . $hook_name . '" filter was called when it should not be for HEAD requests.' );
$this->assertNull( $response->get_data(), 'The server should not generate a body in response to a HEAD request.' );
}

public function test_get_items_context_edit() {
wp_set_current_user( self::$contributor_id );
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies' );
Expand All @@ -79,26 +97,54 @@ public function test_get_items_context_edit() {
$this->assertSame( 'tags', $data['post_tag']['rest_base'] );
}

public function test_get_items_invalid_permission_for_context() {

/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_items_invalid_permission_for_context( $method ) {
wp_set_current_user( 0 );
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies' );
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies' );
$request->set_param( 'context', 'edit' );
$response = rest_get_server()->dispatch( $request );
$this->assertErrorResponse( 'rest_cannot_view', $response, 401 );
}

/**
* Data provider intended to provide HTTP method names for testing GET and HEAD requests.
*
* @return array
*/
public static function data_readable_http_methods() {
return array(
'GET request' => array( 'GET' ),
'HEAD request' => array( 'HEAD' ),
);
}

public function test_get_taxonomies_for_type() {
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies' );
$request->set_param( 'type', 'post' );
$response = rest_get_server()->dispatch( $request );
$this->check_taxonomies_for_type_response( 'post', $response );
}

public function test_get_taxonomies_for_invalid_type() {
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies' );
/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_taxonomies_for_invalid_type( $method ) {
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies' );
$request->set_param( 'type', 'wingding' );
$response = rest_get_server()->dispatch( $request );
$this->assertSame( 200, $response->get_status() );
if ( 'HEAD' === $method ) {
return;
}
$data = $response->get_data();
$this->assertSame( '{}', json_encode( $data ) );
}
Expand All @@ -109,6 +155,24 @@ public function test_get_item() {
$this->check_taxonomy_object_response( 'view', $response );
}

/**
* @ticket 56481
*/
public function test_get_item_with_head_request_should_not_prepare_taxonomy_data() {
$request = new WP_REST_Request( 'HEAD', '/wp/v2/taxonomies/category' );
$hook_name = 'rest_prepare_taxonomy';
$filter = new MockAction();
$callback = array( $filter, 'filter' );
add_filter( $hook_name, $callback );
$response = rest_get_server()->dispatch( $request );
remove_filter( $hook_name, $callback );

$this->assertSame( 200, $response->get_status(), 'The response status should be 200.' );

$this->assertSame( 0, $filter->get_call_count(), 'The "' . $hook_name . '" filter was called when it should not be for HEAD requests.' );
$this->assertNull( $response->get_data(), 'The server should not generate a body in response to a HEAD request.' );
}

public function test_get_item_edit_context() {
$editor_id = self::factory()->user->create( array( 'role' => 'editor' ) );
wp_set_current_user( $editor_id );
Expand All @@ -118,33 +182,57 @@ public function test_get_item_edit_context() {
$this->check_taxonomy_object_response( 'edit', $response );
}

public function test_get_item_invalid_permission_for_context() {
/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_item_invalid_permission_for_context( $method ) {
wp_set_current_user( 0 );
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies/category' );
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies/category' );
$request->set_param( 'context', 'edit' );
$response = rest_get_server()->dispatch( $request );
$this->assertErrorResponse( 'rest_forbidden_context', $response, 401 );
}

public function test_get_invalid_taxonomy() {
$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies/invalid' );
/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_invalid_taxonomy( $method ) {
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies/invalid' );
$response = rest_get_server()->dispatch( $request );
$this->assertErrorResponse( 'rest_taxonomy_invalid', $response, 404 );
}

public function test_get_non_public_taxonomy_not_authenticated() {
/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_non_public_taxonomy_not_authenticated( $method ) {
register_taxonomy( 'api-private', 'post', array( 'public' => false ) );

$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies/api-private' );
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies/api-private' );
$response = rest_get_server()->dispatch( $request );
$this->assertErrorResponse( 'rest_forbidden', $response, 401 );
}

public function test_get_non_public_taxonomy_no_permission() {
/**
* @dataProvider data_readable_http_methods
* @ticket 56481
*
* @param string $method HTTP method to use.
*/
public function test_get_non_public_taxonomy_no_permission( $method ) {
wp_set_current_user( self::$contributor_id );
register_taxonomy( 'api-private', 'post', array( 'public' => false ) );

$request = new WP_REST_Request( 'GET', '/wp/v2/taxonomies/api-private' );
$request = new WP_REST_Request( $method, '/wp/v2/taxonomies/api-private' );
$response = rest_get_server()->dispatch( $request );
$this->assertErrorResponse( 'rest_forbidden', $response, 403 );
}
Expand Down
Loading