From 75719607d872eb8c4069dccd92bc47bc518b186d Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 12:49:37 +0200 Subject: [PATCH 01/21] Fix a bug in the WP_REST_Pattern_Directory_Controller:get_items. We must also implode slugs as implode doesn't work with multi-dimensional arrays. --- ...s-wp-rest-pattern-directory-controller.php | 47 ++++++-- .../rest-pattern-directory-controller.php | 110 ++++++++++++++++++ 2 files changed, 147 insertions(+), 10 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 28f42fd5a4938..a8b53081b79b8 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -119,16 +119,7 @@ public function get_items( $request ) { $query_args['slug'] = $slug; } - /* - * 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 @@ -346,4 +337,40 @@ 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. + */ + private function get_transient_key( $query_args ) { + + if ( array_key_exists( 'slug', $query_args ) ) { + if ( ! is_array( $query_args['slug'] ) ) { + $query_args['slug'] = explode( ',', (string) $query_args['slug'] ); + } + + // Let's sort the array so 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'] ) ) { + unset( $query_args['slug'] ); + } + } + + return 'wp_remote_block_patterns_' . md5( implode( '-', $query_args ) ); + } } diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 53a2e0b3646b9..bb454da5682de 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -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; + /** * Set up class test fixtures. * @@ -34,6 +43,8 @@ public static function wpSetUpBeforeClass( $factory ) { 'role' => 'contributor', ) ); + + static::$controller = new WP_REST_Pattern_Directory_Controller(); } /** @@ -399,6 +410,88 @@ 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 + * + * @ticket xxxxx + * + * @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' ); + + $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 ); + + if ( $assert_same ) { + $this->assertSame( $result_1, $result_2, $message ); + } else { + $this->assertNotSame( $result_1, $result_2, $message ); + } + + } + + public function data_get_query_parameters() { + return array( + array( + array( + 'parameter_1' => 1, + 'slug' => null, + ), + array( + 'parameter_1' => 1, + ), + 'Empty slugs should not affect the transient key.' + ), + array( + array( + 'parameter_1' => 1, + 'slug' => false, + ), + array( + 'parameter_1' => 1, + ), + 'Empty slugs should not affect the transient key.' + ), + array( + array( + 'parameter_1' => 1, + 'slug' => '1,2', + ), + array( + 'parameter_1' => 1, + 'slug' => '2,1', + ), + 'message' => 'The order of slugs should not affect transient key.' + ), + array( + array( + 'parameter_1' => 1, + 'slug' => 'some_slug', + ), + array( + 'parameter_1' => 1, + 'slug' => 'some_other_slug', + ), + 'message' => 'Transient keys must not match.', + false, + ), + ); + } + /** * Simulate a successful outbound HTTP requests, to keep tests pure and performant. * @@ -461,4 +554,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) + { + $reflection_method = new ReflectionMethod($object, $method); + $reflection_method->setAccessible(true); + return $reflection_method; + } } From f06d3d0768c473c0cb020f278f1e776bbfec1f82 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 18:06:46 +0200 Subject: [PATCH 02/21] Update ticket reference. --- .../tests/rest-api/rest-pattern-directory-controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index bb454da5682de..928b7e49a9514 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -417,7 +417,7 @@ public function test_get_item_schema() { * * @covers WP_REST_Pattern_Directory_Controller::get_transient_key * - * @ticket xxxxx + * @ticket 55617 * * @param array $parameters_1 Expected query arguments. * @param array $parameters_2 Actual query arguments. From b504030a99e8856b7fef20fea247f57210198bee Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 18:22:00 +0200 Subject: [PATCH 03/21] Fix code style. --- .../rest-pattern-directory-controller.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 928b7e49a9514..e7e586d652389 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -454,7 +454,7 @@ public function data_get_query_parameters() { array( 'parameter_1' => 1, ), - 'Empty slugs should not affect the transient key.' + 'Empty slugs should not affect the transient key.', ), array( array( @@ -464,7 +464,7 @@ public function data_get_query_parameters() { array( 'parameter_1' => 1, ), - 'Empty slugs should not affect the transient key.' + 'Empty slugs should not affect the transient key.', ), array( array( @@ -475,7 +475,7 @@ public function data_get_query_parameters() { 'parameter_1' => 1, 'slug' => '2,1', ), - 'message' => 'The order of slugs should not affect transient key.' + 'message' => 'The order of slugs should not affect transient key.', ), array( array( @@ -558,17 +558,17 @@ static function ( $return, $args, $url ) use ( $blocked_host ) { /** * 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 + * @since 6.0.0 + * */ - private function get_reflection_method($object, $method) - { - $reflection_method = new ReflectionMethod($object, $method); - $reflection_method->setAccessible(true); + private function get_reflection_method( $object, $method ) { + $reflection_method = new ReflectionMethod( $object, $method ); + $reflection_method->setAccessible( true ); + return $reflection_method; } } From b1598ab4191ac31cb8f3e019163f7c384a22ad5a Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 18:42:18 +0200 Subject: [PATCH 04/21] Add `slug` to the collection parameters. Previously, it was missing. --- .../endpoints/class-wp-rest-pattern-directory-controller.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index a8b53081b79b8..8c3c822cccd45 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -328,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( 'string', 'array' ), + ); + /** * Filter collection parameters for the block pattern directory controller. * From e284e1ec8bccca44cce52eda596c04925320407c Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 18:57:58 +0200 Subject: [PATCH 05/21] Improve PHPDOC blocks. --- .../rest-api/rest-pattern-directory-controller.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index e7e586d652389..30bfd7a9ea058 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -417,6 +417,8 @@ public function test_get_item_schema() { * * @covers WP_REST_Pattern_Directory_Controller::get_transient_key * + * @since 6.0.0 + * * @ticket 55617 * * @param array $parameters_1 Expected query arguments. @@ -444,6 +446,11 @@ public function test_transient_keys_get_generated_correctly( $parameters_1, $par } + /** + * @since 6.0.0 + * + * @ticket 55617 + */ public function data_get_query_parameters() { return array( array( @@ -558,12 +565,12 @@ static function ( $return, $args, $url ) use ( $blocked_host ) { /** * 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 - * @since 6.0.0 - * */ private function get_reflection_method( $object, $method ) { $reflection_method = new ReflectionMethod( $object, $method ); From 7fb829d45cf98a0d4184fb14371bc84f4c91a580 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 20:23:08 +0200 Subject: [PATCH 06/21] Update wp-api-generated.js schema. --- tests/qunit/fixtures/wp-api-generated.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/qunit/fixtures/wp-api-generated.js b/tests/qunit/fixtures/wp-api-generated.js index c8874649d707d..84dfaf24141e6 100644 --- a/tests/qunit/fixtures/wp-api-generated.js +++ b/tests/qunit/fixtures/wp-api-generated.js @@ -10385,7 +10385,15 @@ mockedApiResponse.Schema = { "type": "integer", "minimum": 1, "required": false - } + }, + "slug": { + "description": "Limit results to those matching a pattern (slug).", + "type": [ + "string", + "array" + ], + required: false + } } } ], From 07aa848c3c96c54a6962f9269a6d79efb4869ffd Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 20:37:37 +0200 Subject: [PATCH 07/21] Update wp-api-generated.js schema. --- tests/qunit/fixtures/wp-api-generated.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/qunit/fixtures/wp-api-generated.js b/tests/qunit/fixtures/wp-api-generated.js index 84dfaf24141e6..f94be665d5d98 100644 --- a/tests/qunit/fixtures/wp-api-generated.js +++ b/tests/qunit/fixtures/wp-api-generated.js @@ -10385,15 +10385,15 @@ mockedApiResponse.Schema = { "type": "integer", "minimum": 1, "required": false - }, - "slug": { - "description": "Limit results to those matching a pattern (slug).", - "type": [ - "string", - "array" - ], - required: false - } + }, + "slug": { + "description": "Limit results to those matching a pattern (slug).", + "type": [ + "string", + "array" + ], + required: false + } } } ], From fd1642d736e10ef023bce233b5acc8b4be3af78a Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 20:47:12 +0200 Subject: [PATCH 08/21] Update wp-api-generated.js schema. --- tests/qunit/fixtures/wp-api-generated.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qunit/fixtures/wp-api-generated.js b/tests/qunit/fixtures/wp-api-generated.js index f94be665d5d98..0abee9959e91a 100644 --- a/tests/qunit/fixtures/wp-api-generated.js +++ b/tests/qunit/fixtures/wp-api-generated.js @@ -10392,7 +10392,7 @@ mockedApiResponse.Schema = { "string", "array" ], - required: false + "required": false } } } From 5c9a336481cb584064cb5a9ed7105b1f33316236 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 25 Apr 2022 22:09:54 +0200 Subject: [PATCH 09/21] Update comment. --- .../endpoints/class-wp-rest-pattern-directory-controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 8c3c822cccd45..20d491df72eb7 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -365,7 +365,7 @@ private function get_transient_key( $query_args ) { $query_args['slug'] = explode( ',', (string) $query_args['slug'] ); } - // Let's sort the array so the transient key doesn't depend on the order of slugs. + // 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. From 5e923bc73da1322a2b64bfaf9fc090abfadfc57b Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Tue, 26 Apr 2022 11:17:42 +0200 Subject: [PATCH 10/21] "Slug" must be of type array. Strings will be automatically converted to an array. Per suggestion here: https://github.com/WordPress/wordpress-develop/pull/2625#discussion_r858069357 --- .../endpoints/class-wp-rest-pattern-directory-controller.php | 2 +- tests/qunit/fixtures/wp-api-generated.js | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 20d491df72eb7..786abb628786e 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -330,7 +330,7 @@ public function get_collection_params() { $query_params['slug'] = array( 'description' => __( 'Limit results to those matching a pattern (slug).' ), - 'type' => array( 'string', 'array' ), + 'type' => 'array', ); /** diff --git a/tests/qunit/fixtures/wp-api-generated.js b/tests/qunit/fixtures/wp-api-generated.js index 0abee9959e91a..6161cf14fc137 100644 --- a/tests/qunit/fixtures/wp-api-generated.js +++ b/tests/qunit/fixtures/wp-api-generated.js @@ -10388,10 +10388,7 @@ mockedApiResponse.Schema = { }, "slug": { "description": "Limit results to those matching a pattern (slug).", - "type": [ - "string", - "array" - ], + "type": "array", "required": false } } From c2122fb148d7257f6cbf254a94de6711871fac60 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Tue, 26 Apr 2022 18:22:42 +0200 Subject: [PATCH 11/21] Change the scope of the WP_REST_Pattern_Directory_Controller::get_transient_key method as it would be helpful to make it available to child classes. --- .../endpoints/class-wp-rest-pattern-directory-controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 786abb628786e..a4b24b0505cb6 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -358,7 +358,7 @@ public function get_collection_params() { * * @return string Transient key. */ - private function get_transient_key( $query_args ) { + protected function get_transient_key( $query_args ) { if ( array_key_exists( 'slug', $query_args ) ) { if ( ! is_array( $query_args['slug'] ) ) { From 005de4aa8ae617dc0df455d433861e7a83bf4bce Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Wed, 27 Apr 2022 01:09:40 +0200 Subject: [PATCH 12/21] Replace array_key_exist with isset because $query_args['slug'] isn't supposed to be null. --- ...s-wp-rest-pattern-directory-controller.php | 7 +++---- .../rest-pattern-directory-controller.php | 20 +++++-------------- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index a4b24b0505cb6..a6848f301d214 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -360,10 +360,9 @@ public function get_collection_params() { */ protected function get_transient_key( $query_args ) { - if ( array_key_exists( 'slug', $query_args ) ) { - if ( ! is_array( $query_args['slug'] ) ) { - $query_args['slug'] = explode( ',', (string) $query_args['slug'] ); - } + if ( isset( $query_args['slug'] ) ) { + // This is an additional precaution because the "sort" function expects an array. + $query_args['slug'] = (array) $query_args['slug']; // Sort the array so that the transient key doesn't depend on the order of slugs. sort( $query_args['slug'] ); diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 30bfd7a9ea058..4fa25dd665c05 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -456,7 +456,7 @@ public function data_get_query_parameters() { array( array( 'parameter_1' => 1, - 'slug' => null, + 'slug' => array(), ), array( 'parameter_1' => 1, @@ -466,32 +466,22 @@ public function data_get_query_parameters() { array( array( 'parameter_1' => 1, - 'slug' => false, + 'slug' => array( 0, 2 ), ), array( 'parameter_1' => 1, - ), - 'Empty slugs should not affect the transient key.', - ), - array( - array( - 'parameter_1' => 1, - 'slug' => '1,2', - ), - array( - 'parameter_1' => 1, - 'slug' => '2,1', + 'slug' => array( 2, 0 ), ), 'message' => 'The order of slugs should not affect transient key.', ), array( array( 'parameter_1' => 1, - 'slug' => 'some_slug', + 'slug' => array( 'some_slug' ), ), array( 'parameter_1' => 1, - 'slug' => 'some_other_slug', + 'slug' => array( 'some_other_slug' ), ), 'message' => 'Transient keys must not match.', false, From 0a2c2ebfbfaf9de86be352e0957942f73b253f81 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Wed, 27 Apr 2022 16:15:42 +0200 Subject: [PATCH 13/21] Pass $query_args['slugs'] through wp_parse_list to ensure that it is an array. --- .../endpoints/class-wp-rest-pattern-directory-controller.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index a6848f301d214..4e11ef78bd6b5 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -362,7 +362,7 @@ 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']; + $query_args['slug'] = wp_parse_list( $query_args['slug'] ); // Sort the array so that the transient key doesn't depend on the order of slugs. sort( $query_args['slug'] ); From 7d1c51221c42336c151aa454c7ce3b407b7e250f Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Wed, 27 Apr 2022 16:58:21 +0200 Subject: [PATCH 14/21] Refactor WP_REST_Pattern_Directory_Controller_Test. Use WP_REST_Pattern_Directory_Controller_Test::setUp to set up the controller. --- .../rest-pattern-directory-controller.php | 47 ++++++++----------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 4fa25dd665c05..25fa0cd796b9e 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -28,7 +28,7 @@ class WP_REST_Pattern_Directory_Controller_Test extends WP_Test_REST_Controller_ * * @var WP_REST_Pattern_Directory_Controller */ - private static $controller; + private $controller; /** * Set up class test fixtures. @@ -43,8 +43,17 @@ public static function wpSetUpBeforeClass( $factory ) { 'role' => 'contributor', ) ); + } - static::$controller = new WP_REST_Pattern_Directory_Controller(); + /** + * Set up class test fixtures. + * + * @since 6.0.0 + */ + public function setUp() { + parent::setUp(); + + $this->controller = new WP_REST_Pattern_Directory_Controller(); } /** @@ -333,12 +342,11 @@ public function test_delete_item() { * @since 5.8.0 */ public function test_prepare_item() { - $controller = new WP_REST_Pattern_Directory_Controller(); $raw_patterns = json_decode( self::get_raw_response( 'browse-all' ) ); $raw_patterns[0]->extra_field = 'this should be removed'; - $prepared_pattern = $controller->prepare_response_for_collection( - $controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) + $prepared_pattern = $this->controller->prepare_response_for_collection( + $this->controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) ); $this->assertPatternMatchesSchema( $prepared_pattern ); @@ -351,12 +359,11 @@ public function test_prepare_item() { * @since 5.8.0 */ public function test_prepare_item_search() { - $controller = new WP_REST_Pattern_Directory_Controller(); $raw_patterns = json_decode( self::get_raw_response( 'search' ) ); $raw_patterns[0]->extra_field = 'this should be removed'; - $prepared_pattern = $controller->prepare_response_for_collection( - $controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) + $prepared_pattern = $this->controller->prepare_response_for_collection( + $this->controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) ); $this->assertPatternMatchesSchema( $prepared_pattern ); @@ -427,10 +434,11 @@ public function test_get_item_schema() { * @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' ); + $reflection_method = new ReflectionMethod( $this->controller, 'get_transient_key' ); + $reflection_method->setAccessible( true ); - $result_1 = $method->invoke( self::$controller, $parameters_1 ); - $result_2 = $method->invoke( self::$controller, $parameters_2 ); + $result_1 = $reflection_method->invoke( self::$controller, $parameters_1 ); + $result_2 = $reflection_method->invoke( self::$controller, $parameters_2 ); $this->assertIsString( $result_1 ); $this->assertNotEmpty( $result_1 ); @@ -551,21 +559,4 @@ 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 ) { - $reflection_method = new ReflectionMethod( $object, $method ); - $reflection_method->setAccessible( true ); - - return $reflection_method; - } } From 1257482d039b0322f78f0781c732deb80b0e9c67 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Wed, 27 Apr 2022 19:36:51 +0200 Subject: [PATCH 15/21] Refactor the test to use the static $controller property. It seems there is some issue with the ::setUp method. --- .../rest-pattern-directory-controller.php | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 25fa0cd796b9e..807037da86484 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -28,7 +28,7 @@ class WP_REST_Pattern_Directory_Controller_Test extends WP_Test_REST_Controller_ * * @var WP_REST_Pattern_Directory_Controller */ - private $controller; + private static $controller; /** * Set up class test fixtures. @@ -43,17 +43,8 @@ public static function wpSetUpBeforeClass( $factory ) { 'role' => 'contributor', ) ); - } - - /** - * Set up class test fixtures. - * - * @since 6.0.0 - */ - public function setUp() { - parent::setUp(); - $this->controller = new WP_REST_Pattern_Directory_Controller(); + static::$controller = new WP_REST_Pattern_Directory_Controller(); } /** @@ -62,7 +53,7 @@ public function setUp() { * @param WP_REST_Response[] $pattern An individual pattern from the REST API response. */ public function assertPatternMatchesSchema( $pattern ) { - $schema = ( new WP_REST_Pattern_Directory_Controller() )->get_item_schema(); + $schema = static::$controller->get_item_schema(); $pattern_id = isset( $pattern->id ) ? $pattern->id : '{pattern ID is missing}'; $this->assertTrue( @@ -345,8 +336,8 @@ public function test_prepare_item() { $raw_patterns = json_decode( self::get_raw_response( 'browse-all' ) ); $raw_patterns[0]->extra_field = 'this should be removed'; - $prepared_pattern = $this->controller->prepare_response_for_collection( - $this->controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) + $prepared_pattern = static::$controller->prepare_response_for_collection( + static::$controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) ); $this->assertPatternMatchesSchema( $prepared_pattern ); @@ -362,8 +353,8 @@ public function test_prepare_item_search() { $raw_patterns = json_decode( self::get_raw_response( 'search' ) ); $raw_patterns[0]->extra_field = 'this should be removed'; - $prepared_pattern = $this->controller->prepare_response_for_collection( - $this->controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) + $prepared_pattern = static::$controller->prepare_response_for_collection( + static::$controller->prepare_item_for_response( $raw_patterns[0], new WP_REST_Request() ) ); $this->assertPatternMatchesSchema( $prepared_pattern ); @@ -434,7 +425,7 @@ public function test_get_item_schema() { * @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 ) { - $reflection_method = new ReflectionMethod( $this->controller, 'get_transient_key' ); + $reflection_method = new ReflectionMethod( static::$controller, 'get_transient_key' ); $reflection_method->setAccessible( true ); $result_1 = $reflection_method->invoke( self::$controller, $parameters_1 ); From 2c08eeab216148827b02aa5f99a309948ca31a3b Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Wed, 27 Apr 2022 19:45:22 +0200 Subject: [PATCH 16/21] Use serialize instead of implode because transient keys must depend on request keys. --- ...s-wp-rest-pattern-directory-controller.php | 8 +++----- .../rest-pattern-directory-controller.php | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 4e11ef78bd6b5..8db9eb7f2f824 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -367,14 +367,12 @@ protected function get_transient_key( $query_args ) { // 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'] ) ) { + // Empty arrays should not affect the transient key. + if ( 0 === count( $query_args['slug'] ) ) { unset( $query_args['slug'] ); } } - return 'wp_remote_block_patterns_' . md5( implode( '-', $query_args ) ); + return 'wp_remote_block_patterns_' . md5( serialize( $query_args ) ); } } diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 807037da86484..734e452bc0d69 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -473,6 +473,16 @@ public function data_get_query_parameters() { ), 'message' => 'The order of slugs should not affect transient key.', ), + array( + array( + 'parameter_1' => 1, + 'slug' => array(), + ), + array( + 'parameter_1' => 1, + ), + 'message' => 'Transient keys must match.', + ), array( array( 'parameter_1' => 1, @@ -485,6 +495,16 @@ public function data_get_query_parameters() { 'message' => 'Transient keys must not match.', false, ), + array( + array( + 'parameter_1' => 1, + ), + array( + 'parameter_2' => 1, + ), + 'message' => 'Transient keys must depend on array keys.', + false, + ), ); } From 0411a8126f16dac6d7e7dd730cdd4a2b7cbacf33 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Fri, 29 Apr 2022 15:49:34 +0200 Subject: [PATCH 17/21] Improve WP_REST_Pattern_Directory_Controller_Test::test_transient_keys_get_generated_correctly: add error messages. --- .../tests/rest-api/rest-pattern-directory-controller.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 734e452bc0d69..67f4b6b642723 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -431,11 +431,11 @@ public function test_transient_keys_get_generated_correctly( $parameters_1, $par $result_1 = $reflection_method->invoke( self::$controller, $parameters_1 ); $result_2 = $reflection_method->invoke( self::$controller, $parameters_2 ); - $this->assertIsString( $result_1 ); - $this->assertNotEmpty( $result_1 ); + $this->assertIsString( $result_1, 'The transient key must be a string.' ); + $this->assertNotEmpty( $result_1, 'The transient key must not be empty.' ); - $this->assertIsString( $result_2 ); - $this->assertNotEmpty( $result_2 ); + $this->assertIsString( $result_2, 'The transient key must be a string.' ); + $this->assertNotEmpty( $result_2, 'The transient key must not be empty.' ); if ( $assert_same ) { $this->assertSame( $result_1, $result_2, $message ); From 5653f25aa2893fc715d5dd9be6ecd7483aa83e64 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Fri, 29 Apr 2022 15:52:13 +0200 Subject: [PATCH 18/21] Improve WP_REST_Pattern_Directory_Controller_Test::test_transient_keys_get_generated_correctly: add error messages. --- .../tests/rest-api/rest-pattern-directory-controller.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 67f4b6b642723..c0ff680d36c53 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -431,11 +431,11 @@ public function test_transient_keys_get_generated_correctly( $parameters_1, $par $result_1 = $reflection_method->invoke( self::$controller, $parameters_1 ); $result_2 = $reflection_method->invoke( self::$controller, $parameters_2 ); - $this->assertIsString( $result_1, 'The transient key must be a string.' ); - $this->assertNotEmpty( $result_1, 'The transient key must not be empty.' ); + $this->assertIsString( $result_1, 'Transient key #1 must be a string.' ); + $this->assertNotEmpty( $result_1, 'Transient key #1 must not be empty.' ); - $this->assertIsString( $result_2, 'The transient key must be a string.' ); - $this->assertNotEmpty( $result_2, 'The transient key must not be empty.' ); + $this->assertIsString( $result_2, 'Transient key #2 must be a string.' ); + $this->assertNotEmpty( $result_2, 'Transient key #2 must not be empty.' ); if ( $assert_same ) { $this->assertSame( $result_1, $result_2, $message ); From 0879fe08723b153db98a7db5762e4d9a8898a107 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Fri, 29 Apr 2022 16:20:55 +0200 Subject: [PATCH 19/21] Update the test per @costdev 's suggestion. --- .../rest-pattern-directory-controller.php | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index c0ff680d36c53..2ab0f6b14c32a 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -452,7 +452,7 @@ public function test_transient_keys_get_generated_correctly( $parameters_1, $par */ public function data_get_query_parameters() { return array( - array( + 'same key and empty slugs' => array( array( 'parameter_1' => 1, 'slug' => array(), @@ -462,7 +462,7 @@ public function data_get_query_parameters() { ), 'Empty slugs should not affect the transient key.', ), - array( + 'same key and slugs in different order' => array( array( 'parameter_1' => 1, 'slug' => array( 0, 2 ), @@ -471,19 +471,9 @@ public function data_get_query_parameters() { 'parameter_1' => 1, 'slug' => array( 2, 0 ), ), - 'message' => 'The order of slugs should not affect transient key.', + 'message' => 'The order of slugs should not affect the transient key.', ), - array( - array( - 'parameter_1' => 1, - 'slug' => array(), - ), - array( - 'parameter_1' => 1, - ), - 'message' => 'Transient keys must match.', - ), - array( + 'same key and different slugs' => array( array( 'parameter_1' => 1, 'slug' => array( 'some_slug' ), @@ -495,7 +485,7 @@ public function data_get_query_parameters() { 'message' => 'Transient keys must not match.', false, ), - array( + 'different keys' => array( array( 'parameter_1' => 1, ), From 8ec23a091f7a4b2b640f651da0265d78b136ca52 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Fri, 29 Apr 2022 16:26:27 +0200 Subject: [PATCH 20/21] Update the test per @costdev 's suggestion. --- .../rest-pattern-directory-controller.php | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php index 2ab0f6b14c32a..43ca497edbd40 100644 --- a/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php +++ b/tests/phpunit/tests/rest-api/rest-pattern-directory-controller.php @@ -453,46 +453,46 @@ public function test_transient_keys_get_generated_correctly( $parameters_1, $par public function data_get_query_parameters() { return array( 'same key and empty slugs' => array( - array( + 'parameters_1' => array( 'parameter_1' => 1, 'slug' => array(), ), - array( + 'parameters_2' => array( 'parameter_1' => 1, ), - 'Empty slugs should not affect the transient key.', + 'message' => 'Empty slugs should not affect the transient key.', ), 'same key and slugs in different order' => array( - array( + 'parameters_1' => array( 'parameter_1' => 1, 'slug' => array( 0, 2 ), ), - array( + 'parameters_2' => array( 'parameter_1' => 1, 'slug' => array( 2, 0 ), ), - 'message' => 'The order of slugs should not affect the transient key.', + 'message' => 'The order of slugs should not affect the transient key.', ), 'same key and different slugs' => array( - array( + 'parameters_1' => array( 'parameter_1' => 1, 'slug' => array( 'some_slug' ), ), - array( + 'parameters_2' => array( 'parameter_1' => 1, 'slug' => array( 'some_other_slug' ), ), - 'message' => 'Transient keys must not match.', + 'message' => 'Transient keys must not match.', false, ), 'different keys' => array( - array( + 'parameters_1' => array( 'parameter_1' => 1, ), - array( + 'parameters_2' => array( 'parameter_2' => 1, ), - 'message' => 'Transient keys must depend on array keys.', + 'message' => 'Transient keys must depend on array keys.', false, ), ); From 1d8f893540d5fb7755444376d606059cfd677508 Mon Sep 17 00:00:00 2001 From: Anton Vlasenko Date: Mon, 2 May 2022 15:02:25 +0200 Subject: [PATCH 21/21] Micro-optimisations per @hellofromtonya 's suggestion here: https://github.com/WordPress/wordpress-develop/pull/2625#discussion_r862808359 --- .../class-wp-rest-pattern-directory-controller.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php index 8db9eb7f2f824..8e5f063cd5024 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-pattern-directory-controller.php @@ -364,12 +364,12 @@ protected function get_transient_key( $query_args ) { // This is an additional precaution because the "sort" function expects an array. $query_args['slug'] = wp_parse_list( $query_args['slug'] ); - // Sort the array so that the transient key doesn't depend on the order of slugs. - sort( $query_args['slug'] ); - // Empty arrays should not affect the transient key. - if ( 0 === count( $query_args['slug'] ) ) { + if ( empty( $query_args['slug'] ) ) { unset( $query_args['slug'] ); + } else { + // Sort the array so that the transient key doesn't depend on the order of slugs. + sort( $query_args['slug'] ); } }