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.

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

/wp/v2/pattern-directory/patterns endpoint: slug parameter has no effect on the response #2625

Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7571960
Fix a bug in the WP_REST_Pattern_Directory_Controller:get_items.
anton-vlasenko Apr 25, 2022
f06d3d0
Update ticket reference.
anton-vlasenko Apr 25, 2022
b504030
Fix code style.
anton-vlasenko Apr 25, 2022
b1598ab
Add `slug` to the collection parameters. Previously, it was missing.
anton-vlasenko Apr 25, 2022
e284e1e
Improve PHPDOC blocks.
anton-vlasenko Apr 25, 2022
7fb829d
Update wp-api-generated.js schema.
anton-vlasenko Apr 25, 2022
07aa848
Update wp-api-generated.js schema.
anton-vlasenko Apr 25, 2022
fd1642d
Update wp-api-generated.js schema.
anton-vlasenko Apr 25, 2022
5c9a336
Update comment.
anton-vlasenko Apr 25, 2022
5e923bc
"Slug" must be of type array.
anton-vlasenko Apr 26, 2022
c2122fb
Change the scope of the WP_REST_Pattern_Directory_Controller::get_tra…
anton-vlasenko Apr 26, 2022
005de4a
Replace array_key_exist with isset because $query_args['slug'] isn't …
anton-vlasenko Apr 26, 2022
0a2c2eb
Pass $query_args['slugs'] through wp_parse_list to ensure that it is …
anton-vlasenko Apr 27, 2022
7d1c512
Refactor WP_REST_Pattern_Directory_Controller_Test.
anton-vlasenko Apr 27, 2022
1257482
Refactor the test to use the static $controller property.
anton-vlasenko Apr 27, 2022
2c08eea
Use serialize instead of implode because transient keys must depend o…
anton-vlasenko Apr 27, 2022
0411a81
Improve WP_REST_Pattern_Directory_Controller_Test::test_transient_key…
anton-vlasenko Apr 29, 2022
5653f25
Improve WP_REST_Pattern_Directory_Controller_Test::test_transient_key…
anton-vlasenko Apr 29, 2022
0879fe0
Update the test per @costdev 's suggestion.
anton-vlasenko Apr 29, 2022
8ec23a0
Update the test per @costdev 's suggestion.
anton-vlasenko Apr 29, 2022
1d8f893
Micro-optimisations per @hellofromtonya 's suggestion here: https://g…
anton-vlasenko May 2, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -119,16 +119,7 @@ public function get_items( $request ) {
$query_args['slug'] = $slug;
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
}

/*
* Include a hash of the query args, so that different requests are stored in
* separate caches.
*
* MD5 is chosen for its speed, low-collision rate, universal availability, and to stay
* under the character limit for `_site_transient_timeout_{...}` keys.
*
* @link https://stackoverflow.com/questions/3665247/fastest-hash-for-non-cryptographic-uses
*/
$transient_key = 'wp_remote_block_patterns_' . md5( implode( '-', $query_args ) );
$transient_key = $this->get_transient_key( $query_args );

/*
* Use network-wide transient to improve performance. The locale is the only site
Expand Down Expand Up @@ -337,6 +328,11 @@ public function get_collection_params() {
'minimum' => 1,
);

$query_params['slug'] = array(
'description' => __( 'Limit results to those matching a pattern (slug).' ),
'type' => 'array',
);

/**
* Filter collection parameters for the block pattern directory controller.
*
Expand All @@ -346,4 +342,39 @@ public function get_collection_params() {
*/
return apply_filters( 'rest_pattern_directory_collection_params', $query_params );
}

/*
* Include a hash of the query args, so that different requests are stored in
* separate caches.
*
* MD5 is chosen for its speed, low-collision rate, universal availability, and to stay
* under the character limit for `_site_transient_timeout_{...}` keys.
*
* @link https://stackoverflow.com/questions/3665247/fastest-hash-for-non-cryptographic-uses
*
* @since 6.0.0
*
* @param array $query_args Query arguments to generate a transient key from.
*
* @return string Transient key.
*/
protected function get_transient_key( $query_args ) {

if ( isset( $query_args['slug'] ) ) {
// This is an additional precaution because the "sort" function expects an array.
$query_args['slug'] = (array) $query_args['slug'];
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved

// Sort the array so that the transient key doesn't depend on the order of slugs.
sort( $query_args['slug'] );

// Slugs have to be imploded separately as implode doesn't work with recursive arrays.
$query_args['slug'] = implode( ',', $query_args['slug'] );

if ( '' === trim( $query_args['slug'] ) ) {
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved
unset( $query_args['slug'] );
}
}

return 'wp_remote_block_patterns_' . md5( implode( '-', $query_args ) );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return 'wp_remote_block_patterns_' . md5( implode( '-', $query_args ) );
return 'wp_remote_block_patterns_' . md5( serialize( $query_args ) );

Copy link
Author

@anton-vlasenko anton-vlasenko Apr 27, 2022

Choose a reason for hiding this comment

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

Yes, I've been thinking about replacing implode with wp_json_encode (json_encode is faster than serialize) in the scope of another PR, but serialize will do too.
The result produced by implode doesn't depend on the keys of the $query_args array. Therefore, this increases the chance of collisions.
Fixed in 2c08eea
P.S. I didn't change it because I was afraid that it would cause a sharp increase in the number of requests to the wp/v2/pattern-directory/patterns endpoint once WordPress 6.0 is released. The old values stored in the cache will be invalidated because this PR replaces implode with serialize.
I hope it will not happen since not all WordPress sites are updated simultaneously.

}
}
107 changes: 107 additions & 0 deletions tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ class WP_REST_Pattern_Directory_Controller_Test extends WP_Test_REST_Controller_
*/
protected static $contributor_id;

