Skip to content

Commit

Permalink
Replace validity filter with sanitization filter; unset unset()
Browse files Browse the repository at this point in the history
  • Loading branch information
@westonruter
westonruter committed Dec 16, 2024
1 parent 6005f4a commit 250f094
Show file tree
Hide file tree
Showing 12 changed files with 145 additions and 443 deletions.
23 changes: 10 additions & 13 deletions plugins/image-prioritizer/helper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, mixed>|mixed $data URL Metric data.
* @return array<string, mixed> 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;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion plugins/image-prioritizer/hooks.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
118 changes: 41 additions & 77 deletions plugins/image-prioritizer/tests/test-helper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,78 +747,34 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s
*
* @return array<string, mixed>
*/
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(
Expand All @@ -843,46 +799,54 @@ 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' ),
'tag' => 'DIV',
'id' => null,
'class' => null,
),
$url_metric->get( 'lcpElementExternalBackgroundImage' )
$url_metric_data['lcpElementExternalBackgroundImage']
);
$this->assertSame(
array(
'width' => 30303,
'height' => 40404,
),
$url_metric_data['viewport']
);
},
),
);
}

/**
* 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 );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion plugins/image-prioritizer/tests/test-hooks.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
}
}
45 changes: 5 additions & 40 deletions plugins/optimization-detective/class-od-element.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.' );
}

/**
Expand Down
27 changes: 0 additions & 27 deletions plugins/optimization-detective/class-od-url-metric.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
14 changes: 4 additions & 10 deletions plugins/optimization-detective/readme.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, mixed>`): 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`)

Expand Down
Loading

0 comments on commit 250f094

Please sign in to comment.