From 57a35e4fb61ac5c7826c7fada942ddca609ec792 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 2 Dec 2024 13:18:35 -0800 Subject: [PATCH 01/25] Revert "Remove od_store_url_metric_validity filter to be re-added in follow-up PR" This reverts commit 514c51344f993c010edff305c27084c65e3da70d. --- plugins/image-prioritizer/helper.php | 60 +++++++++++++++++++ plugins/image-prioritizer/hooks.php | 1 + .../storage/rest-api.php | 29 +++++++++ 3 files changed, 90 insertions(+) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index c93153dcf..f569ecbb0 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -136,6 +136,66 @@ function image_prioritizer_add_element_item_schema_properties( array $additional return $additional_properties; } +/** + * Validates that the provided background image URL is valid. + * + * @since n.e.x.t + * + * @param bool|WP_Error|mixed $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + * @return bool|WP_Error Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + */ +function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Strict_URL_Metric $url_metric ) { + if ( ! is_bool( $validity ) && ! ( $validity instanceof WP_Error ) ) { + $validity = (bool) $validity; + } + + $data = $url_metric->get( 'lcpElementExternalBackgroundImage' ); + if ( ! is_array( $data ) ) { + return $validity; + } + + $r = wp_safe_remote_head( + $data['url'], + array( + 'redirection' => 3, // Allow up to 3 redirects. + ) + ); + if ( $r instanceof WP_Error ) { + return new WP_Error( + WP_DEBUG ? $r->get_error_code() : 'head_request_failure', + __( 'HEAD request for background image URL failed.', 'image-prioritizer' ) . ( WP_DEBUG ? ' ' . $r->get_error_message() : '' ), + array( + 'code' => 500, + ) + ); + } + $response_code = wp_remote_retrieve_response_code( $r ); + if ( $response_code < 200 || $response_code >= 400 ) { + return new WP_Error( + 'background_image_response_not_ok', + __( 'HEAD request for background image URL did not return with a success status code.', 'image-prioritizer' ), + array( + 'code' => WP_DEBUG ? $response_code : 400, + ) + ); + } + + $content_type = wp_remote_retrieve_header( $r, 'Content-Type' ); + if ( ! is_string( $content_type ) || ! str_starts_with( $content_type, 'image/' ) ) { + return new WP_Error( + 'background_image_response_not_image', + __( 'HEAD request for background image URL did not return an image Content-Type.', 'image-prioritizer' ), + array( + 'code' => 400, + ) + ); + } + + // TODO: Check for the Content-Length and return invalid if it is gigantic? + return $validity; +} + /** * Gets the path to a script or stylesheet. * diff --git a/plugins/image-prioritizer/hooks.php b/plugins/image-prioritizer/hooks.php index 7587e9e67..4a47f3564 100644 --- a/plugins/image-prioritizer/hooks.php +++ b/plugins/image-prioritizer/hooks.php @@ -13,3 +13,4 @@ add_action( 'od_init', 'image_prioritizer_init' ); add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ); add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ); +add_filter( 'od_store_url_metric_validity', 'image_prioritizer_filter_store_url_metric_validity', 10, 2 ); diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 38f3c9dda..8e4828bec 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -210,6 +210,35 @@ function od_handle_rest_request( WP_REST_Request $request ) { ); } + /** + * Filters whether a URL Metric is valid for storage. + * + * This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This is + * also necessary because the 'validate_callback' key in a JSON Schema is not respected when gathering the REST API + * endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, the REST API doesn't + * support 'validate_callback' for any nested arguments in any case, meaning that custom constraints would be able + * to be applied to multidimensional objects, such as the items inside 'elements'. + * + * This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric + * loaded from the od_url_metric post type. This means that validation logic enforced via this filter can be more + * expensive, such as doing filesystem checks or HTTP requests. + * + * @since n.e.x.t + * + * @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + */ + $validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric ); + if ( false === $validity || ( $validity instanceof WP_Error && $validity->has_errors() ) ) { + if ( false === $validity ) { + $validity = new WP_Error( 'invalid_url_metric', __( 'Validity of URL Metric was rejected by filter.', 'optimization-detective' ) ); + } + if ( ! isset( $validity->error_data['code'] ) ) { + $validity->error_data['code'] = 400; + } + return $validity; + } + // TODO: This should be changed from store_url_metric($slug, $url_metric) instead be update_post( $slug, $group_collection ). As it stands, store_url_metric() is duplicating logic here. $result = OD_URL_Metrics_Post_Type::store_url_metric( $request->get_param( 'slug' ), From 57df74084bd36805187e439f971086ccb49aa726 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 2 Dec 2024 13:19:58 -0800 Subject: [PATCH 02/25] Use 'status' key instead of 'code' Co-authored-by: felixarntz --- plugins/optimization-detective/storage/rest-api.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 8e4828bec..bafbbb7e1 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -233,8 +233,8 @@ function od_handle_rest_request( WP_REST_Request $request ) { if ( false === $validity ) { $validity = new WP_Error( 'invalid_url_metric', __( 'Validity of URL Metric was rejected by filter.', 'optimization-detective' ) ); } - if ( ! isset( $validity->error_data['code'] ) ) { - $validity->error_data['code'] = 400; + if ( ! isset( $validity->error_data['status'] ) ) { + $validity->error_data['status'] = 400; } return $validity; } From 9de74b35a55974c1b82721c08c845836eb63615d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 11 Dec 2024 12:55:29 -0800 Subject: [PATCH 03/25] Add clear_cache() method to OD_URL_Metric_Group --- .../class-od-url-metric-group.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/plugins/optimization-detective/class-od-url-metric-group.php b/plugins/optimization-detective/class-od-url-metric-group.php index b32f4b8b6..29496a1bb 100644 --- a/plugins/optimization-detective/class-od-url-metric-group.php +++ b/plugins/optimization-detective/class-od-url-metric-group.php @@ -237,8 +237,7 @@ public function add_url_metric( OD_URL_Metric $url_metric ): void { ); } - $this->result_cache = array(); - $this->collection->clear_cache(); + $this->clear_cache(); $url_metric->set_group( $this ); $this->url_metrics[] = $url_metric; @@ -471,6 +470,16 @@ public function count(): int { return count( $this->url_metrics ); } + /** + * Clear result cache. + * + * @since n.e.x.t + */ + public function clear_cache(): void { + $this->result_cache = array(); + $this->collection->clear_cache(); + } + /** * Specifies data which should be serialized to JSON. * From ad1d9eab562454fe7ff97a8bcdd4f8079a3160a0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 13:28:09 -0800 Subject: [PATCH 04/25] Add ability to unset an extended property on OD_URL_Metric and OD_Element --- .../class-od-element.php | 45 ++++++++-- .../class-od-url-metric.php | 38 ++++++++- .../tests/test-class-od-element.php | 73 +++++++++++++--- .../tests/test-class-od-url-metric.php | 25 ++++++ ...-class-od-url-metrics-group-collection.php | 83 +++++++++++++++++++ 5 files changed, 245 insertions(+), 19 deletions(-) diff --git a/plugins/optimization-detective/class-od-element.php b/plugins/optimization-detective/class-od-element.php index c7f243d99..be8400ce4 100644 --- a/plugins/optimization-detective/class-od-element.php +++ b/plugins/optimization-detective/class-od-element.php @@ -208,18 +208,53 @@ public function offsetSet( $offset, $value ): void { } /** - * Offset to unset. + * Unsets a property. * - * This is disallowed. Attempting to unset a property will throw an error. + * This is particularly useful in conjunction with the `od_url_metric_schema_element_item_additional_properties` filter. + * This will throw an exception if the property is required by the schema. * - * @since 0.7.0 + * @since n.e.x.t + * + * @param string $key Property. + * + * @throws OD_Data_Validation_Exception When attempting to unset a property required by the JSON Schema. + */ + public function unset( string $key ): void { + $schema = OD_URL_Metric::get_json_schema(); // TODO: This should be a non-static method once the URL Metric is instantiated. + if ( + isset( $schema['properties']['elements']['items']['properties'][ $key ]['required'] ) + && true === $schema['properties']['elements']['items']['properties'][ $key ]['required'] + ) { + throw new OD_Data_Validation_Exception( + esc_html( + sprintf( + /* translators: %s is the property key. */ + __( 'The %s key is required for an item of elements in a URL Metric.', 'optimization-detective' ), + $key + ) + ) + ); + } + unset( $this->data[ $key ] ); // @phpstan-ignore assign.propertyType (Above required check ensures $key is not isLCP, isLCPCandidate, xpath, intersectionRatio, intersectionRect, boundingClientRect.) + $group = $this->url_metric->get_group(); + if ( $group instanceof OD_URL_Metric_Group ) { + $group->clear_cache(); + } + } + + /** + * Unsets an offset. + * + * This will throw an exception if the offset is required by the schema. + * + * @since n.e.x.t * * @param mixed $offset Offset. * - * @throws Exception When attempting to unset a property. + * @throws OD_Data_Validation_Exception When attempting to unset a property required by the JSON Schema. */ public function offsetUnset( $offset ): void { - throw new Exception( 'Element data may not be unset.' ); + $this->unset( (string) $offset ); } /** diff --git a/plugins/optimization-detective/class-od-url-metric.php b/plugins/optimization-detective/class-od-url-metric.php index d3b0d984f..b68045e6e 100644 --- a/plugins/optimization-detective/class-od-url-metric.php +++ b/plugins/optimization-detective/class-od-url-metric.php @@ -503,6 +503,33 @@ function ( array $element ): OD_Element { return $this->elements; } + /** + * Unsets a property from the URL Metric. + * + * @since n.e.x.t + * + * @param string $key Key to unset. + * @throws OD_Data_Validation_Exception If the property is required an exception will be thrown. + */ + public function unset( string $key ): void { + $schema = self::get_json_schema(); // TODO: Consider capturing the schema as a private member variable once the URL Metric is constructed. + if ( isset( $schema['properties'][ $key ]['required'] ) && true === $schema['properties'][ $key ]['required'] ) { + throw new OD_Data_Validation_Exception( + esc_html( + sprintf( + /* translators: %s is the property key. */ + __( 'The %s key is required at the root of a URL Metric.', 'optimization-detective' ), + $key + ) + ) + ); + } + unset( $this->data[ $key ] ); // @phpstan-ignore assign.propertyType (Above required check ensures $key is not uuid, url, timestamp, viewport, or elements.) + if ( $this->group instanceof OD_URL_Metric_Group ) { + $this->group->clear_cache(); + } + } + /** * Specifies data which should be serialized to JSON. * @@ -511,6 +538,15 @@ function ( array $element ): OD_Element { * @return Data Exports to be serialized by json_encode(). */ public function jsonSerialize(): array { - return $this->data; + $data = $this->data; + + $data['elements'] = array_map( + static function ( OD_Element $element ): array { + return $element->jsonSerialize(); + }, + $this->get_elements() + ); + + return $data; } } diff --git a/plugins/optimization-detective/tests/test-class-od-element.php b/plugins/optimization-detective/tests/test-class-od-element.php index 52a4cc43f..1b980d421 100644 --- a/plugins/optimization-detective/tests/test-class-od-element.php +++ b/plugins/optimization-detective/tests/test-class-od-element.php @@ -24,6 +24,7 @@ class Test_OD_Element extends WP_UnitTestCase { * @covers ::offsetExists * @covers ::offsetGet * @covers ::offsetSet + * @covers ::unset * @covers ::offsetUnset * @covers ::jsonSerialize */ @@ -130,22 +131,68 @@ static function ( array $schema ): array { $this->assertTrue( isset( $element['customProp'] ) ); $this->assertTrue( $element->offsetExists( 'customProp' ) ); - $this->assertEquals( $element_data, $element->jsonSerialize() ); + // Try setting property (which is not currently supported). + $functions = array( + static function ( OD_Element $element ): void { + $element['isLCP'] = true; + }, + static function ( OD_Element $element ): void { + $element->offsetSet( 'isLCP', true ); + }, + ); + foreach ( $functions as $function ) { + $exception = null; + try { + $function( $element ); + } catch ( Exception $e ) { + $exception = $e; + } + $this->assertInstanceOf( Exception::class, $exception ); + $this->assertFalse( $element->get( 'isLCP' ) ); + } - $exception = null; - try { - $element['isLCP'] = true; - } catch ( Exception $e ) { - $exception = $e; + // Try unsetting a required property. + $functions = array( + static function ( OD_Element $element ): void { + unset( $element['isLCP'] ); + }, + static function ( OD_Element $element ): void { + $element->unset( 'isLCP' ); + }, + static function ( OD_Element $element ): void { + $element->offsetUnset( 'isLCP' ); + }, + ); + foreach ( $functions as $function ) { + $exception = null; + try { + $function( $element ); + } catch ( Exception $e ) { + $exception = $e; + } + $this->assertInstanceOf( Exception::class, $exception ); + $this->assertArrayHasKey( 'isLCP', $element->jsonSerialize() ); } - $this->assertInstanceOf( Exception::class, $exception ); - $exception = null; - try { - unset( $element['isLCP'] ); - } catch ( Exception $e ) { // @phpstan-ignore catch.neverThrown (It is thrown by offsetUnset actually.) - $exception = $e; + $this->assertEquals( $element_data, $element->jsonSerialize() ); + + // Try unsetting a non-required property. + $functions = array( + static function ( OD_Element $element ): void { + unset( $element['customProp'] ); + }, + static function ( OD_Element $element ): void { + $element->unset( 'customProp' ); + }, + static function ( OD_Element $element ): void { + $element->offsetUnset( 'customProp' ); + }, + ); + foreach ( $functions as $function ) { + $cloned_element = clone $element; + $function( $cloned_element ); + $this->assertFalse( $cloned_element->offsetExists( 'customProp' ) ); + $this->assertArrayNotHasKey( 'customProp', $cloned_element->jsonSerialize() ); } - $this->assertInstanceOf( Exception::class, $exception ); // @phpstan-ignore method.impossibleType (It is thrown by offsetUnset actually.) } } diff --git a/plugins/optimization-detective/tests/test-class-od-url-metric.php b/plugins/optimization-detective/tests/test-class-od-url-metric.php index 51b23c243..df2fa0144 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metric.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metric.php @@ -277,6 +277,7 @@ static function ( $value ) { * @covers ::get_json_schema * @covers ::set_group * @covers ::get_group + * @covers ::unset * * @dataProvider data_provider_to_test_constructor * @@ -336,6 +337,15 @@ static function ( OD_Element $element ) { $this->assertTrue( wp_is_uuid( $url_metric->get_uuid() ) ); $this->assertSame( $url_metric->get_uuid(), $url_metric->get( 'uuid' ) ); + $exception = null; + try { + $url_metric->unset( 'elements' ); + } catch ( OD_Data_Validation_Exception $e ) { + $exception = $e; + } + $this->assertInstanceOf( OD_Data_Validation_Exception::class, $exception ); + $url_metric->unset( 'does_not_exist' ); + $serialized = $url_metric->jsonSerialize(); if ( ! array_key_exists( 'uuid', $data ) ) { $this->assertTrue( wp_is_uuid( $serialized['uuid'] ) ); @@ -397,6 +407,12 @@ static function ( array $properties ): array { $this->assertArrayHasKey( 'isTouch', $extended_data ); $this->assertTrue( $extended_data['isTouch'] ); $this->assertTrue( $extended_url_metric->get( 'isTouch' ) ); + + $this->assertTrue( $extended_url_metric->get( 'isTouch' ) ); + $extended_url_metric->unset( 'isTouch' ); + $this->assertNull( $extended_url_metric->get( 'isTouch' ) ); + $extended_data = $extended_url_metric->jsonSerialize(); + $this->assertArrayNotHasKey( 'isTouch', $extended_data ); }, ), @@ -489,6 +505,13 @@ static function ( array $properties ): array { $this->assertArrayHasKey( 'isColorful', $extended_data['elements'][0] ); $this->assertFalse( $extended_data['elements'][0]['isColorful'] ); $this->assertFalse( $extended_url_metric->get_elements()[0]['isColorful'] ); + + $element = $extended_url_metric->get_elements()[0]; + $this->assertFalse( $element->get( 'isColorful' ) ); + $element->unset( 'isColorful' ); + $this->assertNull( $element->get( 'isColorful' ) ); + $extended_data = $extended_url_metric->jsonSerialize(); + $this->assertArrayNotHasKey( 'isColorful', $extended_data['elements'][0] ); }, ), @@ -554,6 +577,8 @@ static function ( array $properties ): array { * Tests construction with extended schema. * * @covers ::get_json_schema + * @covers ::unset + * @covers OD_Element::unset * * @dataProvider data_provider_to_test_constructor_with_extended_schema * diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index 179d282c4..034e50661 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -199,6 +199,89 @@ public function data_provider_sample_size_and_breakpoints(): array { ); } + /** + * Test clear_cache(). + * + * @covers ::clear_cache + * @covers OD_URL_Metric_Group::clear_cache + * @covers OD_URL_Metric::unset + * @covers OD_Element::unset + */ + public function test_clear_cache(): void { + $collection = new OD_URL_Metric_Group_Collection( array(), md5( '' ), array(), 1, DAY_IN_SECONDS ); + $populated_value = array( 'foo' => true ); + $group = $collection->get_first_group(); + + // Get private members. + $collection_result_cache_reflection_property = new ReflectionProperty( OD_URL_Metric_Group_Collection::class, 'result_cache' ); + $collection_result_cache_reflection_property->setAccessible( true ); + $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); + $group_result_cache_reflection_property = new ReflectionProperty( OD_URL_Metric_Group::class, 'result_cache' ); + $group_result_cache_reflection_property->setAccessible( true ); + $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); + + // Test clear_cache() on collection. + $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); + $collection->clear_cache(); + $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); + + // Test that adding a URL metric to a collection clears the caches. + $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); + $group_result_cache_reflection_property->setValue( $group, $populated_value ); + + add_filter( + 'od_url_metric_schema_root_additional_properties', + static function ( $schema ) { + $schema['new_prop_at_root'] = array( + 'type' => 'string', + ); + return $schema; + } + ); + add_filter( + 'od_url_metric_schema_element_additional_properties', + static function ( $schema ) { + $schema['new_prop_at_element'] = array( + 'type' => 'string', + ); + return $schema; + } + ); + + $collection->add_url_metric( + $this->get_sample_url_metric( + array( + 'element' => array( + 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]', + 'new_prop_at_element' => 'hey there', + ), + 'extended_root' => array( + 'new_prop_at_root' => 'hola', + ), + ) + ) + ); + $url_metric = $group->getIterator()->current(); + $this->assertInstanceOf( OD_URL_Metric::class, $url_metric ); + $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); + $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); + + // Test that modifying a URL Metric empties the cache of the collection and the group. + $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); + $group_result_cache_reflection_property->setValue( $group, $populated_value ); + $url_metric->unset( 'new_prop_at_root' ); + $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); + $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); + + // Test that modifying a URL Metric element empties the cache of the collection and the group. + $element = $url_metric->get_elements()[0]; + $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); + $group_result_cache_reflection_property->setValue( $group, $populated_value ); + $element->unset( 'new_prop_at_element' ); + $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); + $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); + } + /** * Test add_url_metric(). * From 8fba89a0c403e5e08e6d176ee5d69c656f4a3d6f Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 15:37:29 -0800 Subject: [PATCH 05/25] Suppress erroneous IDE warnings --- plugins/image-prioritizer/helper.php | 6 ++++++ .../optimization-detective/class-od-html-tag-processor.php | 7 +++++++ plugins/optimization-detective/helper.php | 7 +++++++ .../storage/class-od-url-metrics-post-type.php | 6 ++++++ 4 files changed, 26 insertions(+) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 7300b9593..6340f5052 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -205,6 +205,7 @@ function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Stric * @param string $src_path Source path, relative to plugin root. * @param string|null $min_path Minified path. If not supplied, then '.min' is injected before the file extension in the source path. * @return string URL to script or stylesheet. + * @noinspection PhpDocMissingThrowsInspection */ function image_prioritizer_get_asset_path( string $src_path, ?string $min_path = null ): string { if ( null === $min_path ) { @@ -215,6 +216,11 @@ function image_prioritizer_get_asset_path( string $src_path, ?string $min_path = $force_src = false; if ( WP_DEBUG && ! file_exists( trailingslashit( __DIR__ ) . $min_path ) ) { $force_src = true; + /** + * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. + * + * @noinspection PhpUnhandledExceptionInspection + */ wp_trigger_error( __FUNCTION__, sprintf( diff --git a/plugins/optimization-detective/class-od-html-tag-processor.php b/plugins/optimization-detective/class-od-html-tag-processor.php index 7fc896711..16952a23f 100644 --- a/plugins/optimization-detective/class-od-html-tag-processor.php +++ b/plugins/optimization-detective/class-od-html-tag-processor.php @@ -645,8 +645,15 @@ public function get_updated_html(): string { * * @param string $function_name Function name. * @param string $message Warning message. + * + * @noinspection PhpDocMissingThrowsInspection */ private function warn( string $function_name, string $message ): void { + /** + * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. + * + * @noinspection PhpUnhandledExceptionInspection + */ wp_trigger_error( $function_name, esc_html( $message ) diff --git a/plugins/optimization-detective/helper.php b/plugins/optimization-detective/helper.php index b9dc348f9..bc9bbfb7f 100644 --- a/plugins/optimization-detective/helper.php +++ b/plugins/optimization-detective/helper.php @@ -73,6 +73,8 @@ function od_render_generator_meta_tag(): void { * @param string $src_path Source path, relative to plugin root. * @param string|null $min_path Minified path. If not supplied, then '.min' is injected before the file extension in the source path. * @return string URL to script or stylesheet. + * + * @noinspection PhpDocMissingThrowsInspection */ function od_get_asset_path( string $src_path, ?string $min_path = null ): string { if ( null === $min_path ) { @@ -83,6 +85,11 @@ function od_get_asset_path( string $src_path, ?string $min_path = null ): string $force_src = false; if ( WP_DEBUG && ! file_exists( trailingslashit( __DIR__ ) . $min_path ) ) { $force_src = true; + /** + * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. + * + * @noinspection PhpUnhandledExceptionInspection + */ wp_trigger_error( __FUNCTION__, sprintf( diff --git a/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php b/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php index 814abaac9..8bf337691 100644 --- a/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php +++ b/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php @@ -115,6 +115,7 @@ public static function get_post( string $slug ): ?WP_Post { * * @param WP_Post $post URL Metrics post. * @return OD_URL_Metric[] URL Metrics. + * @noinspection PhpDocMissingThrowsInspection */ public static function get_url_metrics_from_post( WP_Post $post ): array { $this_function = __METHOD__; @@ -123,6 +124,11 @@ public static function get_url_metrics_from_post( WP_Post $post ): array { if ( ! in_array( $error_level, array( E_USER_NOTICE, E_USER_WARNING, E_USER_ERROR, E_USER_DEPRECATED ), true ) ) { $error_level = E_USER_NOTICE; } + /** + * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. + * + * @noinspection PhpUnhandledExceptionInspection + */ wp_trigger_error( $this_function, esc_html( $message ), $error_level ); }; From 8f2af8766f6ef99e66cc7db634c674352a913f6d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 17:12:22 -0800 Subject: [PATCH 06/25] Unset lcpElementExternalBackgroundImage if URL is invalid --- plugins/image-prioritizer/helper.php | 124 ++++++++++++++---- .../image-prioritizer/tests/test-hooks.php | 1 + .../storage/rest-api.php | 31 ++++- 3 files changed, 127 insertions(+), 29 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 6340f5052..56c7f10c0 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -138,62 +138,134 @@ function image_prioritizer_add_element_item_schema_properties( array $additional } /** - * Validates that the provided background image URL is valid. + * Validates URL for a background image. * * @since n.e.x.t * - * @param bool|WP_Error|mixed $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. - * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. - * @return bool|WP_Error Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param string $url Background image URL. + * @return true|WP_Error Validity. */ -function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Strict_URL_Metric $url_metric ) { - if ( ! is_bool( $validity ) && ! ( $validity instanceof WP_Error ) ) { - $validity = (bool) $validity; +function image_prioritizer_validate_background_image_url( string $url ) { + $parsed_url = wp_parse_url( $url ); + if ( false === $parsed_url || ! isset( $parsed_url['host'] ) ) { + return new WP_Error( + 'background_image_url_lacks_host', + __( 'Supplied background image URL does not have a host.', 'image-prioritizer' ) + ); } - $data = $url_metric->get( 'lcpElementExternalBackgroundImage' ); - if ( ! is_array( $data ) ) { - return $validity; + $allowed_hosts = array_map( + static function ( $host ) { + return wp_parse_url( $host, PHP_URL_HOST ); + }, + get_allowed_http_origins() + ); + + // Obtain the host of an image attachment's URL in case a CDN is pointing all images to an origin other than the home or site URLs. + $image_attachment_query = new WP_Query( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'image', + 'post_status' => 'inherit', + 'posts_per_page' => 1, + 'fields' => 'ids', + ) + ); + if ( isset( $image_attachment_query->posts[0] ) && is_int( $image_attachment_query->posts[0] ) ) { + $src = wp_get_attachment_image_src( $image_attachment_query->posts[0] ); + if ( is_array( $src ) ) { + $attachment_image_src_host = wp_parse_url( $src[0], PHP_URL_HOST ); + if ( is_string( $attachment_image_src_host ) ) { + $allowed_hosts[] = $attachment_image_src_host; + } + } } + // Validate that the background image URL is for an allowed host. + if ( ! in_array( $parsed_url['host'], $allowed_hosts, true ) ) { + return new WP_Error( + 'disallowed_background_image_url_host', + sprintf( + /* translators: %s is the list of allowed hosts */ + __( 'Background image URL host is not among allowed: %s.', 'image-prioritizer' ), + join( ', ', $allowed_hosts ) + ) + ); + } + + // Validate that the URL points to a valid resource. $r = wp_safe_remote_head( - $data['url'], + $url, array( 'redirection' => 3, // Allow up to 3 redirects. ) ); if ( $r instanceof WP_Error ) { - return new WP_Error( - WP_DEBUG ? $r->get_error_code() : 'head_request_failure', - __( 'HEAD request for background image URL failed.', 'image-prioritizer' ) . ( WP_DEBUG ? ' ' . $r->get_error_message() : '' ), - array( - 'code' => 500, - ) - ); + return $r; } $response_code = wp_remote_retrieve_response_code( $r ); if ( $response_code < 200 || $response_code >= 400 ) { return new WP_Error( 'background_image_response_not_ok', - __( 'HEAD request for background image URL did not return with a success status code.', 'image-prioritizer' ), - array( - 'code' => WP_DEBUG ? $response_code : 400, + sprintf( + /* translators: %s is the HTTP status code */ + __( 'HEAD request for background image URL did not return with a success status code: %s.', 'image-prioritizer' ), + $response_code ) ); } - $content_type = wp_remote_retrieve_header( $r, 'Content-Type' ); - if ( ! is_string( $content_type ) || ! str_starts_with( $content_type, 'image/' ) ) { + // Validate that the Content-Type is an image. + $content_type = (array) wp_remote_retrieve_header( $r, 'Content-Type' ); + if ( ! is_string( $content_type[0] ) || ! str_starts_with( $content_type[0], 'image/' ) ) { return new WP_Error( 'background_image_response_not_image', - __( 'HEAD request for background image URL did not return an image Content-Type.', 'image-prioritizer' ), - array( - 'code' => 400, + sprintf( + /* translators: %s is the content type of the response */ + __( 'HEAD request for background image URL did not return an image Content-Type: %s.', 'image-prioritizer' ), + $content_type[0] ) ); } // TODO: Check for the Content-Length and return invalid if it is gigantic? + return true; +} + +/** + * Filters the validity of a URL Metric with an LCP element background image. + * + * This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background + * image URL is not valid. Removal of the property is preferable to + * + * @since n.e.x.t + * + * @param bool|WP_Error|mixed $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + * @return bool|WP_Error Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * + * @noinspection PhpDocMissingThrowsInspection + * @throws OD_Data_Validation_Exception Except it won't because lcpElementExternalBackgroundImage is not a required property. + */ +function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Strict_URL_Metric $url_metric ) { + if ( ! is_bool( $validity ) && ! ( $validity instanceof WP_Error ) ) { + $validity = (bool) $validity; + } + + $data = $url_metric->get( 'lcpElementExternalBackgroundImage' ); + if ( is_array( $data ) && isset( $data['url'] ) && is_string( $data['url'] ) ) { // Note: The isset() and is_string() checks aren't necessary since the JSON Schema enforces them to be set. + $validity = image_prioritizer_validate_background_image_url( $data['url'] ); + if ( is_wp_error( $validity ) ) { + /** + * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. + * + * @noinspection PhpUnhandledExceptionInspection + */ + wp_trigger_error( __FUNCTION__, $validity->get_error_message() . ' Background image URL: ' . $data['url'] ); + $url_metric->unset( 'lcpElementExternalBackgroundImage' ); + } + } + return $validity; } diff --git a/plugins/image-prioritizer/tests/test-hooks.php b/plugins/image-prioritizer/tests/test-hooks.php index 740c71a3d..282180768 100644 --- a/plugins/image-prioritizer/tests/test-hooks.php +++ b/plugins/image-prioritizer/tests/test-hooks.php @@ -14,5 +14,6 @@ public function test_hooks_added(): void { $this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) ); $this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) ); $this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) ); + $this->assertEquals( 10, has_filter( 'od_store_url_metric_validity', 'image_prioritizer_filter_store_url_metric_validity' ) ); } } diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index bafbbb7e1..35fd27a99 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -223,12 +223,37 @@ function od_handle_rest_request( WP_REST_Request $request ) { * loaded from the od_url_metric post type. This means that validation logic enforced via this filter can be more * expensive, such as doing filesystem checks or HTTP requests. * + * In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a + * plugin may also mutate the OD_URL_Metric instance passed by reference to the filter callback. This is useful + * for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. + * * @since n.e.x.t * - * @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. - * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + * @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + * @param array $url_metric_data Original URL Metric data before any mutations. */ - $validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric ); + try { + $validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric, $url_metric->jsonSerialize() ); // TODO: A better name might be `od_url_metric_storage_validity`. + } catch ( Exception $e ) { + $error_data = null; + if ( WP_DEBUG ) { + $error_data = array( + 'exception_class' => get_class( $e ), + 'exception_message' => $e->getMessage(), + 'exception_code' => $e->getCode(), + ); + } + $validity = new WP_Error( + 'exception', + sprintf( + /* translators: %s is the filter name 'od_store_url_metric_validity' */ + __( 'An %s filter callback threw an exception.', 'optimization-detective' ), + 'od_store_url_metric_validity' + ), + $error_data + ); + } if ( false === $validity || ( $validity instanceof WP_Error && $validity->has_errors() ) ) { if ( false === $validity ) { $validity = new WP_Error( 'invalid_url_metric', __( 'Validity of URL Metric was rejected by filter.', 'optimization-detective' ) ); From 42005e6e41d9dd0a055b3b98c968aeaddc63c69c Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 17:31:54 -0800 Subject: [PATCH 07/25] Improve docs and tidiness --- plugins/image-prioritizer/hooks.php | 2 +- .../image-prioritizer/tests/test-hooks.php | 2 +- .../class-od-url-metric-group-collection.php | 4 +- .../class-od-url-metric-group.php | 2 +- plugins/optimization-detective/readme.txt | 14 +++++ .../storage/rest-api.php | 52 +++++++++---------- ...-class-od-url-metrics-group-collection.php | 6 +-- 7 files changed, 47 insertions(+), 35 deletions(-) diff --git a/plugins/image-prioritizer/hooks.php b/plugins/image-prioritizer/hooks.php index 4a47f3564..fbfe8c6b3 100644 --- a/plugins/image-prioritizer/hooks.php +++ b/plugins/image-prioritizer/hooks.php @@ -13,4 +13,4 @@ add_action( 'od_init', 'image_prioritizer_init' ); add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ); add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ); -add_filter( 'od_store_url_metric_validity', 'image_prioritizer_filter_store_url_metric_validity', 10, 2 ); +add_filter( 'od_url_metric_storage_validity', 'image_prioritizer_filter_store_url_metric_validity', 10, 2 ); diff --git a/plugins/image-prioritizer/tests/test-hooks.php b/plugins/image-prioritizer/tests/test-hooks.php index 282180768..3c2e02014 100644 --- a/plugins/image-prioritizer/tests/test-hooks.php +++ b/plugins/image-prioritizer/tests/test-hooks.php @@ -14,6 +14,6 @@ public function test_hooks_added(): void { $this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) ); $this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) ); $this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) ); - $this->assertEquals( 10, has_filter( 'od_store_url_metric_validity', 'image_prioritizer_filter_store_url_metric_validity' ) ); + $this->assertEquals( 10, has_filter( 'od_url_metric_storage_validity', 'image_prioritizer_filter_store_url_metric_validity' ) ); } } diff --git a/plugins/optimization-detective/class-od-url-metric-group-collection.php b/plugins/optimization-detective/class-od-url-metric-group-collection.php index ed7fe6928..c87ac93e3 100644 --- a/plugins/optimization-detective/class-od-url-metric-group-collection.php +++ b/plugins/optimization-detective/class-od-url-metric-group-collection.php @@ -225,7 +225,7 @@ public function get_last_group(): OD_URL_Metric_Group { } /** - * Clear result cache. + * Clears result cache. * * @since 0.3.0 */ @@ -234,7 +234,7 @@ public function clear_cache(): void { } /** - * Create groups. + * Creates groups. * * @since 0.1.0 * diff --git a/plugins/optimization-detective/class-od-url-metric-group.php b/plugins/optimization-detective/class-od-url-metric-group.php index 29496a1bb..f8772eca7 100644 --- a/plugins/optimization-detective/class-od-url-metric-group.php +++ b/plugins/optimization-detective/class-od-url-metric-group.php @@ -471,7 +471,7 @@ public function count(): int { } /** - * Clear result cache. + * Clears result cache. * * @since n.e.x.t */ diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index dbe6afd22..c1530f56f 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -246,6 +246,20 @@ The ETag is a unique identifier that changes whenever the underlying data used t When the ETag for URL Metrics in a complete viewport group no longer matches the current environment's ETag, new URL Metrics will then begin to be collected until there are no more stored URL Metrics with the old ETag. These new URL Metrics will include data relevant to the newly activated plugins and their tag visitors. +**Filter:** `od_url_metric_storage_validity` (default args: true) + +Filters whether a URL Metric is valid for storage. + +Three paramters are passed to this filter: + +1. `$validity` (`bool|WP_Error`): Validity. Invalid if false or a WP_Error with errors. +2. `$url_metric` (`OD_Strict_URL_Metric`): URL Metric, already validated against the JSON Schema. +3. `$url_metric_data` (`array`): Original URL Metric data before any mutations. + +This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is loaded from the `od_url_metrics` post type. This means that validation logic enforced via this filter can be more expensive, such as doing filesystem checks or HTTP requests. + +In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a plugin may also mutate the `OD_URL_Metric` instance passed by reference to the filter callback. This is useful for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. + **Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`) Fires whenever a URL Metric was successfully stored. diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 35fd27a99..b8e33cf1e 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -210,31 +210,31 @@ function od_handle_rest_request( WP_REST_Request $request ) { ); } - /** - * Filters whether a URL Metric is valid for storage. - * - * This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This is - * also necessary because the 'validate_callback' key in a JSON Schema is not respected when gathering the REST API - * endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, the REST API doesn't - * support 'validate_callback' for any nested arguments in any case, meaning that custom constraints would be able - * to be applied to multidimensional objects, such as the items inside 'elements'. - * - * This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric - * loaded from the od_url_metric post type. This means that validation logic enforced via this filter can be more - * expensive, such as doing filesystem checks or HTTP requests. - * - * In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a - * plugin may also mutate the OD_URL_Metric instance passed by reference to the filter callback. This is useful - * for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. - * - * @since n.e.x.t - * - * @param bool|WP_Error $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. - * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. - * @param array $url_metric_data Original URL Metric data before any mutations. - */ try { - $validity = apply_filters( 'od_store_url_metric_validity', true, $url_metric, $url_metric->jsonSerialize() ); // TODO: A better name might be `od_url_metric_storage_validity`. + /** + * Filters whether a URL Metric is valid for storage. + * + * This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This is + * also necessary because the 'validate_callback' key in a JSON Schema is not respected when gathering the REST API + * endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, the REST API doesn't + * support 'validate_callback' for any nested arguments in any case, meaning that custom constraints would be able + * to be applied to multidimensional objects, such as the items inside 'elements'. + * + * This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is + * loaded from the od_url_metrics post type. This means that validation logic enforced via this filter can be more + * expensive, such as doing filesystem checks or HTTP requests. + * + * In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a + * plugin may also mutate the OD_URL_Metric instance passed by reference to the filter callback. This is useful + * for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. + * + * @since n.e.x.t + * + * @param bool|WP_Error $validity Validity. Invalid if false or a WP_Error with errors. + * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. + * @param array $url_metric_data Original URL Metric data before any mutations. + */ + $validity = apply_filters( 'od_url_metric_storage_validity', true, $url_metric, $url_metric->jsonSerialize() ); } catch ( Exception $e ) { $error_data = null; if ( WP_DEBUG ) { @@ -247,9 +247,9 @@ function od_handle_rest_request( WP_REST_Request $request ) { $validity = new WP_Error( 'exception', sprintf( - /* translators: %s is the filter name 'od_store_url_metric_validity' */ + /* translators: %s is the filter name 'od_url_metric_storage_validity' */ __( 'An %s filter callback threw an exception.', 'optimization-detective' ), - 'od_store_url_metric_validity' + 'od_url_metric_storage_validity' ), $error_data ); diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index 034e50661..957acd1a4 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -226,9 +226,6 @@ public function test_clear_cache(): void { $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); // Test that adding a URL metric to a collection clears the caches. - $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); - $group_result_cache_reflection_property->setValue( $group, $populated_value ); - add_filter( 'od_url_metric_schema_root_additional_properties', static function ( $schema ) { @@ -247,7 +244,8 @@ static function ( $schema ) { return $schema; } ); - + $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); + $group_result_cache_reflection_property->setValue( $group, $populated_value ); $collection->add_url_metric( $this->get_sample_url_metric( array( From 0d584b711803e68b847329c4859827f470b925f2 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 22:56:26 -0800 Subject: [PATCH 08/25] Add tests for od_url_metric_storage_validity filter --- .../storage/rest-api.php | 33 ++- .../tests/storage/test-rest-api.php | 201 ++++++++++++++---- 2 files changed, 182 insertions(+), 52 deletions(-) diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index b8e33cf1e..198b57284 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -236,12 +236,14 @@ function od_handle_rest_request( WP_REST_Request $request ) { */ $validity = apply_filters( 'od_url_metric_storage_validity', true, $url_metric, $url_metric->jsonSerialize() ); } catch ( Exception $e ) { - $error_data = null; + $error_data = array( + 'status' => 500, + ); if ( WP_DEBUG ) { - $error_data = array( - 'exception_class' => get_class( $e ), - 'exception_message' => $e->getMessage(), - 'exception_code' => $e->getCode(), + $error_data['exception'] = array( + 'class' => get_class( $e ), + 'message' => $e->getMessage(), + 'code' => $e->getCode(), ); } $validity = new WP_Error( @@ -254,12 +256,13 @@ function od_handle_rest_request( WP_REST_Request $request ) { $error_data ); } - if ( false === $validity || ( $validity instanceof WP_Error && $validity->has_errors() ) ) { + if ( false === (bool) $validity || ( $validity instanceof WP_Error && $validity->has_errors() ) ) { if ( false === $validity ) { $validity = new WP_Error( 'invalid_url_metric', __( 'Validity of URL Metric was rejected by filter.', 'optimization-detective' ) ); } - if ( ! isset( $validity->error_data['status'] ) ) { - $validity->error_data['status'] = 400; + $error_code = $validity->get_error_code(); + if ( ! isset( $validity->error_data[ $error_code ]['status'] ) ) { + $validity->error_data[ $error_code ]['status'] = 400; } return $validity; } @@ -269,9 +272,19 @@ function od_handle_rest_request( WP_REST_Request $request ) { $request->get_param( 'slug' ), $url_metric ); - if ( $result instanceof WP_Error ) { - return $result; + $error_data = array( + 'status' => 500, + ); + if ( WP_DEBUG ) { + $error_data['code'] = $result->get_error_code(); + $error_data['message'] = $result->get_error_message(); + } + return new WP_Error( + 'unable_to_store_url_metric', + __( 'Unable to store URL Metric.', 'optimization-detective' ), + $error_data + ); } $post_id = $result; diff --git a/plugins/optimization-detective/tests/storage/test-rest-api.php b/plugins/optimization-detective/tests/storage/test-rest-api.php index ad514aa48..0caf8b5a3 100644 --- a/plugins/optimization-detective/tests/storage/test-rest-api.php +++ b/plugins/optimization-detective/tests/storage/test-rest-api.php @@ -28,21 +28,74 @@ public function test_od_register_endpoint_hooked(): void { * @return array */ public function data_provider_to_test_rest_request_good_params(): array { + $add_root_extra_property = static function ( string $property_name ): void { + add_filter( + 'od_url_metric_schema_root_additional_properties', + static function ( array $properties ) use ( $property_name ): array { + $properties[ $property_name ] = array( + 'type' => 'string', + ); + return $properties; + } + ); + }; + return array( - 'not_extended' => array( - 'set_up' => function (): array { + 'not_extended' => array( + 'set_up' => function (): array { return $this->get_valid_params(); }, + 'expect_stored' => true, + 'expected_status' => 200, ), - 'extended' => array( - 'set_up' => function (): array { + 'extended' => array( + 'set_up' => function () use ( $add_root_extra_property ): array { + $add_root_extra_property( 'extra' ); + $params = $this->get_valid_params(); + $params['extra'] = 'foo'; + return $params; + }, + 'expect_stored' => true, + 'expected_status' => 200, + ), + 'extended_but_unset' => array( + 'set_up' => function () use ( $add_root_extra_property ): array { + $add_root_extra_property( 'unset_prop' ); add_filter( - 'od_url_metric_schema_root_additional_properties', - static function ( array $properties ): array { - $properties['extra'] = array( - 'type' => 'string', - ); - return $properties; + 'od_url_metric_storage_validity', + static function ( $validity, OD_URL_Metric $url_metric ) { + $url_metric->unset( 'extra' ); + return $validity; + }, + 10, + 2 + ); + $params = $this->get_valid_params(); + $params['unset_prop'] = 'bar'; + return $params; + }, + 'expect_stored' => true, + 'expected_status' => 200, + ), + 'extended_and_rejected_by_returning_false' => array( + 'set_up' => function () use ( $add_root_extra_property ): array { + $add_root_extra_property( 'extra' ); + add_filter( 'od_url_metric_storage_validity', '__return_false' ); + + $params = $this->get_valid_params(); + $params['extra'] = 'foo'; + return $params; + }, + 'expect_stored' => false, + 'expected_status' => 400, + ), + 'extended_and_rejected_by_returning_wp_error' => array( + 'set_up' => function () use ( $add_root_extra_property ): array { + $add_root_extra_property( 'extra' ); + add_filter( + 'od_url_metric_storage_validity', + static function () { + return new WP_Error( 'rejected', 'Rejected!' ); } ); @@ -50,9 +103,40 @@ static function ( array $properties ): array { $params['extra'] = 'foo'; return $params; }, + 'expect_stored' => false, + 'expected_status' => 400, ), - 'with_cache_purge_post_id' => array( - 'set_up' => function (): array { + 'rejected_by_returning_wp_error_with_forbidden_status' => array( + 'set_up' => function (): array { + add_filter( + 'od_url_metric_storage_validity', + static function () { + return new WP_Error( 'forbidden', 'Forbidden!', array( 'status' => 403 ) ); + } + ); + return $this->get_valid_params(); + }, + 'expect_stored' => false, + 'expected_status' => 403, + ), + 'rejected_by_exception' => array( + 'set_up' => function (): array { + add_filter( + 'od_url_metric_storage_validity', + static function ( $validity, OD_URL_Metric $url_metric ) { + $url_metric->unset( 'viewport' ); + return $validity; + }, + 10, + 2 + ); + return $this->get_valid_params(); + }, + 'expect_stored' => false, + 'expected_status' => 500, + ), + 'with_cache_purge_post_id' => array( + 'set_up' => function (): array { $params = $this->get_valid_params(); $params['cache_purge_post_id'] = self::factory()->post->create(); $params['url'] = get_permalink( $params['cache_purge_post_id'] ); @@ -60,6 +144,8 @@ static function ( array $properties ): array { $params['hmac'] = od_get_url_metrics_storage_hmac( $params['slug'], $params['current_etag'], $params['url'], $params['cache_purge_post_id'] ); return $params; }, + 'expect_stored' => true, + 'expected_status' => 200, ), ); } @@ -73,7 +159,25 @@ static function ( array $properties ): array { * @covers ::od_handle_rest_request * @covers ::od_trigger_page_cache_invalidation */ - public function test_rest_request_good_params( Closure $set_up ): void { + public function test_rest_request_good_params( Closure $set_up, bool $expect_stored, int $expected_status ): void { + $filtered_url_metric_obj = null; + $filtered_url_metric_data = null; + $filter_called = 0; + add_filter( + 'od_url_metric_storage_validity', + function ( $validity, $url_metric, $url_metric_data ) use ( &$filter_called, &$filtered_url_metric_obj, &$filtered_url_metric_data ) { + $this->assertTrue( $validity ); + $this->assertInstanceOf( OD_URL_Metric::class, $url_metric ); + $this->assertIsArray( $url_metric_data ); + $filtered_url_metric_obj = $url_metric; + $filtered_url_metric_data = $url_metric_data; + $filter_called++; + return $validity; + }, + 0, + 3 + ); + $stored_context = null; add_action( 'od_url_metric_stored', @@ -96,38 +200,51 @@ function ( OD_URL_Metric_Store_Request_Context $context ) use ( &$stored_context $this->assertCount( 0, get_posts( array( 'post_type' => OD_URL_Metrics_Post_Type::SLUG ) ) ); $request = $this->create_request( $valid_params ); $response = rest_get_server()->dispatch( $request ); - $this->assertSame( 200, $response->get_status(), 'Response: ' . wp_json_encode( $response ) ); - - $data = $response->get_data(); - $this->assertTrue( $data['success'] ); - - $this->assertCount( 1, get_posts( array( 'post_type' => OD_URL_Metrics_Post_Type::SLUG ) ) ); - $post = OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ); - $this->assertInstanceOf( WP_Post::class, $post ); - - $url_metrics = OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ); - $this->assertCount( 1, $url_metrics, 'Expected number of URL Metrics stored.' ); - $this->assertSame( $valid_params['elements'], $this->get_array_json_data( $url_metrics[0]->get( 'elements' ) ) ); - $this->assertSame( $valid_params['viewport']['width'], $url_metrics[0]->get_viewport_width() ); - - $expected_data = $valid_params; - unset( $expected_data['hmac'], $expected_data['slug'], $expected_data['current_etag'], $expected_data['cache_purge_post_id'] ); - $this->assertSame( - $expected_data, - wp_array_slice_assoc( $url_metrics[0]->jsonSerialize(), array_keys( $expected_data ) ) - ); - $this->assertSame( 1, did_action( 'od_url_metric_stored' ) ); - $this->assertInstanceOf( OD_URL_Metric_Store_Request_Context::class, $stored_context ); + $this->assertSame( 1, $filter_called ); + $this->assertSame( $expect_stored ? 1 : 0, did_action( 'od_url_metric_stored' ) ); - // Now check that od_trigger_page_cache_invalidation() cleaned caches as expected. - $this->assertSame( $url_metrics[0]->jsonSerialize(), $stored_context->url_metric->jsonSerialize() ); - $cache_purge_post_id = $stored_context->request->get_param( 'cache_purge_post_id' ); + $this->assertSame( $expected_status, $response->get_status(), 'Response: ' . wp_json_encode( $response ) ); + $data = $response->get_data(); + $this->assertCount( $expect_stored ? 1 : 0, get_posts( array( 'post_type' => OD_URL_Metrics_Post_Type::SLUG ) ) ); + + if ( ! $expect_stored ) { + $this->assertArrayHasKey( 'code', $data ); + $this->assertArrayHasKey( 'message', $data ); + $this->assertNull( $stored_context ); + $this->assertNull( OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ) ); + } else { + $this->assertTrue( $data['success'] ); + + $post = OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ); + $this->assertInstanceOf( WP_Post::class, $post ); + + $url_metrics = OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ); + $this->assertCount( 1, $url_metrics, 'Expected number of URL Metrics stored.' ); + $this->assertSame( $valid_params['elements'], $this->get_array_json_data( $url_metrics[0]->get( 'elements' ) ) ); + $this->assertSame( $valid_params['viewport']['width'], $url_metrics[0]->get_viewport_width() ); + + $expected_data = $valid_params; + unset( $expected_data['hmac'], $expected_data['slug'], $expected_data['current_etag'], $expected_data['cache_purge_post_id'] ); + unset( $expected_data['unset_prop'] ); + $this->assertSame( + $expected_data, + wp_array_slice_assoc( $url_metrics[0]->jsonSerialize(), array_keys( $expected_data ) ) + ); - if ( isset( $valid_params['cache_purge_post_id'] ) ) { - $scheduled = wp_next_scheduled( 'od_trigger_page_cache_invalidation', array( $valid_params['cache_purge_post_id'] ) ); - $this->assertIsInt( $scheduled ); - $this->assertGreaterThan( time(), $scheduled ); + $this->assertInstanceOf( OD_URL_Metric_Store_Request_Context::class, $stored_context ); + $this->assertSame( $stored_context->url_metric, $filtered_url_metric_obj ); + $this->assertSame( $stored_context->url_metric->jsonSerialize(), $filtered_url_metric_data ); + + // Now check that od_trigger_page_cache_invalidation() cleaned caches as expected. + $this->assertSame( $url_metrics[0]->jsonSerialize(), $stored_context->url_metric->jsonSerialize() ); + if ( isset( $valid_params['cache_purge_post_id'] ) ) { + $cache_purge_post_id = $stored_context->request->get_param( 'cache_purge_post_id' ); + $this->assertSame( $valid_params['cache_purge_post_id'], $cache_purge_post_id ); + $scheduled = wp_next_scheduled( 'od_trigger_page_cache_invalidation', array( $cache_purge_post_id ) ); + $this->assertIsInt( $scheduled ); + $this->assertGreaterThan( time(), $scheduled ); + } } } From e40b3f177de220fc6572d2a3c331cdae7a4542eb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Thu, 12 Dec 2024 23:12:11 -0800 Subject: [PATCH 09/25] Fix typo in readme --- plugins/optimization-detective/readme.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index c1530f56f..f0a8cc147 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -250,7 +250,7 @@ When the ETag for URL Metrics in a complete viewport group no longer matches the Filters whether a URL Metric is valid for storage. -Three paramters are passed to this filter: +Three parameters are passed to this filter: 1. `$validity` (`bool|WP_Error`): Validity. Invalid if false or a WP_Error with errors. 2. `$url_metric` (`OD_Strict_URL_Metric`): URL Metric, already validated against the JSON Schema. From d73f8cac4765b90cd0b7794a0596ef7f3469cc43 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sat, 14 Dec 2024 22:45:18 -0800 Subject: [PATCH 10/25] Scaffold new tests for Image Prioritizer --- .../image-prioritizer/tests/test-helper.php | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 180a2d807..91469b244 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -467,6 +467,63 @@ public function test_image_prioritizer_add_element_item_schema_properties_inputs } } + /** + * Data provider. + * + * @return array + */ + public function data_provider_to_test_image_prioritizer_validate_background_image_url(): array { + return array( + 'url_parse_error' => array( + 'set_up' => static function (): string { + return 'https:///www.example.com'; + }, + 'expect_error' => 'background_image_url_lacks_host', + ), + 'url_no_host' => array( + 'set_up' => static function (): string { + return '/foo/bar?baz=1'; + }, + 'expect_error' => 'background_image_url_lacks_host', + ), + 'url_disallowed_origin' => array( + 'set_up' => static function (): string { + return 'https://bad.example.com/foo.jpg'; + }, + 'expect_error' => 'disallowed_background_image_url_host', + ), + // TODO: Try uploading image attachment and have it point to a CDN. + // TODO: Try a URL that returns a non image Content-Type. + ); + } + + /** + * Tests image_prioritizer_validate_background_image_url(). + * + * @covers ::image_prioritizer_validate_background_image_url + * + * @dataProvider data_provider_to_test_image_prioritizer_validate_background_image_url + */ + public function test_image_prioritizer_validate_background_image_url( Closure $set_up, ?string $expect_error ): void { + $url = $set_up(); + $validity = image_prioritizer_validate_background_image_url( $url ); + if ( null === $expect_error ) { + $this->assertTrue( $validity ); + } else { + $this->assertInstanceOf( WP_Error::class, $validity ); + $this->assertSame( $expect_error, $validity->get_error_code() ); + } + } + + /** + * Tests image_prioritizer_filter_store_url_metric_validity(). + * + * @covers ::image_prioritizer_filter_store_url_metric_validity + */ + public function test_image_prioritizer_filter_store_url_metric_validity(): void { + $this->markTestIncomplete(); + } + /** * Test image_prioritizer_get_video_lazy_load_script. * From 18553eee9733442eecdf0caeda11d9840cbe0363 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 08:43:38 -0800 Subject: [PATCH 11/25] Add missing access private tags --- .../class-image-prioritizer-video-tag-visitor.php | 1 - plugins/image-prioritizer/helper.php | 13 ++++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php b/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php index 1e940fab0..4399d7a40 100644 --- a/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php +++ b/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php @@ -17,7 +17,6 @@ * Image Prioritizer: Image_Prioritizer_Video_Tag_Visitor class * * @since 0.2.0 - * * @access private */ final class Image_Prioritizer_Video_Tag_Visitor extends Image_Prioritizer_Tag_Visitor { diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 56c7f10c0..790599dec 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -14,6 +14,7 @@ * Initializes Image Prioritizer when Optimization Detective has loaded. * * @since 0.2.0 + * @access private * * @param string $optimization_detective_version Current version of the optimization detective plugin. */ @@ -52,6 +53,7 @@ static function (): void { * See {@see 'wp_head'}. * * @since 0.1.0 + * @access private */ function image_prioritizer_render_generator_meta_tag(): void { // Use the plugin slug as it is immutable. @@ -62,6 +64,7 @@ function image_prioritizer_render_generator_meta_tag(): void { * Registers tag visitors. * * @since 0.1.0 + * @access private * * @param OD_Tag_Visitor_Registry $registry Tag visitor registry. */ @@ -81,6 +84,7 @@ function image_prioritizer_register_tag_visitors( OD_Tag_Visitor_Registry $regis * Filters the list of Optimization Detective extension module URLs to include the extension for Image Prioritizer. * * @since n.e.x.t + * @access private * * @param string[]|mixed $extension_module_urls Extension module URLs. * @return string[] Extension module URLs. @@ -97,6 +101,7 @@ function image_prioritizer_filter_extension_module_urls( $extension_module_urls * Filters additional properties for the element item schema for Optimization Detective. * * @since n.e.x.t + * @access private * * @param array $additional_properties Additional properties. * @return array Additional properties. @@ -141,6 +146,7 @@ function image_prioritizer_add_element_item_schema_properties( array $additional * Validates URL for a background image. * * @since n.e.x.t + * @access private * * @param string $url Background image URL. * @return true|WP_Error Validity. @@ -238,7 +244,8 @@ static function ( $host ) { * This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background * image URL is not valid. Removal of the property is preferable to * - * @since n.e.x.t + * @since n.e.x.t + * @access private * * @param bool|WP_Error|mixed $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. @@ -273,6 +280,7 @@ function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Stric * Gets the path to a script or stylesheet. * * @since n.e.x.t + * @access private * * @param string $src_path Source path, relative to plugin root. * @param string|null $min_path Minified path. If not supplied, then '.min' is injected before the file extension in the source path. @@ -319,6 +327,7 @@ function image_prioritizer_get_asset_path( string $src_path, ?string $min_path = * Handles 'autoplay' and 'preload' attributes accordingly. * * @since 0.2.0 + * @access private * * @return string Lazy load script. */ @@ -333,6 +342,7 @@ function image_prioritizer_get_video_lazy_load_script(): string { * Load the background image when it approaches the viewport using an IntersectionObserver. * * @since n.e.x.t + * @access private * * @return string Lazy load script. */ @@ -345,6 +355,7 @@ function image_prioritizer_get_lazy_load_bg_image_script(): string { * Gets the stylesheet to lazy-load background images. * * @since n.e.x.t + * @access private * * @return string Lazy load stylesheet. */ From b7b1a4769e7d6b09cd2642d355f899cf3487d7b8 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 13:39:25 -0800 Subject: [PATCH 12/25] Add tests for various validity conditions for external BG images --- plugins/image-prioritizer/helper.php | 4 +- .../image-prioritizer/tests/test-helper.php | 175 +++++++++++++++++- 2 files changed, 172 insertions(+), 7 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 790599dec..47751cb43 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -194,7 +194,7 @@ static function ( $host ) { sprintf( /* translators: %s is the list of allowed hosts */ __( 'Background image URL host is not among allowed: %s.', 'image-prioritizer' ), - join( ', ', $allowed_hosts ) + join( ', ', array_unique( $allowed_hosts ) ) ) ); } @@ -222,7 +222,7 @@ static function ( $host ) { } // Validate that the Content-Type is an image. - $content_type = (array) wp_remote_retrieve_header( $r, 'Content-Type' ); + $content_type = (array) wp_remote_retrieve_header( $r, 'content-type' ); if ( ! is_string( $content_type[0] ) || ! str_starts_with( $content_type[0], 'image/' ) ) { return new WP_Error( 'background_image_response_not_image', diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 91469b244..322ef6060 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -474,26 +474,191 @@ public function test_image_prioritizer_add_element_item_schema_properties_inputs */ public function data_provider_to_test_image_prioritizer_validate_background_image_url(): array { return array( - 'url_parse_error' => array( + 'bad_url_parse_error' => array( 'set_up' => static function (): string { return 'https:///www.example.com'; }, 'expect_error' => 'background_image_url_lacks_host', ), - 'url_no_host' => array( + 'bad_url_no_host' => array( 'set_up' => static function (): string { return '/foo/bar?baz=1'; }, 'expect_error' => 'background_image_url_lacks_host', ), - 'url_disallowed_origin' => array( + + 'bad_url_disallowed_origin' => array( 'set_up' => static function (): string { return 'https://bad.example.com/foo.jpg'; }, 'expect_error' => 'disallowed_background_image_url_host', ), - // TODO: Try uploading image attachment and have it point to a CDN. - // TODO: Try a URL that returns a non image Content-Type. + + 'good_other_origin_via_allowed_http_origins_filter' => array( + 'set_up' => static function (): string { + $image_url = 'https://other-origin.example.com/foo.jpg'; + + add_filter( + 'allowed_http_origins', + static function ( array $allowed_origins ): array { + $allowed_origins[] = 'https://other-origin.example.com'; + return $allowed_origins; + } + ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $image_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $image_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'image/jpeg', + 'content-length' => '288449', + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + return $image_url; + }, + 'expect_error' => null, + ), + + 'good_url_allowed_cdn_origin' => array( + 'set_up' => function (): string { + $attachment_id = self::factory()->attachment->create_upload_object( TESTS_PLUGIN_DIR . '/tests/data/images/car.jpeg' ); + $this->assertIsInt( $attachment_id ); + + add_filter( + 'wp_get_attachment_image_src', + static function ( $src ): array { + $src[0] = preg_replace( '#^https?://#i', 'https://my-image-cdn.example.com/', $src[0] ); + return $src; + } + ); + + $src = wp_get_attachment_image_src( $attachment_id, 'large' ); + $this->assertIsArray( $src ); + $this->assertStringStartsWith( 'https://my-image-cdn.example.com/', $src[0] ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $src ) { + if ( 'HEAD' !== $parsed_args['method'] || $src[0] !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'image/jpeg', + 'content-length' => '288449', + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + return $src[0]; + }, + 'expect_error' => null, + ), + + 'bad_not_found' => array( + 'set_up' => static function (): string { + $image_url = home_url( '/bad.jpg' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $image_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $image_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'text/html', + 'content-length' => 1000, + ), + 'body' => '', + 'response' => array( + 'code' => 404, + 'message' => 'Not Found', + ), + ); + }, + 10, + 3 + ); + + return $image_url; + }, + 'expect_error' => 'background_image_response_not_ok', + ), + + 'bad_content_type' => array( + 'set_up' => static function (): string { + $video_url = home_url( '/bad.mp4' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $video_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $video_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'video/mp4', + 'content-length' => '288449000', + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + return $video_url; + }, + 'expect_error' => 'background_image_response_not_image', + ), + + 'bad_redirect' => array( + 'set_up' => static function (): string { + $redirect_url = home_url( '/redirect.jpg' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $redirect_url ) { + if ( $redirect_url === $url ) { + return new WP_Error( 'http_request_failed', 'Too many redirects.' ); + } + return $pre; + }, + 10, + 3 + ); + + return $redirect_url; + }, + 'expect_error' => 'http_request_failed', + ), ); } From 537323340800afcd83c735d20788339ef8c8ec01 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 14:00:39 -0800 Subject: [PATCH 13/25] Fix handling of invalid external BG image and add tests --- plugins/image-prioritizer/helper.php | 6 +- .../image-prioritizer/tests/test-helper.php | 169 +++++++++++++++++- 2 files changed, 170 insertions(+), 5 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 47751cb43..ba7a88800 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -261,14 +261,14 @@ function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Stric $data = $url_metric->get( 'lcpElementExternalBackgroundImage' ); if ( is_array( $data ) && isset( $data['url'] ) && is_string( $data['url'] ) ) { // Note: The isset() and is_string() checks aren't necessary since the JSON Schema enforces them to be set. - $validity = image_prioritizer_validate_background_image_url( $data['url'] ); - if ( is_wp_error( $validity ) ) { + $image_validity = image_prioritizer_validate_background_image_url( $data['url'] ); + if ( is_wp_error( $image_validity ) ) { /** * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. * * @noinspection PhpUnhandledExceptionInspection */ - wp_trigger_error( __FUNCTION__, $validity->get_error_message() . ' Background image URL: ' . $data['url'] ); + wp_trigger_error( __FUNCTION__, $image_validity->get_error_message() . ' Background image URL: ' . $data['url'] ); $url_metric->unset( 'lcpElementExternalBackgroundImage' ); } } diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 322ef6060..7d3d3a5db 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -659,6 +659,37 @@ static function ( $pre, $parsed_args, $url ) use ( $redirect_url ) { }, 'expect_error' => 'http_request_failed', ), + + 'good_same_origin' => array( + 'set_up' => static function (): string { + $image_url = home_url( '/good.jpg' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $image_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $image_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'image/jpeg', + 'content-length' => '288449', + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + return $image_url; + }, + 'expect_error' => null, + ), ); } @@ -680,13 +711,147 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s } } + /** + * Data provider. + * + * @return array + */ + public function data_provider_to_test_image_prioritizer_filter_store_url_metric_validity(): array { + return array( + 'pass_through_true' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( true, $url_metric ); + }, + 'assert' => function ( $value ): void { + $this->assertTrue( $value ); + }, + ), + + 'pass_through_false' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( false, $url_metric ); + }, + 'assert' => function ( $value ): void { + $this->assertFalse( $value ); + }, + ), + + 'pass_through_truthy_string' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( 'so true', $url_metric ); + }, + 'assert' => function ( $value ): void { + $this->assertTrue( $value ); + }, + ), + + 'pass_through_falsy_string' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( '', $url_metric ); + }, + 'assert' => function ( $value ): void { + $this->assertFalse( $value ); + }, + ), + + 'pass_through_wp_error' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( new WP_Error( 'bad', 'Evil' ), $url_metric ); + }, + 'assert' => function ( $value ): void { + $this->assertInstanceOf( WP_Error::class, $value ); + $this->assertSame( 'bad', $value->get_error_code() ); + }, + ), + + 'invalid_external_bg_image' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $sample_url_metric_data['lcpElementExternalBackgroundImage'] = array( + 'url' => 'https://bad-origin.example.com/image.jpg', + 'tag' => 'DIV', + 'id' => null, + 'class' => null, + ); + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( true, $url_metric ); + }, + 'assert' => function ( $value, OD_Strict_URL_Metric $url_metric ): void { + $this->assertTrue( $value ); + $this->assertNull( $url_metric->get( 'lcpElementExternalBackgroundImage' ) ); + }, + ), + + 'valid_external_bg_image' => array( + 'set_up' => static function ( array $sample_url_metric_data ): array { + $image_url = home_url( '/good.jpg' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $image_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $image_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'image/jpeg', + 'content-length' => '288449', + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + $sample_url_metric_data['lcpElementExternalBackgroundImage'] = array( + 'url' => $image_url, + 'tag' => 'DIV', + 'id' => null, + 'class' => null, + ); + $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); + return array( true, $url_metric ); + }, + 'assert' => function ( $value, OD_Strict_URL_Metric $url_metric ): void { + $this->assertTrue( $value ); + $this->assertIsArray( $url_metric->get( 'lcpElementExternalBackgroundImage' ) ); + $this->assertSame( + array( + 'url' => home_url( '/good.jpg' ), + 'tag' => 'DIV', + 'id' => null, + 'class' => null, + ), + $url_metric->get( 'lcpElementExternalBackgroundImage' ) + ); + }, + ), + ); + } + /** * Tests image_prioritizer_filter_store_url_metric_validity(). * + * @dataProvider data_provider_to_test_image_prioritizer_filter_store_url_metric_validity + * * @covers ::image_prioritizer_filter_store_url_metric_validity + * @covers ::image_prioritizer_validate_background_image_url */ - public function test_image_prioritizer_filter_store_url_metric_validity(): void { - $this->markTestIncomplete(); + public function test_image_prioritizer_filter_store_url_metric_validity( Closure $set_up, Closure $assert ): void { + $sample_url_metric_data = $this->get_sample_url_metric( array() )->jsonSerialize(); + list( $validity, $url_metric ) = $set_up( $sample_url_metric_data ); + + $validity = image_prioritizer_filter_store_url_metric_validity( $validity, $url_metric ); + $assert( $validity, $url_metric ); } /** From bcefd693e3dc4944681418a6f8865220f397f0b7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 14:11:36 -0800 Subject: [PATCH 14/25] Avoid preloading background images larger than 2MB --- plugins/image-prioritizer/helper.php | 25 ++++++++++++++- .../image-prioritizer/tests/test-helper.php | 31 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index ba7a88800..e80b8f41c 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -234,7 +234,30 @@ static function ( $host ) { ); } - // TODO: Check for the Content-Length and return invalid if it is gigantic? + /* + * Validate that the Content-Length is not too massive, as it would be better to err on the side of + * not preloading something so weighty in case the image won't actually end up as LCP. + * The value of 2MB is chosen because according to Web Almanac 2022, the largest image by byte size + * on a page is 1MB at the 90th percentile: . + * The 2MB value is double this 1MB size. + */ + $content_length = (array) wp_remote_retrieve_header( $r, 'content-length' ); + if ( ! is_numeric( $content_length[0] ) ) { + return new WP_Error( + 'background_image_content_length_unknown', + __( 'HEAD request for background image URL did not include a Content-Length response header.', 'image-prioritizer' ) + ); + } elseif ( (int) $content_length[0] > 2 * MB_IN_BYTES ) { + return new WP_Error( + 'background_image_content_length_too_large', + sprintf( + /* translators: %s is the content length of the response */ + __( 'HEAD request for background image URL returned Content-Length greater than 2MB: %s.', 'image-prioritizer' ), + $content_length[0] + ) + ); + } + return true; } diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 7d3d3a5db..45e91cccd 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -639,6 +639,37 @@ static function ( $pre, $parsed_args, $url ) use ( $video_url ) { 'expect_error' => 'background_image_response_not_image', ), + 'bad_content_length' => array( + 'set_up' => static function (): string { + $image_url = home_url( '/massive-image.jpg' ); + + add_filter( + 'pre_http_request', + static function ( $pre, $parsed_args, $url ) use ( $image_url ) { + if ( 'HEAD' !== $parsed_args['method'] || $image_url !== $url ) { + return $pre; + } + return array( + 'headers' => array( + 'content-type' => 'image/jpeg', + 'content-length' => (string) ( 2 * MB_IN_BYTES + 1 ), + ), + 'body' => '', + 'response' => array( + 'code' => 200, + 'message' => 'OK', + ), + ); + }, + 10, + 3 + ); + + return $image_url; + }, + 'expect_error' => 'background_image_content_length_too_large', + ), + 'bad_redirect' => array( 'set_up' => static function (): string { $redirect_url = home_url( '/redirect.jpg' ); From 6005f4a9f084825edc4f2eeeb4152b41b29ef0ce Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Sun, 15 Dec 2024 14:24:48 -0800 Subject: [PATCH 15/25] Update readme to relate features of Image Prioritizer --- plugins/image-prioritizer/readme.txt | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/plugins/image-prioritizer/readme.txt b/plugins/image-prioritizer/readme.txt index 37ea1fb91..029fe7438 100644 --- a/plugins/image-prioritizer/readme.txt +++ b/plugins/image-prioritizer/readme.txt @@ -15,13 +15,19 @@ This plugin optimizes the loading of images (and videos) with prioritization, la The current optimizations include: -1. Ensure `fetchpriority=high` is only added to an `IMG` when it is the Largest Contentful Paint (LCP) element across all responsive breakpoints. -2. Add breakpoint-specific `fetchpriority=high` preload links for the LCP elements which are `IMG` elements or elements with a CSS `background-image` inline style. -3. Apply lazy-loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. (Additionally, [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is then also correctly applied.) -4. Implement lazy-loading of CSS background images added via inline `style` attributes. -5. Add `fetchpriority=low` to `IMG` tags which appear in the initial viewport but are not visible, such as when they are subsequent carousel slides. +1. Add breakpoint-specific `fetchpriority=high` preload links (`LINK[rel=preload]`) for image URLs of LCP elements: + 1. An `IMG` element, including the `srcset`/`sizes` attributes supplied as `imagesrcset`/`imagesizes` on the `LINK`. + 2. The first `SOURCE` element with a `type` attribute in a `PICTURE` element. (Art-directed `PICTURE` elements using media queries are not supported.) + 3. An element with a CSS `background-image` inline `style` attribute. + 4. An element with a CSS `background-image` applied with a stylesheet (when the image is from an allowed origin). +2. Ensure `fetchpriority=high` is only added to an `IMG` when it is the Largest Contentful Paint (LCP) element across all responsive breakpoints. +3. Add `fetchpriority=low` to `IMG` tags which appear in the initial viewport but are not visible, such as when they are subsequent carousel slides. +4. Lazy-Loading: + 1. Apply lazy-loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. (Additionally, [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is then also correctly applied.) + 2. Implement lazy-loading of CSS background images added via inline `style` attributes. + 3. Lazy-load `VIDEO` tags by setting the appropriate attributes based on whether they appear in the initial viewport. If a `VIDEO` is the LCP element, it gets `preload=auto`; if it is in an initial viewport, the `preload=metadata` default is left; if it is not in an initial viewport, it gets `preload=none`. Lazy-loaded videos also get initial `preload`, `autoplay`, and `poster` attributes restored when the `VIDEO` is going to enter the viewport. +5. Ensure that `sizes=auto` is added to all lazy-loaded `IMG` elements. 6. Reduce the size of the `poster` image of a `VIDEO` from full size to the size appropriate for the maximum width of the video (on desktop). -7. Lazy-load `VIDEO` tags by setting the appropriate attributes based on whether they appear in the initial viewport. If a `VIDEO` is the LCP element, it gets `preload=auto`; if it is in an initial viewport, the `preload=metadata` default is left; if it is not in an initial viewport, it gets `preload=none`. Lazy-loaded videos also get initial `preload`, `autoplay`, and `poster` attributes restored when the `VIDEO` is going to enter the viewport. **This plugin requires the [Optimization Detective](https://wordpress.org/plugins/optimization-detective/) plugin as a dependency.** Please refer to that plugin for additional background on how this plugin works as well as additional developer options. From 250f094c129ea3968722f84b7c96040fb9fc6f5d Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Dec 2024 10:30:04 -0800 Subject: [PATCH 16/25] Replace validity filter with sanitization filter; unset unset() --- plugins/image-prioritizer/helper.php | 23 ++-- plugins/image-prioritizer/hooks.php | 2 +- .../image-prioritizer/tests/test-helper.php | 118 ++++++------------ .../image-prioritizer/tests/test-hooks.php | 2 +- .../class-od-element.php | 45 +------ .../class-od-url-metric.php | 27 ---- plugins/optimization-detective/readme.txt | 14 +-- .../storage/rest-api.php | 111 +++++++--------- .../tests/storage/test-rest-api.php | 99 ++++----------- .../tests/test-class-od-element.php | 73 ++--------- .../tests/test-class-od-url-metric.php | 25 ---- ...-class-od-url-metrics-group-collection.php | 49 +------- 12 files changed, 145 insertions(+), 443 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index e80b8f41c..9b1c72c15 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -270,33 +270,30 @@ static function ( $host ) { * @since n.e.x.t * @access private * - * @param bool|WP_Error|mixed $validity Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. - * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. - * @return bool|WP_Error Validity. Valid if true or a WP_Error without any errors, or invalid otherwise. + * @param array|mixed $data URL Metric data. + * @return array Sanitized URL Metric data. * * @noinspection PhpDocMissingThrowsInspection - * @throws OD_Data_Validation_Exception Except it won't because lcpElementExternalBackgroundImage is not a required property. */ -function image_prioritizer_filter_store_url_metric_validity( $validity, OD_Strict_URL_Metric $url_metric ) { - if ( ! is_bool( $validity ) && ! ( $validity instanceof WP_Error ) ) { - $validity = (bool) $validity; +function image_prioritizer_filter_url_metric_data_pre_storage( $data ): array { + if ( ! is_array( $data ) ) { + $data = array(); } - $data = $url_metric->get( 'lcpElementExternalBackgroundImage' ); - if ( is_array( $data ) && isset( $data['url'] ) && is_string( $data['url'] ) ) { // Note: The isset() and is_string() checks aren't necessary since the JSON Schema enforces them to be set. - $image_validity = image_prioritizer_validate_background_image_url( $data['url'] ); + if ( isset( $data['lcpElementExternalBackgroundImage']['url'] ) && is_string( $data['lcpElementExternalBackgroundImage']['url'] ) ) { + $image_validity = image_prioritizer_validate_background_image_url( $data['lcpElementExternalBackgroundImage']['url'] ); if ( is_wp_error( $image_validity ) ) { /** * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. * * @noinspection PhpUnhandledExceptionInspection */ - wp_trigger_error( __FUNCTION__, $image_validity->get_error_message() . ' Background image URL: ' . $data['url'] ); - $url_metric->unset( 'lcpElementExternalBackgroundImage' ); + wp_trigger_error( __FUNCTION__, $image_validity->get_error_message() . ' Background image URL: ' . $data['lcpElementExternalBackgroundImage']['url'] ); + unset( $data['lcpElementExternalBackgroundImage'] ); } } - return $validity; + return $data; } /** diff --git a/plugins/image-prioritizer/hooks.php b/plugins/image-prioritizer/hooks.php index fbfe8c6b3..63420cd6d 100644 --- a/plugins/image-prioritizer/hooks.php +++ b/plugins/image-prioritizer/hooks.php @@ -13,4 +13,4 @@ add_action( 'od_init', 'image_prioritizer_init' ); add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ); add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ); -add_filter( 'od_url_metric_storage_validity', 'image_prioritizer_filter_store_url_metric_validity', 10, 2 ); +add_filter( 'od_url_metric_data_pre_storage', 'image_prioritizer_filter_url_metric_data_pre_storage' ); diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 45e91cccd..05de49635 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -747,78 +747,34 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s * * @return array */ - public function data_provider_to_test_image_prioritizer_filter_store_url_metric_validity(): array { + public function data_provider_to_test_image_prioritizer_filter_url_metric_data_pre_storage(): array { return array( - 'pass_through_true' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( true, $url_metric ); - }, - 'assert' => function ( $value ): void { - $this->assertTrue( $value ); - }, - ), - - 'pass_through_false' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( false, $url_metric ); - }, - 'assert' => function ( $value ): void { - $this->assertFalse( $value ); - }, - ), - - 'pass_through_truthy_string' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( 'so true', $url_metric ); - }, - 'assert' => function ( $value ): void { - $this->assertTrue( $value ); - }, - ), - - 'pass_through_falsy_string' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( '', $url_metric ); - }, - 'assert' => function ( $value ): void { - $this->assertFalse( $value ); - }, - ), - - 'pass_through_wp_error' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( new WP_Error( 'bad', 'Evil' ), $url_metric ); - }, - 'assert' => function ( $value ): void { - $this->assertInstanceOf( WP_Error::class, $value ); - $this->assertSame( 'bad', $value->get_error_code() ); - }, - ), - - 'invalid_external_bg_image' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { - $sample_url_metric_data['lcpElementExternalBackgroundImage'] = array( + 'invalid_external_bg_image' => array( + 'set_up' => static function ( array $url_metric_data ): array { + $url_metric_data['lcpElementExternalBackgroundImage'] = array( 'url' => 'https://bad-origin.example.com/image.jpg', 'tag' => 'DIV', 'id' => null, 'class' => null, ); - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( true, $url_metric ); + $url_metric_data['viewport']['width'] = 10101; + $url_metric_data['viewport']['height'] = 20202; + return $url_metric_data; }, - 'assert' => function ( $value, OD_Strict_URL_Metric $url_metric ): void { - $this->assertTrue( $value ); - $this->assertNull( $url_metric->get( 'lcpElementExternalBackgroundImage' ) ); + 'assert' => function ( array $url_metric_data ): void { + $this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data ); + $this->assertSame( + array( + 'width' => 10101, + 'height' => 20202, + ), + $url_metric_data['viewport'] + ); }, ), - 'valid_external_bg_image' => array( - 'set_up' => static function ( array $sample_url_metric_data ): array { + 'valid_external_bg_image' => array( + 'set_up' => static function ( array $url_metric_data ): array { $image_url = home_url( '/good.jpg' ); add_filter( @@ -843,18 +799,20 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { 3 ); - $sample_url_metric_data['lcpElementExternalBackgroundImage'] = array( + $url_metric_data['lcpElementExternalBackgroundImage'] = array( 'url' => $image_url, 'tag' => 'DIV', 'id' => null, 'class' => null, ); - $url_metric = new OD_Strict_URL_Metric( $sample_url_metric_data ); - return array( true, $url_metric ); + + $url_metric_data['viewport']['width'] = 30303; + $url_metric_data['viewport']['height'] = 40404; + return $url_metric_data; }, - 'assert' => function ( $value, OD_Strict_URL_Metric $url_metric ): void { - $this->assertTrue( $value ); - $this->assertIsArray( $url_metric->get( 'lcpElementExternalBackgroundImage' ) ); + 'assert' => function ( array $url_metric_data ): void { + $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data ); + $this->assertIsArray( $url_metric_data['lcpElementExternalBackgroundImage'] ); $this->assertSame( array( 'url' => home_url( '/good.jpg' ), @@ -862,7 +820,14 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { 'id' => null, 'class' => null, ), - $url_metric->get( 'lcpElementExternalBackgroundImage' ) + $url_metric_data['lcpElementExternalBackgroundImage'] + ); + $this->assertSame( + array( + 'width' => 30303, + 'height' => 40404, + ), + $url_metric_data['viewport'] ); }, ), @@ -870,19 +835,18 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { } /** - * Tests image_prioritizer_filter_store_url_metric_validity(). + * Tests image_prioritizer_filter_url_metric_data_pre_storage(). * - * @dataProvider data_provider_to_test_image_prioritizer_filter_store_url_metric_validity + * @dataProvider data_provider_to_test_image_prioritizer_filter_url_metric_data_pre_storage * - * @covers ::image_prioritizer_filter_store_url_metric_validity + * @covers ::image_prioritizer_filter_url_metric_data_pre_storage * @covers ::image_prioritizer_validate_background_image_url */ - public function test_image_prioritizer_filter_store_url_metric_validity( Closure $set_up, Closure $assert ): void { - $sample_url_metric_data = $this->get_sample_url_metric( array() )->jsonSerialize(); - list( $validity, $url_metric ) = $set_up( $sample_url_metric_data ); + public function test_image_prioritizer_filter_url_metric_data_pre_storage( Closure $set_up, Closure $assert ): void { + $url_metric_data = $set_up( $this->get_sample_url_metric( array() )->jsonSerialize() ); - $validity = image_prioritizer_filter_store_url_metric_validity( $validity, $url_metric ); - $assert( $validity, $url_metric ); + $url_metric_data = image_prioritizer_filter_url_metric_data_pre_storage( $url_metric_data ); + $assert( $url_metric_data ); } /** diff --git a/plugins/image-prioritizer/tests/test-hooks.php b/plugins/image-prioritizer/tests/test-hooks.php index 3c2e02014..416523cb1 100644 --- a/plugins/image-prioritizer/tests/test-hooks.php +++ b/plugins/image-prioritizer/tests/test-hooks.php @@ -14,6 +14,6 @@ public function test_hooks_added(): void { $this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) ); $this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) ); $this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) ); - $this->assertEquals( 10, has_filter( 'od_url_metric_storage_validity', 'image_prioritizer_filter_store_url_metric_validity' ) ); + $this->assertEquals( 10, has_filter( 'od_url_metric_data_pre_storage', 'image_prioritizer_filter_url_metric_data_pre_storage' ) ); } } diff --git a/plugins/optimization-detective/class-od-element.php b/plugins/optimization-detective/class-od-element.php index be8400ce4..c7f243d99 100644 --- a/plugins/optimization-detective/class-od-element.php +++ b/plugins/optimization-detective/class-od-element.php @@ -208,53 +208,18 @@ public function offsetSet( $offset, $value ): void { } /** - * Unsets a property. + * Offset to unset. * - * This is particularly useful in conjunction with the `od_url_metric_schema_element_item_additional_properties` filter. - * This will throw an exception if the property is required by the schema. - * - * @since n.e.x.t - * - * @param string $key Property. + * This is disallowed. Attempting to unset a property will throw an error. * - * @throws OD_Data_Validation_Exception When attempting to unset a property required by the JSON Schema. - */ - public function unset( string $key ): void { - $schema = OD_URL_Metric::get_json_schema(); // TODO: This should be a non-static method once the URL Metric is instantiated. - if ( - isset( $schema['properties']['elements']['items']['properties'][ $key ]['required'] ) - && true === $schema['properties']['elements']['items']['properties'][ $key ]['required'] - ) { - throw new OD_Data_Validation_Exception( - esc_html( - sprintf( - /* translators: %s is the property key. */ - __( 'The %s key is required for an item of elements in a URL Metric.', 'optimization-detective' ), - $key - ) - ) - ); - } - unset( $this->data[ $key ] ); // @phpstan-ignore assign.propertyType (Above required check ensures $key is not isLCP, isLCPCandidate, xpath, intersectionRatio, intersectionRect, boundingClientRect.) - $group = $this->url_metric->get_group(); - if ( $group instanceof OD_URL_Metric_Group ) { - $group->clear_cache(); - } - } - - /** - * Unsets an offset. - * - * This will throw an exception if the offset is required by the schema. - * - * @since n.e.x.t + * @since 0.7.0 * * @param mixed $offset Offset. * - * @throws OD_Data_Validation_Exception When attempting to unset a property required by the JSON Schema. + * @throws Exception When attempting to unset a property. */ public function offsetUnset( $offset ): void { - $this->unset( (string) $offset ); + throw new Exception( 'Element data may not be unset.' ); } /** diff --git a/plugins/optimization-detective/class-od-url-metric.php b/plugins/optimization-detective/class-od-url-metric.php index b68045e6e..f4a4e25cc 100644 --- a/plugins/optimization-detective/class-od-url-metric.php +++ b/plugins/optimization-detective/class-od-url-metric.php @@ -503,33 +503,6 @@ function ( array $element ): OD_Element { return $this->elements; } - /** - * Unsets a property from the URL Metric. - * - * @since n.e.x.t - * - * @param string $key Key to unset. - * @throws OD_Data_Validation_Exception If the property is required an exception will be thrown. - */ - public function unset( string $key ): void { - $schema = self::get_json_schema(); // TODO: Consider capturing the schema as a private member variable once the URL Metric is constructed. - if ( isset( $schema['properties'][ $key ]['required'] ) && true === $schema['properties'][ $key ]['required'] ) { - throw new OD_Data_Validation_Exception( - esc_html( - sprintf( - /* translators: %s is the property key. */ - __( 'The %s key is required at the root of a URL Metric.', 'optimization-detective' ), - $key - ) - ) - ); - } - unset( $this->data[ $key ] ); // @phpstan-ignore assign.propertyType (Above required check ensures $key is not uuid, url, timestamp, viewport, or elements.) - if ( $this->group instanceof OD_URL_Metric_Group ) { - $this->group->clear_cache(); - } - } - /** * Specifies data which should be serialized to JSON. * diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index f0a8cc147..d78c4864d 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -246,19 +246,13 @@ The ETag is a unique identifier that changes whenever the underlying data used t When the ETag for URL Metrics in a complete viewport group no longer matches the current environment's ETag, new URL Metrics will then begin to be collected until there are no more stored URL Metrics with the old ETag. These new URL Metrics will include data relevant to the newly activated plugins and their tag visitors. -**Filter:** `od_url_metric_storage_validity` (default args: true) +**Filter:** `od_url_metric_data_pre_storage` (default arg: URL Metric data array) -Filters whether a URL Metric is valid for storage. +Filters the URL Metric data prior to validation for storage. -Three parameters are passed to this filter: +This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in JSON Schema. This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this filter can be more expensive, such as doing filesystem checks or HTTP requests. -1. `$validity` (`bool|WP_Error`): Validity. Invalid if false or a WP_Error with errors. -2. `$url_metric` (`OD_Strict_URL_Metric`): URL Metric, already validated against the JSON Schema. -3. `$url_metric_data` (`array`): Original URL Metric data before any mutations. - -This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is loaded from the `od_url_metrics` post type. This means that validation logic enforced via this filter can be more expensive, such as doing filesystem checks or HTTP requests. - -In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a plugin may also mutate the `OD_URL_Metric` instance passed by reference to the filter callback. This is useful for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. +To fail validation for the provided URL Metric data, an OD_Data_Validation_Exception exception should be thrown. Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. **Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`) diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 198b57284..940928769 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -186,85 +186,64 @@ function od_handle_rest_request( WP_REST_Request $request ) { OD_Storage_Lock::set_lock(); try { - // The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed. - $url_metric = new OD_Strict_URL_Metric( - array_merge( - $data, - array( - // Now supply the readonly args which were omitted from the REST API params due to being `readonly`. - 'timestamp' => microtime( true ), - 'uuid' => wp_generate_uuid4(), - 'etag' => $request->get_param( 'current_etag' ), - ) + $data = array_merge( + $data, + array( + // Now supply the readonly args which were omitted from the REST API params due to being `readonly`. + 'timestamp' => microtime( true ), + 'uuid' => wp_generate_uuid4(), + 'etag' => $request->get_param( 'current_etag' ), ) ); - } catch ( OD_Data_Validation_Exception $e ) { - return new WP_Error( - 'rest_invalid_param', - sprintf( - /* translators: %s is exception name */ - __( 'Failed to validate URL Metric: %s', 'optimization-detective' ), - $e->getMessage() - ), - array( 'status' => 400 ) - ); - } - try { /** - * Filters whether a URL Metric is valid for storage. + * Filters the URL Metric data prior to validation for storage. * - * This allows for custom validation constraints to be applied beyond what can be expressed in JSON Schema. This is - * also necessary because the 'validate_callback' key in a JSON Schema is not respected when gathering the REST API - * endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, the REST API doesn't - * support 'validate_callback' for any nested arguments in any case, meaning that custom constraints would be able - * to be applied to multidimensional objects, such as the items inside 'elements'. + * This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in + * JSON Schema. This is also necessary because the 'validate_callback' key in a JSON Schema is not respected when + * gathering the REST API endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, + * the REST API doesn't support 'validate_callback' for any nested arguments in any case, meaning that custom + * constraints would be able to be applied to multidimensional objects, such as the items inside 'elements'. * * This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is - * loaded from the od_url_metrics post type. This means that validation logic enforced via this filter can be more - * expensive, such as doing filesystem checks or HTTP requests. + * loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this + * filter can be more expensive, such as doing filesystem checks or HTTP requests. * - * In addition to having the filter return `false` or a non-empty `WP_Error` to block storing the URL Metric, a - * plugin may also mutate the OD_URL_Metric instance passed by reference to the filter callback. This is useful - * for plugins in particular to unset extended properties which couldn't be validated using JSON Schema alone. + * To fail validation for the provided URL Metric data, an OD_Data_Validation_Exception exception should be thrown. + * Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. * * @since n.e.x.t * - * @param bool|WP_Error $validity Validity. Invalid if false or a WP_Error with errors. - * @param OD_Strict_URL_Metric $url_metric URL Metric, already validated against the JSON Schema. - * @param array $url_metric_data Original URL Metric data before any mutations. + * @param array $data URL Metric data. This is the Data type from OD_URL_Metric. */ - $validity = apply_filters( 'od_url_metric_storage_validity', true, $url_metric, $url_metric->jsonSerialize() ); + $data = apply_filters( 'od_url_metric_data_pre_storage', $data ); + + // The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed. + $url_metric = new OD_Strict_URL_Metric( $data ); } catch ( Exception $e ) { - $error_data = array( - 'status' => 500, - ); - if ( WP_DEBUG ) { - $error_data['exception'] = array( - 'class' => get_class( $e ), - 'message' => $e->getMessage(), - 'code' => $e->getCode(), + $error_data = array(); + if ( $e instanceof OD_Data_Validation_Exception ) { + $error_message = sprintf( + /* translators: %s is exception name */ + __( 'Failed to validate URL Metric: %s', 'optimization-detective' ), + $e->getMessage() ); + $error_data['status'] = 400; + } else { + $error_message = sprintf( + /* translators: %s is exception name */ + __( 'Failed to validate URL Metric: %s', 'optimization-detective' ), + __( 'An unrecognized exception was thrown', 'optimization-detective' ) + ); + $error_data['status'] = 500; + if ( WP_DEBUG ) { + $error_data['exception_class'] = get_class( $e ); + $error_data['exception_message'] = $e->getMessage(); + $error_data['exception_code'] = $e->getCode(); + } } - $validity = new WP_Error( - 'exception', - sprintf( - /* translators: %s is the filter name 'od_url_metric_storage_validity' */ - __( 'An %s filter callback threw an exception.', 'optimization-detective' ), - 'od_url_metric_storage_validity' - ), - $error_data - ); - } - if ( false === (bool) $validity || ( $validity instanceof WP_Error && $validity->has_errors() ) ) { - if ( false === $validity ) { - $validity = new WP_Error( 'invalid_url_metric', __( 'Validity of URL Metric was rejected by filter.', 'optimization-detective' ) ); - } - $error_code = $validity->get_error_code(); - if ( ! isset( $validity->error_data[ $error_code ]['status'] ) ) { - $validity->error_data[ $error_code ]['status'] = 400; - } - return $validity; + + return new WP_Error( 'rest_invalid_param', $error_message, $error_data ); } // TODO: This should be changed from store_url_metric($slug, $url_metric) instead be update_post( $slug, $group_collection ). As it stands, store_url_metric() is duplicating logic here. @@ -277,8 +256,8 @@ function od_handle_rest_request( WP_REST_Request $request ) { 'status' => 500, ); if ( WP_DEBUG ) { - $error_data['code'] = $result->get_error_code(); - $error_data['message'] = $result->get_error_message(); + $error_data['error_code'] = $result->get_error_code(); + $error_data['error_message'] = $result->get_error_message(); } return new WP_Error( 'unable_to_store_url_metric', diff --git a/plugins/optimization-detective/tests/storage/test-rest-api.php b/plugins/optimization-detective/tests/storage/test-rest-api.php index 0caf8b5a3..01684eb3a 100644 --- a/plugins/optimization-detective/tests/storage/test-rest-api.php +++ b/plugins/optimization-detective/tests/storage/test-rest-api.php @@ -41,14 +41,14 @@ static function ( array $properties ) use ( $property_name ): array { }; return array( - 'not_extended' => array( + 'not_extended' => array( 'set_up' => function (): array { return $this->get_valid_params(); }, 'expect_stored' => true, 'expected_status' => 200, ), - 'extended' => array( + 'extended' => array( 'set_up' => function () use ( $add_root_extra_property ): array { $add_root_extra_property( 'extra' ); $params = $this->get_valid_params(); @@ -58,84 +58,39 @@ static function ( array $properties ) use ( $property_name ): array { 'expect_stored' => true, 'expected_status' => 200, ), - 'extended_but_unset' => array( - 'set_up' => function () use ( $add_root_extra_property ): array { - $add_root_extra_property( 'unset_prop' ); - add_filter( - 'od_url_metric_storage_validity', - static function ( $validity, OD_URL_Metric $url_metric ) { - $url_metric->unset( 'extra' ); - return $validity; - }, - 10, - 2 - ); - $params = $this->get_valid_params(); - $params['unset_prop'] = 'bar'; - return $params; - }, - 'expect_stored' => true, - 'expected_status' => 200, - ), - 'extended_and_rejected_by_returning_false' => array( - 'set_up' => function () use ( $add_root_extra_property ): array { - $add_root_extra_property( 'extra' ); - add_filter( 'od_url_metric_storage_validity', '__return_false' ); - - $params = $this->get_valid_params(); - $params['extra'] = 'foo'; - return $params; - }, - 'expect_stored' => false, - 'expected_status' => 400, - ), - 'extended_and_rejected_by_returning_wp_error' => array( - 'set_up' => function () use ( $add_root_extra_property ): array { - $add_root_extra_property( 'extra' ); - add_filter( - 'od_url_metric_storage_validity', - static function () { - return new WP_Error( 'rejected', 'Rejected!' ); - } - ); - - $params = $this->get_valid_params(); - $params['extra'] = 'foo'; - return $params; - }, - 'expect_stored' => false, - 'expected_status' => 400, - ), - 'rejected_by_returning_wp_error_with_forbidden_status' => array( + 'rejected_by_generic_exception' => array( 'set_up' => function (): array { add_filter( - 'od_url_metric_storage_validity', - static function () { - return new WP_Error( 'forbidden', 'Forbidden!', array( 'status' => 403 ) ); + 'od_url_metric_data_pre_storage', + static function ( $data ): array { + if ( count( $data ) > 0 ) { + throw new Exception( 'bad' ); + } + return $data; } ); return $this->get_valid_params(); }, 'expect_stored' => false, - 'expected_status' => 403, + 'expected_status' => 500, ), - 'rejected_by_exception' => array( + 'rejected_by_validation_exception' => array( 'set_up' => function (): array { add_filter( - 'od_url_metric_storage_validity', - static function ( $validity, OD_URL_Metric $url_metric ) { - $url_metric->unset( 'viewport' ); - return $validity; - }, - 10, - 2 + 'od_url_metric_data_pre_storage', + static function ( $data ): array { + if ( count( $data ) > 0 ) { + throw new OD_Data_Validation_Exception( 'bad' ); + } + return $data; + } ); return $this->get_valid_params(); }, 'expect_stored' => false, - 'expected_status' => 500, + 'expected_status' => 400, ), - 'with_cache_purge_post_id' => array( + 'with_cache_purge_post_id' => array( 'set_up' => function (): array { $params = $this->get_valid_params(); $params['cache_purge_post_id'] = self::factory()->post->create(); @@ -160,22 +115,17 @@ static function ( $validity, OD_URL_Metric $url_metric ) { * @covers ::od_trigger_page_cache_invalidation */ public function test_rest_request_good_params( Closure $set_up, bool $expect_stored, int $expected_status ): void { - $filtered_url_metric_obj = null; $filtered_url_metric_data = null; $filter_called = 0; add_filter( - 'od_url_metric_storage_validity', - function ( $validity, $url_metric, $url_metric_data ) use ( &$filter_called, &$filtered_url_metric_obj, &$filtered_url_metric_data ) { - $this->assertTrue( $validity ); - $this->assertInstanceOf( OD_URL_Metric::class, $url_metric ); + 'od_url_metric_data_pre_storage', + function ( $url_metric_data ) use ( &$filter_called, &$filtered_url_metric_data ) { $this->assertIsArray( $url_metric_data ); - $filtered_url_metric_obj = $url_metric; $filtered_url_metric_data = $url_metric_data; $filter_called++; - return $validity; + return $url_metric_data; }, - 0, - 3 + 0 ); $stored_context = null; @@ -233,7 +183,6 @@ function ( OD_URL_Metric_Store_Request_Context $context ) use ( &$stored_context ); $this->assertInstanceOf( OD_URL_Metric_Store_Request_Context::class, $stored_context ); - $this->assertSame( $stored_context->url_metric, $filtered_url_metric_obj ); $this->assertSame( $stored_context->url_metric->jsonSerialize(), $filtered_url_metric_data ); // Now check that od_trigger_page_cache_invalidation() cleaned caches as expected. diff --git a/plugins/optimization-detective/tests/test-class-od-element.php b/plugins/optimization-detective/tests/test-class-od-element.php index 1b980d421..52a4cc43f 100644 --- a/plugins/optimization-detective/tests/test-class-od-element.php +++ b/plugins/optimization-detective/tests/test-class-od-element.php @@ -24,7 +24,6 @@ class Test_OD_Element extends WP_UnitTestCase { * @covers ::offsetExists * @covers ::offsetGet * @covers ::offsetSet - * @covers ::unset * @covers ::offsetUnset * @covers ::jsonSerialize */ @@ -131,68 +130,22 @@ static function ( array $schema ): array { $this->assertTrue( isset( $element['customProp'] ) ); $this->assertTrue( $element->offsetExists( 'customProp' ) ); - // Try setting property (which is not currently supported). - $functions = array( - static function ( OD_Element $element ): void { - $element['isLCP'] = true; - }, - static function ( OD_Element $element ): void { - $element->offsetSet( 'isLCP', true ); - }, - ); - foreach ( $functions as $function ) { - $exception = null; - try { - $function( $element ); - } catch ( Exception $e ) { - $exception = $e; - } - $this->assertInstanceOf( Exception::class, $exception ); - $this->assertFalse( $element->get( 'isLCP' ) ); - } + $this->assertEquals( $element_data, $element->jsonSerialize() ); - // Try unsetting a required property. - $functions = array( - static function ( OD_Element $element ): void { - unset( $element['isLCP'] ); - }, - static function ( OD_Element $element ): void { - $element->unset( 'isLCP' ); - }, - static function ( OD_Element $element ): void { - $element->offsetUnset( 'isLCP' ); - }, - ); - foreach ( $functions as $function ) { - $exception = null; - try { - $function( $element ); - } catch ( Exception $e ) { - $exception = $e; - } - $this->assertInstanceOf( Exception::class, $exception ); - $this->assertArrayHasKey( 'isLCP', $element->jsonSerialize() ); + $exception = null; + try { + $element['isLCP'] = true; + } catch ( Exception $e ) { + $exception = $e; } + $this->assertInstanceOf( Exception::class, $exception ); - $this->assertEquals( $element_data, $element->jsonSerialize() ); - - // Try unsetting a non-required property. - $functions = array( - static function ( OD_Element $element ): void { - unset( $element['customProp'] ); - }, - static function ( OD_Element $element ): void { - $element->unset( 'customProp' ); - }, - static function ( OD_Element $element ): void { - $element->offsetUnset( 'customProp' ); - }, - ); - foreach ( $functions as $function ) { - $cloned_element = clone $element; - $function( $cloned_element ); - $this->assertFalse( $cloned_element->offsetExists( 'customProp' ) ); - $this->assertArrayNotHasKey( 'customProp', $cloned_element->jsonSerialize() ); + $exception = null; + try { + unset( $element['isLCP'] ); + } catch ( Exception $e ) { // @phpstan-ignore catch.neverThrown (It is thrown by offsetUnset actually.) + $exception = $e; } + $this->assertInstanceOf( Exception::class, $exception ); // @phpstan-ignore method.impossibleType (It is thrown by offsetUnset actually.) } } diff --git a/plugins/optimization-detective/tests/test-class-od-url-metric.php b/plugins/optimization-detective/tests/test-class-od-url-metric.php index df2fa0144..51b23c243 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metric.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metric.php @@ -277,7 +277,6 @@ static function ( $value ) { * @covers ::get_json_schema * @covers ::set_group * @covers ::get_group - * @covers ::unset * * @dataProvider data_provider_to_test_constructor * @@ -337,15 +336,6 @@ static function ( OD_Element $element ) { $this->assertTrue( wp_is_uuid( $url_metric->get_uuid() ) ); $this->assertSame( $url_metric->get_uuid(), $url_metric->get( 'uuid' ) ); - $exception = null; - try { - $url_metric->unset( 'elements' ); - } catch ( OD_Data_Validation_Exception $e ) { - $exception = $e; - } - $this->assertInstanceOf( OD_Data_Validation_Exception::class, $exception ); - $url_metric->unset( 'does_not_exist' ); - $serialized = $url_metric->jsonSerialize(); if ( ! array_key_exists( 'uuid', $data ) ) { $this->assertTrue( wp_is_uuid( $serialized['uuid'] ) ); @@ -407,12 +397,6 @@ static function ( array $properties ): array { $this->assertArrayHasKey( 'isTouch', $extended_data ); $this->assertTrue( $extended_data['isTouch'] ); $this->assertTrue( $extended_url_metric->get( 'isTouch' ) ); - - $this->assertTrue( $extended_url_metric->get( 'isTouch' ) ); - $extended_url_metric->unset( 'isTouch' ); - $this->assertNull( $extended_url_metric->get( 'isTouch' ) ); - $extended_data = $extended_url_metric->jsonSerialize(); - $this->assertArrayNotHasKey( 'isTouch', $extended_data ); }, ), @@ -505,13 +489,6 @@ static function ( array $properties ): array { $this->assertArrayHasKey( 'isColorful', $extended_data['elements'][0] ); $this->assertFalse( $extended_data['elements'][0]['isColorful'] ); $this->assertFalse( $extended_url_metric->get_elements()[0]['isColorful'] ); - - $element = $extended_url_metric->get_elements()[0]; - $this->assertFalse( $element->get( 'isColorful' ) ); - $element->unset( 'isColorful' ); - $this->assertNull( $element->get( 'isColorful' ) ); - $extended_data = $extended_url_metric->jsonSerialize(); - $this->assertArrayNotHasKey( 'isColorful', $extended_data['elements'][0] ); }, ), @@ -577,8 +554,6 @@ static function ( array $properties ): array { * Tests construction with extended schema. * * @covers ::get_json_schema - * @covers ::unset - * @covers OD_Element::unset * * @dataProvider data_provider_to_test_constructor_with_extended_schema * diff --git a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php index 957acd1a4..ef05fc2ab 100644 --- a/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php +++ b/plugins/optimization-detective/tests/test-class-od-url-metrics-group-collection.php @@ -204,8 +204,6 @@ public function data_provider_sample_size_and_breakpoints(): array { * * @covers ::clear_cache * @covers OD_URL_Metric_Group::clear_cache - * @covers OD_URL_Metric::unset - * @covers OD_Element::unset */ public function test_clear_cache(): void { $collection = new OD_URL_Metric_Group_Collection( array(), md5( '' ), array(), 1, DAY_IN_SECONDS ); @@ -226,58 +224,13 @@ public function test_clear_cache(): void { $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); // Test that adding a URL metric to a collection clears the caches. - add_filter( - 'od_url_metric_schema_root_additional_properties', - static function ( $schema ) { - $schema['new_prop_at_root'] = array( - 'type' => 'string', - ); - return $schema; - } - ); - add_filter( - 'od_url_metric_schema_element_additional_properties', - static function ( $schema ) { - $schema['new_prop_at_element'] = array( - 'type' => 'string', - ); - return $schema; - } - ); $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); $group_result_cache_reflection_property->setValue( $group, $populated_value ); - $collection->add_url_metric( - $this->get_sample_url_metric( - array( - 'element' => array( - 'xpath' => '/*[1][self::HTML]/*[2][self::BODY]/*[1][self::IMG]', - 'new_prop_at_element' => 'hey there', - ), - 'extended_root' => array( - 'new_prop_at_root' => 'hola', - ), - ) - ) - ); + $collection->add_url_metric( $this->get_sample_url_metric( array() ) ); $url_metric = $group->getIterator()->current(); $this->assertInstanceOf( OD_URL_Metric::class, $url_metric ); $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); - - // Test that modifying a URL Metric empties the cache of the collection and the group. - $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); - $group_result_cache_reflection_property->setValue( $group, $populated_value ); - $url_metric->unset( 'new_prop_at_root' ); - $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); - $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); - - // Test that modifying a URL Metric element empties the cache of the collection and the group. - $element = $url_metric->get_elements()[0]; - $collection_result_cache_reflection_property->setValue( $collection, $populated_value ); - $group_result_cache_reflection_property->setValue( $group, $populated_value ); - $element->unset( 'new_prop_at_element' ); - $this->assertSame( array(), $collection_result_cache_reflection_property->getValue( $collection ) ); - $this->assertSame( array(), $group_result_cache_reflection_property->getValue( $group ) ); } /** From f74f4f4b1b08dfb71b4eb0fc25d03e8859dad330 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Dec 2024 11:19:18 -0800 Subject: [PATCH 17/25] Rename filter --- plugins/image-prioritizer/helper.php | 2 +- plugins/image-prioritizer/hooks.php | 2 +- plugins/image-prioritizer/tests/test-helper.php | 12 ++++++------ plugins/image-prioritizer/tests/test-hooks.php | 2 +- plugins/optimization-detective/readme.txt | 2 +- plugins/optimization-detective/storage/rest-api.php | 2 +- .../tests/storage/test-rest-api.php | 6 +++--- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 9b1c72c15..c710ddca5 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -275,7 +275,7 @@ static function ( $host ) { * * @noinspection PhpDocMissingThrowsInspection */ -function image_prioritizer_filter_url_metric_data_pre_storage( $data ): array { +function image_prioritizer_filter_store_url_metric_data( $data ): array { if ( ! is_array( $data ) ) { $data = array(); } diff --git a/plugins/image-prioritizer/hooks.php b/plugins/image-prioritizer/hooks.php index 63420cd6d..d257ed735 100644 --- a/plugins/image-prioritizer/hooks.php +++ b/plugins/image-prioritizer/hooks.php @@ -13,4 +13,4 @@ add_action( 'od_init', 'image_prioritizer_init' ); add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ); add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ); -add_filter( 'od_url_metric_data_pre_storage', 'image_prioritizer_filter_url_metric_data_pre_storage' ); +add_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' ); diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 05de49635..8ecdd4a33 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -747,7 +747,7 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s * * @return array */ - public function data_provider_to_test_image_prioritizer_filter_url_metric_data_pre_storage(): array { + public function data_provider_to_test_image_prioritizer_filter_store_url_metric_data(): array { return array( 'invalid_external_bg_image' => array( 'set_up' => static function ( array $url_metric_data ): array { @@ -835,17 +835,17 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { } /** - * Tests image_prioritizer_filter_url_metric_data_pre_storage(). + * Tests image_prioritizer_filter_store_url_metric_data(). * - * @dataProvider data_provider_to_test_image_prioritizer_filter_url_metric_data_pre_storage + * @dataProvider data_provider_to_test_image_prioritizer_filter_store_url_metric_data * - * @covers ::image_prioritizer_filter_url_metric_data_pre_storage + * @covers ::image_prioritizer_filter_store_url_metric_data * @covers ::image_prioritizer_validate_background_image_url */ - public function test_image_prioritizer_filter_url_metric_data_pre_storage( Closure $set_up, Closure $assert ): void { + public function test_image_prioritizer_filter_store_url_metric_data( Closure $set_up, Closure $assert ): void { $url_metric_data = $set_up( $this->get_sample_url_metric( array() )->jsonSerialize() ); - $url_metric_data = image_prioritizer_filter_url_metric_data_pre_storage( $url_metric_data ); + $url_metric_data = image_prioritizer_filter_store_url_metric_data( $url_metric_data ); $assert( $url_metric_data ); } diff --git a/plugins/image-prioritizer/tests/test-hooks.php b/plugins/image-prioritizer/tests/test-hooks.php index 416523cb1..4d34043d3 100644 --- a/plugins/image-prioritizer/tests/test-hooks.php +++ b/plugins/image-prioritizer/tests/test-hooks.php @@ -14,6 +14,6 @@ public function test_hooks_added(): void { $this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) ); $this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) ); $this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) ); - $this->assertEquals( 10, has_filter( 'od_url_metric_data_pre_storage', 'image_prioritizer_filter_url_metric_data_pre_storage' ) ); + $this->assertEquals( 10, has_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' ) ); } } diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index d78c4864d..c21b3a523 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -252,7 +252,7 @@ Filters the URL Metric data prior to validation for storage. This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in JSON Schema. This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this filter can be more expensive, such as doing filesystem checks or HTTP requests. -To fail validation for the provided URL Metric data, an OD_Data_Validation_Exception exception should be thrown. Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. +To fail validation for the provided URL Metric data, an `OD_Data_Validation_Exception` exception should be thrown. Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. **Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`) diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 940928769..204a44140 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -216,7 +216,7 @@ function od_handle_rest_request( WP_REST_Request $request ) { * * @param array $data URL Metric data. This is the Data type from OD_URL_Metric. */ - $data = apply_filters( 'od_url_metric_data_pre_storage', $data ); + $data = apply_filters( 'od_store_url_metric_data', $data ); // The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed. $url_metric = new OD_Strict_URL_Metric( $data ); diff --git a/plugins/optimization-detective/tests/storage/test-rest-api.php b/plugins/optimization-detective/tests/storage/test-rest-api.php index 01684eb3a..8817e7337 100644 --- a/plugins/optimization-detective/tests/storage/test-rest-api.php +++ b/plugins/optimization-detective/tests/storage/test-rest-api.php @@ -61,7 +61,7 @@ static function ( array $properties ) use ( $property_name ): array { 'rejected_by_generic_exception' => array( 'set_up' => function (): array { add_filter( - 'od_url_metric_data_pre_storage', + 'od_store_url_metric_data', static function ( $data ): array { if ( count( $data ) > 0 ) { throw new Exception( 'bad' ); @@ -77,7 +77,7 @@ static function ( $data ): array { 'rejected_by_validation_exception' => array( 'set_up' => function (): array { add_filter( - 'od_url_metric_data_pre_storage', + 'od_store_url_metric_data', static function ( $data ): array { if ( count( $data ) > 0 ) { throw new OD_Data_Validation_Exception( 'bad' ); @@ -118,7 +118,7 @@ public function test_rest_request_good_params( Closure $set_up, bool $expect_sto $filtered_url_metric_data = null; $filter_called = 0; add_filter( - 'od_url_metric_data_pre_storage', + 'od_store_url_metric_data', function ( $url_metric_data ) use ( &$filter_called, &$filtered_url_metric_data ) { $this->assertIsArray( $url_metric_data ); $filtered_url_metric_data = $url_metric_data; From 6856026e287becca40d4bca887fb73aef5427618 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Dec 2024 14:20:11 -0800 Subject: [PATCH 18/25] Further optimize WP_Query Co-authored-by: swissspidy --- plugins/image-prioritizer/helper.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index c710ddca5..884adf0c9 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -170,11 +170,13 @@ static function ( $host ) { // Obtain the host of an image attachment's URL in case a CDN is pointing all images to an origin other than the home or site URLs. $image_attachment_query = new WP_Query( array( - 'post_type' => 'attachment', - 'post_mime_type' => 'image', - 'post_status' => 'inherit', - 'posts_per_page' => 1, - 'fields' => 'ids', + 'post_type' => 'attachment', + 'post_mime_type' => 'image', + 'post_status' => 'inherit', + 'posts_per_page' => 1, + 'fields' => 'ids', + 'no_found_rows' => true, + 'update_post_term_cache' => false, // Note that update_post_meta_cache is not included as well because wp_get_attachment_image_src() needs postmeta. ) ); if ( isset( $image_attachment_query->posts[0] ) && is_int( $image_attachment_query->posts[0] ) ) { From e1d0ac9dd935634b782d711c7e1ae85d296f44cf Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Dec 2024 14:23:14 -0800 Subject: [PATCH 19/25] Improve translatability of error message Co-authored-by: swissspidy --- plugins/image-prioritizer/helper.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 884adf0c9..f16f2eb08 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -290,7 +290,15 @@ function image_prioritizer_filter_store_url_metric_data( $data ): array { * * @noinspection PhpUnhandledExceptionInspection */ - wp_trigger_error( __FUNCTION__, $image_validity->get_error_message() . ' Background image URL: ' . $data['lcpElementExternalBackgroundImage']['url'] ); + wp_trigger_error( + __FUNCTION__, + sprintf( + /* translators: 1: error message. 2: image url */ + __( 'Error: %1$s. Background image URL: %2$s.', 'image-prioritizer' ), + rtrim( $image_validity->get_error_message(), '.' ), + $data['lcpElementExternalBackgroundImage']['url'] + ) + ); unset( $data['lcpElementExternalBackgroundImage'] ); } } From b4e693c4a194aa4c3ca946685828634bade3a136 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Mon, 16 Dec 2024 16:52:39 -0800 Subject: [PATCH 20/25] Update readme with links to where the performance features are implemented --- plugins/embed-optimizer/readme.txt | 6 ++--- plugins/image-prioritizer/readme.txt | 11 +++++---- plugins/optimization-detective/readme.txt | 29 ++++++++++++++++++++++- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/plugins/embed-optimizer/readme.txt b/plugins/embed-optimizer/readme.txt index b02756cfb..a36210a5a 100644 --- a/plugins/embed-optimizer/readme.txt +++ b/plugins/embed-optimizer/readme.txt @@ -15,9 +15,9 @@ This plugin's purpose is to optimize the performance of [embeds in WordPress](ht The current optimizations include: -1. Lazy loading embeds just before they come into view -2. Adding preconnect links for embeds in the initial viewport -3. Reserving space for embeds that resize to reduce layout shifting +1. Lazy loading embeds just before they come into view. +2. Adding preconnect links for embeds in the initial viewport. +3. Reserving space for embeds that resize to reduce layout shifting. **Lazy loading embeds** improves performance because embeds are generally very resource-intensive, so lazy loading them ensures that they don't compete with resources when the page is loading. Lazy loading of `IFRAME`\-based embeds is handled simply by adding the `loading=lazy` attribute. Lazy loading embeds that include `SCRIPT` tags is handled by using an Intersection Observer to watch for when the embed’s `FIGURE` container is going to enter the viewport and then it dynamically inserts the `SCRIPT` tag. diff --git a/plugins/image-prioritizer/readme.txt b/plugins/image-prioritizer/readme.txt index 029fe7438..867272d30 100644 --- a/plugins/image-prioritizer/readme.txt +++ b/plugins/image-prioritizer/readme.txt @@ -7,7 +7,7 @@ License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html Tags: performance, optimization, image, lcp, lazy-load -Prioritizes the loading of images and videos based on how visible they are to actual visitors; adds fetchpriority and applies lazy-loading. +Prioritizes the loading of images and videos based on how visible they are to actual visitors; adds fetchpriority and applies lazy loading. == Description == @@ -20,13 +20,14 @@ The current optimizations include: 2. The first `SOURCE` element with a `type` attribute in a `PICTURE` element. (Art-directed `PICTURE` elements using media queries are not supported.) 3. An element with a CSS `background-image` inline `style` attribute. 4. An element with a CSS `background-image` applied with a stylesheet (when the image is from an allowed origin). + 5. A `VIDEO` element's `poster` image. 2. Ensure `fetchpriority=high` is only added to an `IMG` when it is the Largest Contentful Paint (LCP) element across all responsive breakpoints. 3. Add `fetchpriority=low` to `IMG` tags which appear in the initial viewport but are not visible, such as when they are subsequent carousel slides. -4. Lazy-Loading: - 1. Apply lazy-loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. (Additionally, [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is then also correctly applied.) - 2. Implement lazy-loading of CSS background images added via inline `style` attributes. +4. Lazy loading: + 1. Apply lazy loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. + 2. Implement lazy loading of CSS background images added via inline `style` attributes. 3. Lazy-load `VIDEO` tags by setting the appropriate attributes based on whether they appear in the initial viewport. If a `VIDEO` is the LCP element, it gets `preload=auto`; if it is in an initial viewport, the `preload=metadata` default is left; if it is not in an initial viewport, it gets `preload=none`. Lazy-loaded videos also get initial `preload`, `autoplay`, and `poster` attributes restored when the `VIDEO` is going to enter the viewport. -5. Ensure that `sizes=auto` is added to all lazy-loaded `IMG` elements. +5. Ensure that [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is added to all lazy-loaded `IMG` elements. 6. Reduce the size of the `poster` image of a `VIDEO` from full size to the size appropriate for the maximum width of the video (on desktop). **This plugin requires the [Optimization Detective](https://wordpress.org/plugins/optimization-detective/) plugin as a dependency.** Please refer to that plugin for additional background on how this plugin works as well as additional developer options. diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index c21b3a523..90ef25eb2 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -17,7 +17,7 @@ This plugin is a dependency which does not provide end-user functionality on its = Background = -WordPress uses [server-side heuristics](https://make.wordpress.org/core/2023/07/13/image-performance-enhancements-in-wordpress-6-3/) to make educated guesses about which images are likely to be in the initial viewport. Likewise, it uses server-side heuristics to identify a hero image which is likely to be the Largest Contentful Paint (LCP) element. To optimize page loading, it avoids lazy-loading any of these images while also adding `fetchpriority=high` to the hero image. When these heuristics are applied successfully, the LCP metric for page loading can be improved 5-10%. Unfortunately, however, there are limitations to the heuristics that make the correct identification of which image is the LCP element only about 50% effective. See [Analyzing the Core Web Vitals performance impact of WordPress 6.3 in the field](https://make.wordpress.org/core/2023/09/19/analyzing-the-core-web-vitals-performance-impact-of-wordpress-6-3-in-the-field/). For example, it is [common](https://github.com/GoogleChromeLabs/wpp-research/pull/73) for the LCP element to vary between different viewport widths, such as desktop versus mobile. Since WordPress's heuristics are completely server-side it has no knowledge of how the page is actually laid out, and it cannot prioritize loading of images according to the client's viewport width. +WordPress uses [server-side heuristics](https://make.wordpress.org/core/2023/07/13/image-performance-enhancements-in-wordpress-6-3/) to make educated guesses about which images are likely to be in the initial viewport. Likewise, it uses server-side heuristics to identify a hero image which is likely to be the Largest Contentful Paint (LCP) element. To optimize page loading, it avoids lazy loading any of these images while also adding `fetchpriority=high` to the hero image. When these heuristics are applied successfully, the LCP metric for page loading can be improved 5-10%. Unfortunately, however, there are limitations to the heuristics that make the correct identification of which image is the LCP element only about 50% effective. See [Analyzing the Core Web Vitals performance impact of WordPress 6.3 in the field](https://make.wordpress.org/core/2023/09/19/analyzing-the-core-web-vitals-performance-impact-of-wordpress-6-3-in-the-field/). For example, it is [common](https://github.com/GoogleChromeLabs/wpp-research/pull/73) for the LCP element to vary between different viewport widths, such as desktop versus mobile. Since WordPress's heuristics are completely server-side it has no knowledge of how the page is actually laid out, and it cannot prioritize loading of images according to the client's viewport width. In order to increase the accuracy of identifying the LCP element, including across various client viewport widths, this plugin gathers metrics from real users (RUM) to detect the actual LCP element and then use this information to optimize the page for future visitors so that the loading of the LCP element is properly prioritized. This is the purpose of Optimization Detective. The approach is heavily inspired by Philip Walton’s [Dynamic LCP Priority: Learning from Past Visits](https://philipwalton.com/articles/dynamic-lcp-priority/). See also the initial exploration document that laid out this project: [Image Loading Optimization via Client-side Detection](https://docs.google.com/document/u/1/d/16qAJ7I_ljhEdx2Cn2VlK7IkiixobY9zNn8FXxN9T9Ls/view). @@ -40,6 +40,33 @@ There are currently **no settings** and no user interface for this plugin since When the `WP_DEBUG` constant is enabled, additional logging for Optimization Detective is added to the browser console. += Use Cases and Examples = + +As mentioned above, this plugin is a dependency that doesn't provide features on its own. Dependent plugins leverage the collected URL Metrics to apply optimizations. What follows us a running list of the optimizations which are enabled by Optimization Detective, along with a links to the related code used for the implementation: + +**[Image Prioritizer](https://wordpress.org/plugins/image-prioritizer/) ([GitHub](https://github.com/WordPress/performance/tree/trunk/plugins/image-prioritizer)):** + +1. Add breakpoint-specific `fetchpriority=high` preload links (`LINK[rel=preload]`) for image URLs of LCP elements: + 1. An `IMG` element, including the `srcset`/`sizes` attributes supplied as `imagesrcset`/`imagesizes` on the `LINK`. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L167-L177), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L304-L349)) + 2. The first `SOURCE` element with a `type` attribute in a `PICTURE` element. (Art-directed `PICTURE` elements using media queries are not supported.) ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L192-L275), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L304-L349)) + 3. An element with a CSS `background-image` inline `style` attribute. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php#L62-L92), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php#L182-L203)) + 4. An element with a CSS `background-image` applied with a stylesheet (when the image is from an allowed origin). ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php#L82-L83), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php#L135-L203), [3](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/helper.php#L83-L264), [4](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/detect.js)) + 5. A `VIDEO` element's `poster` image. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php#L127-L161)) +2. Ensure `fetchpriority=high` is only added to an `IMG` when it is the Largest Contentful Paint (LCP) element across all responsive breakpoints. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L65-L91), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L137-L146)) +3. Add `fetchpriority=low` to `IMG` tags which appear in the initial viewport but are not visible, such as when they are subsequent carousel slides. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L105-L123), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L137-L146)) +4. Lazy loading: + 1. Apply lazy loading to `IMG` tags based on whether they appear in any breakpoint’s initial viewport. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L124-L133)) + 2. Implement lazy loading of CSS background images added via inline `style` attributes. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-background-image-styled-tag-visitor.php#L205-L238), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/helper.php#L369-L382), [3](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/lazy-load-bg-image.js)) + 3. Lazy-load `VIDEO` tags by setting the appropriate attributes based on whether they appear in the initial viewport. If a `VIDEO` is the LCP element, it gets `preload=auto`; if it is in an initial viewport, the `preload=metadata` default is left; if it is not in an initial viewport, it gets `preload=none`. Lazy-loaded videos also get initial `preload`, `autoplay`, and `poster` attributes restored when the `VIDEO` is going to enter the viewport. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php#L163-L246), [2](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/helper.php#L352-L367), [3](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/lazy-load-video.js)) +5. Ensure that [`sizes=auto`](https://make.wordpress.org/core/2024/10/18/auto-sizes-for-lazy-loaded-images-in-wordpress-6-7/) is added to all lazy-loaded `IMG` elements. ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php#L148-L163)) +6. Reduce the size of the `poster` image of a `VIDEO` from full size to the size appropriate for the maximum width of the video (on desktop). ([1](https://github.com/WordPress/performance/blob/e1d0ac9dd935634b782d711c7e1ae85d296f44cf/plugins/image-prioritizer/class-image-prioritizer-video-tag-visitor.php#L84-L125)) + +**[Embed Optimizer](https://wordpress.org/plugins/embed-optimizer/) ([GitHub](https://github.com/WordPress/performance/tree/trunk/plugins/embed-optimizer)):** + +1. Lazy loading embeds just before they come into view. ([1](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php#L191-L194), [2](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/hooks.php#L168-L336)) +2. Adding preconnect links for embeds in the initial viewport. ([1](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php#L114-L190)) +3. Reserving space for embeds that resize to reduce layout shifting. ([1](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/hooks.php#L81-L144), [2](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/detect.js), [3](https://github.com/WordPress/performance/blob/ce76a6a77c15248126b5dab895bc11d0adda0baa/plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php#L218-L285)) + = Hooks = **Action:** `od_init` (argument: plugin version) From d4c9f40fb3307d11b1679da76c2e8333a80f91d0 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 17 Dec 2024 09:42:05 -0800 Subject: [PATCH 21/25] Eliminate od_store_url_metric_data filter in favor of reusing rest_request_before_callbacks --- plugins/image-prioritizer/helper.php | 32 +++-- plugins/image-prioritizer/hooks.php | 2 +- .../image-prioritizer/tests/test-helper.php | 86 ++++++++--- .../image-prioritizer/tests/test-hooks.php | 2 +- plugins/optimization-detective/readme.txt | 8 -- .../storage/rest-api.php | 74 +++------- .../tests/storage/test-rest-api.php | 136 +++++------------- 7 files changed, 144 insertions(+), 196 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index f16f2eb08..e3dcda832 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -264,26 +264,34 @@ static function ( $host ) { } /** - * Filters the validity of a URL Metric with an LCP element background image. + * Filters the response before executing any REST API callbacks. * * This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background - * image URL is not valid. Removal of the property is preferable to + * image URL is not valid. Removal of the property is preferable to invalidating the entire URL Metric because then + * potentially no URL Metrics would ever be collected if, for example, the background image URL is pointing to a + * disallowed origin. Then none of the other optimizations would be able to be applied. * * @since n.e.x.t * @access private * - * @param array|mixed $data URL Metric data. - * @return array Sanitized URL Metric data. + * @phpstan-param WP_REST_Request> $request * + * @param WP_REST_Response|WP_HTTP_Response|WP_Error|mixed $response Result to send to the client. + * Usually a WP_REST_Response or WP_Error. + * @param array $handler Route handler used for the request. + * @param WP_REST_Request $request Request used to generate the response. + * + * @return WP_REST_Response|WP_HTTP_Response|WP_Error|mixed Result to send to the client. * @noinspection PhpDocMissingThrowsInspection */ -function image_prioritizer_filter_store_url_metric_data( $data ): array { - if ( ! is_array( $data ) ) { - $data = array(); +function image_prioritizer_filter_rest_request_before_callbacks( $response, array $handler, WP_REST_Request $request ) { + if ( $request->get_method() !== 'POST' || OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( $request->get_route(), '/' ) ) { + return $response; } - if ( isset( $data['lcpElementExternalBackgroundImage']['url'] ) && is_string( $data['lcpElementExternalBackgroundImage']['url'] ) ) { - $image_validity = image_prioritizer_validate_background_image_url( $data['lcpElementExternalBackgroundImage']['url'] ); + $lcp_external_background_image = $request['lcpElementExternalBackgroundImage']; + if ( is_array( $lcp_external_background_image ) && isset( $lcp_external_background_image['url'] ) && is_string( $lcp_external_background_image['url'] ) ) { + $image_validity = image_prioritizer_validate_background_image_url( $lcp_external_background_image['url'] ); if ( is_wp_error( $image_validity ) ) { /** * No WP_Exception is thrown by wp_trigger_error() since E_USER_ERROR is not passed as the error level. @@ -296,14 +304,14 @@ function image_prioritizer_filter_store_url_metric_data( $data ): array { /* translators: 1: error message. 2: image url */ __( 'Error: %1$s. Background image URL: %2$s.', 'image-prioritizer' ), rtrim( $image_validity->get_error_message(), '.' ), - $data['lcpElementExternalBackgroundImage']['url'] + $lcp_external_background_image['url'] ) ); - unset( $data['lcpElementExternalBackgroundImage'] ); + unset( $request['lcpElementExternalBackgroundImage'] ); } } - return $data; + return $response; } /** diff --git a/plugins/image-prioritizer/hooks.php b/plugins/image-prioritizer/hooks.php index d257ed735..636590875 100644 --- a/plugins/image-prioritizer/hooks.php +++ b/plugins/image-prioritizer/hooks.php @@ -13,4 +13,4 @@ add_action( 'od_init', 'image_prioritizer_init' ); add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ); add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ); -add_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' ); +add_filter( 'rest_request_before_callbacks', 'image_prioritizer_filter_rest_request_before_callbacks', 10, 3 ); diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 8ecdd4a33..65a6272d2 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -747,10 +747,23 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s * * @return array */ - public function data_provider_to_test_image_prioritizer_filter_store_url_metric_data(): array { + public function data_provider_to_test_image_prioritizer_filter_rest_request_before_callbacks(): array { + $get_sample_url_metric_data = function (): array { + return $this->get_sample_url_metric( array() )->jsonSerialize(); + }; + + $create_request = static function ( array $url_metric_data ): WP_REST_Request { + $request = new WP_REST_Request( 'POST', '/' . OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ); + $request->set_header( 'content-type', 'application/json' ); + $request->set_body( wp_json_encode( $url_metric_data ) ); + return $request; + }; + return array( 'invalid_external_bg_image' => array( - 'set_up' => static function ( array $url_metric_data ): array { + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + $url_metric_data = $get_sample_url_metric_data(); + $url_metric_data['lcpElementExternalBackgroundImage'] = array( 'url' => 'https://bad-origin.example.com/image.jpg', 'tag' => 'DIV', @@ -759,22 +772,23 @@ public function data_provider_to_test_image_prioritizer_filter_store_url_metric_ ); $url_metric_data['viewport']['width'] = 10101; $url_metric_data['viewport']['height'] = 20202; - return $url_metric_data; + return $create_request( $url_metric_data ); }, - 'assert' => function ( array $url_metric_data ): void { - $this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data ); + 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $request ); $this->assertSame( array( 'width' => 10101, 'height' => 20202, ), - $url_metric_data['viewport'] + $request['viewport'] ); }, ), 'valid_external_bg_image' => array( - 'set_up' => static function ( array $url_metric_data ): array { + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + $url_metric_data = $get_sample_url_metric_data(); $image_url = home_url( '/good.jpg' ); add_filter( @@ -808,11 +822,11 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { $url_metric_data['viewport']['width'] = 30303; $url_metric_data['viewport']['height'] = 40404; - return $url_metric_data; + return $create_request( $url_metric_data ); }, - 'assert' => function ( array $url_metric_data ): void { - $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data ); - $this->assertIsArray( $url_metric_data['lcpElementExternalBackgroundImage'] ); + 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request ); + $this->assertIsArray( $request['lcpElementExternalBackgroundImage'] ); $this->assertSame( array( 'url' => home_url( '/good.jpg' ), @@ -820,33 +834,63 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { 'id' => null, 'class' => null, ), - $url_metric_data['lcpElementExternalBackgroundImage'] + $request['lcpElementExternalBackgroundImage'] ); $this->assertSame( array( 'width' => 30303, 'height' => 40404, ), - $url_metric_data['viewport'] + $request['viewport'] ); }, ), + + 'not_store_post_request' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + $url_metric_data = $get_sample_url_metric_data(); + $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/'; + $request = $create_request( $url_metric_data ); + $request->set_method( 'GET' ); + return $request; + }, + 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request ); + $this->assertSame( 'https://totally-different.example.com/', $request['lcpElementExternalBackgroundImage'] ); + }, + ), + + 'not_store_request' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + $url_metric_data = $get_sample_url_metric_data(); + $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/'; + $request = $create_request( $url_metric_data ); + $request->set_route( '/foo/v2/bar' ); + return $request; + }, + 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request ); + $this->assertSame( 'https://totally-different.example.com/', $request['lcpElementExternalBackgroundImage'] ); + }, + ), ); } /** - * Tests image_prioritizer_filter_store_url_metric_data(). + * Tests image_prioritizer_filter_rest_request_before_callbacks(). * - * @dataProvider data_provider_to_test_image_prioritizer_filter_store_url_metric_data + * @dataProvider data_provider_to_test_image_prioritizer_filter_rest_request_before_callbacks * - * @covers ::image_prioritizer_filter_store_url_metric_data + * @covers ::image_prioritizer_filter_rest_request_before_callbacks * @covers ::image_prioritizer_validate_background_image_url */ - public function test_image_prioritizer_filter_store_url_metric_data( Closure $set_up, Closure $assert ): void { - $url_metric_data = $set_up( $this->get_sample_url_metric( array() )->jsonSerialize() ); - - $url_metric_data = image_prioritizer_filter_store_url_metric_data( $url_metric_data ); - $assert( $url_metric_data ); + public function test_image_prioritizer_filter_rest_request_before_callbacks( Closure $set_up, Closure $assert ): void { + $request = $set_up(); + $response = new WP_REST_Response(); + $handler = array(); + $filtered_response = image_prioritizer_filter_rest_request_before_callbacks( $response, $handler, $request ); + $this->assertSame( $response, $filtered_response ); + $assert( $request ); } /** diff --git a/plugins/image-prioritizer/tests/test-hooks.php b/plugins/image-prioritizer/tests/test-hooks.php index 4d34043d3..840212da4 100644 --- a/plugins/image-prioritizer/tests/test-hooks.php +++ b/plugins/image-prioritizer/tests/test-hooks.php @@ -14,6 +14,6 @@ public function test_hooks_added(): void { $this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) ); $this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) ); $this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) ); - $this->assertEquals( 10, has_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' ) ); + $this->assertEquals( 10, has_filter( 'rest_request_before_callbacks', 'image_prioritizer_filter_rest_request_before_callbacks' ) ); } } diff --git a/plugins/optimization-detective/readme.txt b/plugins/optimization-detective/readme.txt index 90ef25eb2..1e783ef82 100644 --- a/plugins/optimization-detective/readme.txt +++ b/plugins/optimization-detective/readme.txt @@ -273,14 +273,6 @@ The ETag is a unique identifier that changes whenever the underlying data used t When the ETag for URL Metrics in a complete viewport group no longer matches the current environment's ETag, new URL Metrics will then begin to be collected until there are no more stored URL Metrics with the old ETag. These new URL Metrics will include data relevant to the newly activated plugins and their tag visitors. -**Filter:** `od_url_metric_data_pre_storage` (default arg: URL Metric data array) - -Filters the URL Metric data prior to validation for storage. - -This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in JSON Schema. This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this filter can be more expensive, such as doing filesystem checks or HTTP requests. - -To fail validation for the provided URL Metric data, an `OD_Data_Validation_Exception` exception should be thrown. Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. - **Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`) Fires whenever a URL Metric was successfully stored. diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index 204a44140..a67cc354e 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -174,7 +174,7 @@ function od_handle_rest_request( WP_REST_Request $request ) { ); } - $data = $request->get_json_params(); + $data = $request->get_json_params(); // TODO: Why not just get_params()? if ( ! is_array( $data ) ) { return new WP_Error( 'missing_array_json_body', @@ -186,64 +186,28 @@ function od_handle_rest_request( WP_REST_Request $request ) { OD_Storage_Lock::set_lock(); try { - $data = array_merge( - $data, - array( - // Now supply the readonly args which were omitted from the REST API params due to being `readonly`. - 'timestamp' => microtime( true ), - 'uuid' => wp_generate_uuid4(), - 'etag' => $request->get_param( 'current_etag' ), + // The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed. + $url_metric = new OD_Strict_URL_Metric( + array_merge( + $data, + array( + // Now supply the readonly args which were omitted from the REST API params due to being `readonly`. + 'timestamp' => microtime( true ), + 'uuid' => wp_generate_uuid4(), + 'etag' => $request->get_param( 'current_etag' ), + ) ) ); - - /** - * Filters the URL Metric data prior to validation for storage. - * - * This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in - * JSON Schema. This is also necessary because the 'validate_callback' key in a JSON Schema is not respected when - * gathering the REST API endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this, - * the REST API doesn't support 'validate_callback' for any nested arguments in any case, meaning that custom - * constraints would be able to be applied to multidimensional objects, such as the items inside 'elements'. - * - * This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is - * loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this - * filter can be more expensive, such as doing filesystem checks or HTTP requests. - * - * To fail validation for the provided URL Metric data, an OD_Data_Validation_Exception exception should be thrown. - * Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data. - * - * @since n.e.x.t - * - * @param array $data URL Metric data. This is the Data type from OD_URL_Metric. - */ - $data = apply_filters( 'od_store_url_metric_data', $data ); - - // The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed. - $url_metric = new OD_Strict_URL_Metric( $data ); - } catch ( Exception $e ) { - $error_data = array(); - if ( $e instanceof OD_Data_Validation_Exception ) { - $error_message = sprintf( - /* translators: %s is exception name */ + } catch ( OD_Data_Validation_Exception $e ) { + return new WP_Error( + 'rest_invalid_param', + sprintf( + /* translators: %s is exception message */ __( 'Failed to validate URL Metric: %s', 'optimization-detective' ), $e->getMessage() - ); - $error_data['status'] = 400; - } else { - $error_message = sprintf( - /* translators: %s is exception name */ - __( 'Failed to validate URL Metric: %s', 'optimization-detective' ), - __( 'An unrecognized exception was thrown', 'optimization-detective' ) - ); - $error_data['status'] = 500; - if ( WP_DEBUG ) { - $error_data['exception_class'] = get_class( $e ); - $error_data['exception_message'] = $e->getMessage(); - $error_data['exception_code'] = $e->getCode(); - } - } - - return new WP_Error( 'rest_invalid_param', $error_message, $error_data ); + ), + array( 'status' => 400 ) + ); } // TODO: This should be changed from store_url_metric($slug, $url_metric) instead be update_post( $slug, $group_collection ). As it stands, store_url_metric() is duplicating logic here. diff --git a/plugins/optimization-detective/tests/storage/test-rest-api.php b/plugins/optimization-detective/tests/storage/test-rest-api.php index 8817e7337..3671b5784 100644 --- a/plugins/optimization-detective/tests/storage/test-rest-api.php +++ b/plugins/optimization-detective/tests/storage/test-rest-api.php @@ -41,57 +41,21 @@ static function ( array $properties ) use ( $property_name ): array { }; return array( - 'not_extended' => array( - 'set_up' => function (): array { + 'not_extended' => array( + 'set_up' => function (): array { return $this->get_valid_params(); }, - 'expect_stored' => true, - 'expected_status' => 200, ), - 'extended' => array( - 'set_up' => function () use ( $add_root_extra_property ): array { + 'extended' => array( + 'set_up' => function () use ( $add_root_extra_property ): array { $add_root_extra_property( 'extra' ); $params = $this->get_valid_params(); $params['extra'] = 'foo'; return $params; }, - 'expect_stored' => true, - 'expected_status' => 200, ), - 'rejected_by_generic_exception' => array( - 'set_up' => function (): array { - add_filter( - 'od_store_url_metric_data', - static function ( $data ): array { - if ( count( $data ) > 0 ) { - throw new Exception( 'bad' ); - } - return $data; - } - ); - return $this->get_valid_params(); - }, - 'expect_stored' => false, - 'expected_status' => 500, - ), - 'rejected_by_validation_exception' => array( - 'set_up' => function (): array { - add_filter( - 'od_store_url_metric_data', - static function ( $data ): array { - if ( count( $data ) > 0 ) { - throw new OD_Data_Validation_Exception( 'bad' ); - } - return $data; - } - ); - return $this->get_valid_params(); - }, - 'expect_stored' => false, - 'expected_status' => 400, - ), - 'with_cache_purge_post_id' => array( - 'set_up' => function (): array { + 'with_cache_purge_post_id' => array( + 'set_up' => function (): array { $params = $this->get_valid_params(); $params['cache_purge_post_id'] = self::factory()->post->create(); $params['url'] = get_permalink( $params['cache_purge_post_id'] ); @@ -99,8 +63,6 @@ static function ( $data ): array { $params['hmac'] = od_get_url_metrics_storage_hmac( $params['slug'], $params['current_etag'], $params['url'], $params['cache_purge_post_id'] ); return $params; }, - 'expect_stored' => true, - 'expected_status' => 200, ), ); } @@ -114,20 +76,7 @@ static function ( $data ): array { * @covers ::od_handle_rest_request * @covers ::od_trigger_page_cache_invalidation */ - public function test_rest_request_good_params( Closure $set_up, bool $expect_stored, int $expected_status ): void { - $filtered_url_metric_data = null; - $filter_called = 0; - add_filter( - 'od_store_url_metric_data', - function ( $url_metric_data ) use ( &$filter_called, &$filtered_url_metric_data ) { - $this->assertIsArray( $url_metric_data ); - $filtered_url_metric_data = $url_metric_data; - $filter_called++; - return $url_metric_data; - }, - 0 - ); - + public function test_rest_request_good_params( Closure $set_up ): void { $stored_context = null; add_action( 'od_url_metric_stored', @@ -151,49 +100,40 @@ function ( OD_URL_Metric_Store_Request_Context $context ) use ( &$stored_context $request = $this->create_request( $valid_params ); $response = rest_get_server()->dispatch( $request ); - $this->assertSame( 1, $filter_called ); - $this->assertSame( $expect_stored ? 1 : 0, did_action( 'od_url_metric_stored' ) ); + $this->assertSame( 1, did_action( 'od_url_metric_stored' ) ); - $this->assertSame( $expected_status, $response->get_status(), 'Response: ' . wp_json_encode( $response ) ); + $this->assertSame( 200, $response->get_status(), 'Response: ' . wp_json_encode( $response ) ); $data = $response->get_data(); - $this->assertCount( $expect_stored ? 1 : 0, get_posts( array( 'post_type' => OD_URL_Metrics_Post_Type::SLUG ) ) ); - - if ( ! $expect_stored ) { - $this->assertArrayHasKey( 'code', $data ); - $this->assertArrayHasKey( 'message', $data ); - $this->assertNull( $stored_context ); - $this->assertNull( OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ) ); - } else { - $this->assertTrue( $data['success'] ); - - $post = OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ); - $this->assertInstanceOf( WP_Post::class, $post ); - - $url_metrics = OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ); - $this->assertCount( 1, $url_metrics, 'Expected number of URL Metrics stored.' ); - $this->assertSame( $valid_params['elements'], $this->get_array_json_data( $url_metrics[0]->get( 'elements' ) ) ); - $this->assertSame( $valid_params['viewport']['width'], $url_metrics[0]->get_viewport_width() ); - - $expected_data = $valid_params; - unset( $expected_data['hmac'], $expected_data['slug'], $expected_data['current_etag'], $expected_data['cache_purge_post_id'] ); - unset( $expected_data['unset_prop'] ); - $this->assertSame( - $expected_data, - wp_array_slice_assoc( $url_metrics[0]->jsonSerialize(), array_keys( $expected_data ) ) - ); + $this->assertCount( 1, get_posts( array( 'post_type' => OD_URL_Metrics_Post_Type::SLUG ) ) ); - $this->assertInstanceOf( OD_URL_Metric_Store_Request_Context::class, $stored_context ); - $this->assertSame( $stored_context->url_metric->jsonSerialize(), $filtered_url_metric_data ); - - // Now check that od_trigger_page_cache_invalidation() cleaned caches as expected. - $this->assertSame( $url_metrics[0]->jsonSerialize(), $stored_context->url_metric->jsonSerialize() ); - if ( isset( $valid_params['cache_purge_post_id'] ) ) { - $cache_purge_post_id = $stored_context->request->get_param( 'cache_purge_post_id' ); - $this->assertSame( $valid_params['cache_purge_post_id'], $cache_purge_post_id ); - $scheduled = wp_next_scheduled( 'od_trigger_page_cache_invalidation', array( $cache_purge_post_id ) ); - $this->assertIsInt( $scheduled ); - $this->assertGreaterThan( time(), $scheduled ); - } + $this->assertTrue( $data['success'] ); + + $post = OD_URL_Metrics_Post_Type::get_post( $valid_params['slug'] ); + $this->assertInstanceOf( WP_Post::class, $post ); + + $url_metrics = OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ); + $this->assertCount( 1, $url_metrics, 'Expected number of URL Metrics stored.' ); + $this->assertSame( $valid_params['elements'], $this->get_array_json_data( $url_metrics[0]->get( 'elements' ) ) ); + $this->assertSame( $valid_params['viewport']['width'], $url_metrics[0]->get_viewport_width() ); + + $expected_data = $valid_params; + unset( $expected_data['hmac'], $expected_data['slug'], $expected_data['current_etag'], $expected_data['cache_purge_post_id'] ); + unset( $expected_data['unset_prop'] ); + $this->assertSame( + $expected_data, + wp_array_slice_assoc( $url_metrics[0]->jsonSerialize(), array_keys( $expected_data ) ) + ); + + $this->assertInstanceOf( OD_URL_Metric_Store_Request_Context::class, $stored_context ); + + // Now check that od_trigger_page_cache_invalidation() cleaned caches as expected. + $this->assertSame( $url_metrics[0]->jsonSerialize(), $stored_context->url_metric->jsonSerialize() ); + if ( isset( $valid_params['cache_purge_post_id'] ) ) { + $cache_purge_post_id = $stored_context->request->get_param( 'cache_purge_post_id' ); + $this->assertSame( $valid_params['cache_purge_post_id'], $cache_purge_post_id ); + $scheduled = wp_next_scheduled( 'od_trigger_page_cache_invalidation', array( $cache_purge_post_id ) ); + $this->assertIsInt( $scheduled ); + $this->assertGreaterThan( time(), $scheduled ); } } From f8a00b42c9624ce601a8a801fc2c0f1b7cc99add Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 17 Dec 2024 09:51:00 -0800 Subject: [PATCH 22/25] Remove todo This is using get_json_params() instead of get_params() because any URL query parameter would then be included among the params, so by just getting the params from the JSON body we avoid potential failures due to additional query parameters being injected into the request URL. --- plugins/optimization-detective/storage/rest-api.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/optimization-detective/storage/rest-api.php b/plugins/optimization-detective/storage/rest-api.php index a67cc354e..09ce02501 100644 --- a/plugins/optimization-detective/storage/rest-api.php +++ b/plugins/optimization-detective/storage/rest-api.php @@ -174,7 +174,7 @@ function od_handle_rest_request( WP_REST_Request $request ) { ); } - $data = $request->get_json_params(); // TODO: Why not just get_params()? + $data = $request->get_json_params(); if ( ! is_array( $data ) ) { return new WP_Error( 'missing_array_json_body', From 4a8dc1675f8ee310eafc12fcc61dc8234105e9aa Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 17 Dec 2024 10:14:21 -0800 Subject: [PATCH 23/25] Account for route matching being case insensitive --- plugins/image-prioritizer/helper.php | 7 ++++- .../image-prioritizer/tests/test-helper.php | 27 ++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index e3dcda832..6189c896a 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -285,7 +285,12 @@ static function ( $host ) { * @noinspection PhpDocMissingThrowsInspection */ function image_prioritizer_filter_rest_request_before_callbacks( $response, array $handler, WP_REST_Request $request ) { - if ( $request->get_method() !== 'POST' || OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( $request->get_route(), '/' ) ) { + if ( + $request->get_method() !== 'POST' + || + // The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match. + OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== strtolower( trim( $request->get_route(), '/' ) ) + ) { return $response; } diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index 65a6272d2..f1a7c0d1e 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -760,7 +760,7 @@ public function data_provider_to_test_image_prioritizer_filter_rest_request_befo }; return array( - 'invalid_external_bg_image' => array( + 'invalid_external_bg_image' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); @@ -786,7 +786,7 @@ public function data_provider_to_test_image_prioritizer_filter_rest_request_befo }, ), - 'valid_external_bg_image' => array( + 'valid_external_bg_image' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); $image_url = home_url( '/good.jpg' ); @@ -846,7 +846,26 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { }, ), - 'not_store_post_request' => array( + 'invalid_external_bg_image_variant_route' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + $url_metric_data = $get_sample_url_metric_data(); + + $url_metric_data['lcpElementExternalBackgroundImage'] = array( + 'url' => 'https://bad-origin.example.com/image.jpg', + 'tag' => 'DIV', + 'id' => null, + 'class' => null, + ); + $request = $create_request( $url_metric_data ); + $request->set_route( str_replace( 'store', 'STORE', $request->get_route() ) ); + return $request; + }, + 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $request ); + }, + ), + + 'not_store_post_request' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/'; @@ -860,7 +879,7 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { }, ), - 'not_store_request' => array( + 'not_store_request' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/'; From ba14c364c76754e855c42bd0fa20e7318f74ede7 Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 17 Dec 2024 10:31:44 -0800 Subject: [PATCH 24/25] Improve function description and further trim route --- plugins/image-prioritizer/helper.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 6189c896a..201f03a96 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -264,7 +264,7 @@ static function ( $host ) { } /** - * Filters the response before executing any REST API callbacks. + * Sanitizes the lcpElementExternalBackgroundImage property from the request URL Metric storage request. * * This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background * image URL is not valid. Removal of the property is preferable to invalidating the entire URL Metric because then @@ -288,8 +288,8 @@ function image_prioritizer_filter_rest_request_before_callbacks( $response, arra if ( $request->get_method() !== 'POST' || - // The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match. - OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== strtolower( trim( $request->get_route(), '/' ) ) + // The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match. + OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( strtolower( trim( $request->get_route(), '/' ) ) ) ) { return $response; } From 5ab7fd1a1ca07413795332a89fcf6bb3995360bb Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Tue, 17 Dec 2024 11:15:02 -0800 Subject: [PATCH 25/25] Add test case for route ending in newline --- plugins/image-prioritizer/helper.php | 4 +- .../image-prioritizer/tests/test-helper.php | 72 ++++++++++++------- 2 files changed, 47 insertions(+), 29 deletions(-) diff --git a/plugins/image-prioritizer/helper.php b/plugins/image-prioritizer/helper.php index 201f03a96..75c3e18d4 100644 --- a/plugins/image-prioritizer/helper.php +++ b/plugins/image-prioritizer/helper.php @@ -288,8 +288,8 @@ function image_prioritizer_filter_rest_request_before_callbacks( $response, arra if ( $request->get_method() !== 'POST' || - // The strtolower() is due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match. - OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( strtolower( trim( $request->get_route(), '/' ) ) ) + // The strtolower() and outer trim are due to \WP_REST_Server::match_request_to_handler() using case-insensitive pattern match and using '$' instead of '\z'. + OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== rtrim( strtolower( ltrim( $request->get_route(), '/' ) ) ) ) { return $response; } diff --git a/plugins/image-prioritizer/tests/test-helper.php b/plugins/image-prioritizer/tests/test-helper.php index f1a7c0d1e..dfd05b854 100644 --- a/plugins/image-prioritizer/tests/test-helper.php +++ b/plugins/image-prioritizer/tests/test-helper.php @@ -759,17 +759,19 @@ public function data_provider_to_test_image_prioritizer_filter_rest_request_befo return $request; }; + $bad_origin_data = array( + 'url' => 'https://bad-origin.example.com/image.jpg', + 'tag' => 'DIV', + 'id' => null, + 'class' => null, + ); + return array( - 'invalid_external_bg_image' => array( - 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { + 'invalid_external_bg_image' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request, $bad_origin_data ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); - $url_metric_data['lcpElementExternalBackgroundImage'] = array( - 'url' => 'https://bad-origin.example.com/image.jpg', - 'tag' => 'DIV', - 'id' => null, - 'class' => null, - ); + $url_metric_data['lcpElementExternalBackgroundImage'] = $bad_origin_data; $url_metric_data['viewport']['width'] = 10101; $url_metric_data['viewport']['height'] = 20202; return $create_request( $url_metric_data ); @@ -786,7 +788,7 @@ public function data_provider_to_test_image_prioritizer_filter_rest_request_befo }, ), - 'valid_external_bg_image' => array( + 'valid_external_bg_image' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); $image_url = home_url( '/good.jpg' ); @@ -846,17 +848,14 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { }, ), - 'invalid_external_bg_image_variant_route' => array( - 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { - $url_metric_data = $get_sample_url_metric_data(); - - $url_metric_data['lcpElementExternalBackgroundImage'] = array( - 'url' => 'https://bad-origin.example.com/image.jpg', - 'tag' => 'DIV', - 'id' => null, - 'class' => null, + 'invalid_external_bg_image_uppercase_route' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request, $bad_origin_data ): WP_REST_Request { + $request = $create_request( + array_merge( + $get_sample_url_metric_data(), + array( 'lcpElementExternalBackgroundImage' => $bad_origin_data ) + ) ); - $request = $create_request( $url_metric_data ); $request->set_route( str_replace( 'store', 'STORE', $request->get_route() ) ); return $request; }, @@ -865,21 +864,40 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) { }, ), - 'not_store_post_request' => array( - 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { - $url_metric_data = $get_sample_url_metric_data(); - $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/'; - $request = $create_request( $url_metric_data ); - $request->set_method( 'GET' ); + 'invalid_external_bg_image_trailing_newline_route' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request, $bad_origin_data ): WP_REST_Request { + $request = $create_request( + array_merge( + $get_sample_url_metric_data(), + array( 'lcpElementExternalBackgroundImage' => $bad_origin_data ) + ) + ); + $request->set_route( $request->get_route() . "\n" ); return $request; }, 'assert' => function ( WP_REST_Request $request ): void { + $this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $request ); + }, + ), + + 'not_store_post_request' => array( + 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request, $bad_origin_data ): WP_REST_Request { + $request = $create_request( + array_merge( + $get_sample_url_metric_data(), + array( 'lcpElementExternalBackgroundImage' => $bad_origin_data ) + ) + ); + $request->set_method( 'GET' ); + return $request; + }, + 'assert' => function ( WP_REST_Request $request ) use ( $bad_origin_data ): void { $this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request ); - $this->assertSame( 'https://totally-different.example.com/', $request['lcpElementExternalBackgroundImage'] ); + $this->assertSame( $bad_origin_data, $request['lcpElementExternalBackgroundImage'] ); }, ), - 'not_store_request' => array( + 'not_store_request' => array( 'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request { $url_metric_data = $get_sample_url_metric_data(); $url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/';