/**
* An instance of WP_REST_Pattern_Directory_Controller class.
*
* @since 6.0.0
*
* @var WP_REST_Pattern_Directory_Controller
*/
private static $controller;
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved

/**
* Set up class test fixtures.
*
Expand All @@ -34,6 +43,8 @@ public static function wpSetUpBeforeClass( $factory ) {
'role' => 'contributor',
)
);

static::$controller = new WP_REST_Pattern_Directory_Controller();
}

/**
Expand Down Expand Up @@ -399,6 +410,85 @@ public function test_get_item_schema() {
$this->markTestSkipped( "The controller's schema is hardcoded, so tests would not be meaningful." );
}

/**
* Tests if the transient key gets generated correctly.
*
* @dataProvider data_get_query_parameters
*
* @covers WP_REST_Pattern_Directory_Controller::get_transient_key
*
* @since 6.0.0
*
* @ticket 55617
*
* @param array $parameters_1 Expected query arguments.
* @param array $parameters_2 Actual query arguments.
* @param string $message An error message to display.
* @param bool $assert_same Assertion type (assertSame vs assertNotSame).
*/
public function test_transient_keys_get_generated_correctly( $parameters_1, $parameters_2, $message, $assert_same = true ) {
$method = $this->get_reflection_method( self::$controller, 'get_transient_key' );
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved

$result_1 = $method->invoke( self::$controller, $parameters_1 );
$result_2 = $method->invoke( self::$controller, $parameters_2 );

$this->assertIsString( $result_1 );
$this->assertNotEmpty( $result_1 );

$this->assertIsString( $result_2 );
$this->assertNotEmpty( $result_2 );
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved

if ( $assert_same ) {
$this->assertSame( $result_1, $result_2, $message );
} else {
$this->assertNotSame( $result_1, $result_2, $message );
}

}

/**
* @since 6.0.0
*
* @ticket 55617
*/
public function data_get_query_parameters() {
return array(
array(
array(
'parameter_1' => 1,
'slug' => array(),
),
array(
'parameter_1' => 1,
),
'Empty slugs should not affect the transient key.',
),
array(
array(
'parameter_1' => 1,
'slug' => array( 0, 2 ),
),
array(
'parameter_1' => 1,
'slug' => array( 2, 0 ),
),
'message' => 'The order of slugs should not affect transient key.',
),
array(
array(
'parameter_1' => 1,
'slug' => array( 'some_slug' ),
),
array(
'parameter_1' => 1,
'slug' => array( 'some_other_slug' ),
),
'message' => 'Transient keys must not match.',
false,
),
);
}

/**
* Simulate a successful outbound HTTP requests, to keep tests pure and performant.
*
Expand Down Expand Up @@ -461,4 +551,21 @@ static function ( $return, $args, $url ) use ( $blocked_host ) {
3
);
}

/**
* Returns a reflection method and changes its scope.
*
* @since 6.0.0
*
* @param $object An object that has a private method that has to be called.
* @param $method Name of the method.
*
* @return ReflectionMethod
*/
private function get_reflection_method( $object, $method ) {
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved
hellofromtonya marked this conversation as resolved.
Show resolved Hide resolved
$reflection_method = new ReflectionMethod( $object, $method );
$reflection_method->setAccessible( true );

return $reflection_method;
}
}
5 changes: 5 additions & 0 deletions tests/qunit/fixtures/wp-api-generated.js
Original file line number Diff line number Diff line change
Expand Up @@ -10385,6 +10385,11 @@ mockedApiResponse.Schema = {
"type": "integer",
"minimum": 1,
"required": false
},
"slug": {
"description": "Limit results to those matching a pattern (slug).",
"type": "array",
"required": false
}
}
}
Expand Down