Skip to content

Commit d4c9f40

Browse files
westonruterwestonruter
committed
Eliminate od_store_url_metric_data filter in favor of reusing rest_request_before_callbacks
1 parent 42c3de8 commit d4c9f40

File tree

7 files changed

+144
-196
lines changed

7 files changed

+144
-196
lines changed

‎plugins/image-prioritizer/helper.php

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -264,26 +264,34 @@ static function ( $host ) {
264264
}
265265

266266
/**
267-
* Filters the validity of a URL Metric with an LCP element background image.
267+
* Filters the response before executing any REST API callbacks.
268268
*
269269
* This removes the lcpElementExternalBackgroundImage from the URL Metric prior to it being stored if the background
270-
* image URL is not valid. Removal of the property is preferable to
270+
* image URL is not valid. Removal of the property is preferable to invalidating the entire URL Metric because then
271+
* potentially no URL Metrics would ever be collected if, for example, the background image URL is pointing to a
272+
* disallowed origin. Then none of the other optimizations would be able to be applied.
271273
*
272274
* @since n.e.x.t
273275
* @access private
274276
*
275-
* @param array<string, mixed>|mixed $data URL Metric data.
276-
* @return array<string, mixed> Sanitized URL Metric data.
277+
* @phpstan-param WP_REST_Request<array<string, mixed>> $request
277278
*
279+
* @param WP_REST_Response|WP_HTTP_Response|WP_Error|mixed $response Result to send to the client.
280+
* Usually a WP_REST_Response or WP_Error.
281+
* @param array<string, mixed> $handler Route handler used for the request.
282+
* @param WP_REST_Request $request Request used to generate the response.
283+
*
284+
* @return WP_REST_Response|WP_HTTP_Response|WP_Error|mixed Result to send to the client.
278285
* @noinspection PhpDocMissingThrowsInspection
279286
*/
280-
function image_prioritizer_filter_store_url_metric_data( $data ): array {
281-
if ( ! is_array( $data ) ) {
282-
$data = array();
287+
function image_prioritizer_filter_rest_request_before_callbacks( $response, array $handler, WP_REST_Request $request ) {
288+
if ( $request->get_method() !== 'POST' || OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE !== trim( $request->get_route(), '/' ) ) {
289+
return $response;
283290
}
284291

285-
if ( isset( $data['lcpElementExternalBackgroundImage']['url'] ) && is_string( $data['lcpElementExternalBackgroundImage']['url'] ) ) {
286-
$image_validity = image_prioritizer_validate_background_image_url( $data['lcpElementExternalBackgroundImage']['url'] );
292+
$lcp_external_background_image = $request['lcpElementExternalBackgroundImage'];
293+
if ( is_array( $lcp_external_background_image ) && isset( $lcp_external_background_image['url'] ) && is_string( $lcp_external_background_image['url'] ) ) {
294+
$image_validity = image_prioritizer_validate_background_image_url( $lcp_external_background_image['url'] );
287295
if ( is_wp_error( $image_validity ) ) {
288296
/**
289297
* 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 {
296304
/* translators: 1: error message. 2: image url */
297305
__( 'Error: %1$s. Background image URL: %2$s.', 'image-prioritizer' ),
298306
rtrim( $image_validity->get_error_message(), '.' ),
299-
$data['lcpElementExternalBackgroundImage']['url']
307+
$lcp_external_background_image['url']
300308
)
301309
);
302-
unset( $data['lcpElementExternalBackgroundImage'] );
310+
unset( $request['lcpElementExternalBackgroundImage'] );
303311
}
304312
}
305313

306-
return $data;
314+
return $response;
307315
}
308316

309317
/**

‎plugins/image-prioritizer/hooks.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,4 @@
1313
add_action( 'od_init', 'image_prioritizer_init' );
1414
add_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' );
1515
add_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' );
16-
add_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' );
16+
add_filter( 'rest_request_before_callbacks', 'image_prioritizer_filter_rest_request_before_callbacks', 10, 3 );

‎plugins/image-prioritizer/tests/test-helper.php

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -747,10 +747,23 @@ public function test_image_prioritizer_validate_background_image_url( Closure $s
747747
*
748748
* @return array<string, mixed>
749749
*/
750-
public function data_provider_to_test_image_prioritizer_filter_store_url_metric_data(): array {
750+
public function data_provider_to_test_image_prioritizer_filter_rest_request_before_callbacks(): array {
751+
$get_sample_url_metric_data = function (): array {
752+
return $this->get_sample_url_metric( array() )->jsonSerialize();
753+
};
754+
755+
$create_request = static function ( array $url_metric_data ): WP_REST_Request {
756+
$request = new WP_REST_Request( 'POST', '/' . OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE );
757+
$request->set_header( 'content-type', 'application/json' );
758+
$request->set_body( wp_json_encode( $url_metric_data ) );
759+
return $request;
760+
};
761+
751762
return array(
752763
'invalid_external_bg_image' => array(
753-
'set_up' => static function ( array $url_metric_data ): array {
764+
'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request {
765+
$url_metric_data = $get_sample_url_metric_data();
766+
754767
$url_metric_data['lcpElementExternalBackgroundImage'] = array(
755768
'url' => 'https://bad-origin.example.com/image.jpg',
756769
'tag' => 'DIV',
@@ -759,22 +772,23 @@ public function data_provider_to_test_image_prioritizer_filter_store_url_metric_
759772
);
760773
$url_metric_data['viewport']['width'] = 10101;
761774
$url_metric_data['viewport']['height'] = 20202;
762-
return $url_metric_data;
775+
return $create_request( $url_metric_data );
763776
},
764-
'assert' => function ( array $url_metric_data ): void {
765-
$this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data );
777+
'assert' => function ( WP_REST_Request $request ): void {
778+
$this->assertArrayNotHasKey( 'lcpElementExternalBackgroundImage', $request );
766779
$this->assertSame(
767780
array(
768781
'width' => 10101,
769782
'height' => 20202,
770783
),
771-
$url_metric_data['viewport']
784+
$request['viewport']
772785
);
773786
},
774787
),
775788

776789
'valid_external_bg_image' => array(
777-
'set_up' => static function ( array $url_metric_data ): array {
790+
'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request {
791+
$url_metric_data = $get_sample_url_metric_data();
778792
$image_url = home_url( '/good.jpg' );
779793

780794
add_filter(
@@ -808,45 +822,75 @@ static function ( $pre, $parsed_args, $url ) use ( $image_url ) {
808822

809823
$url_metric_data['viewport']['width'] = 30303;
810824
$url_metric_data['viewport']['height'] = 40404;
811-
return $url_metric_data;
825+
return $create_request( $url_metric_data );
812826
},
813-
'assert' => function ( array $url_metric_data ): void {
814-
$this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $url_metric_data );
815-
$this->assertIsArray( $url_metric_data['lcpElementExternalBackgroundImage'] );
827+
'assert' => function ( WP_REST_Request $request ): void {
828+
$this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request );
829+
$this->assertIsArray( $request['lcpElementExternalBackgroundImage'] );
816830
$this->assertSame(
817831
array(
818832
'url' => home_url( '/good.jpg' ),
819833
'tag' => 'DIV',
820834
'id' => null,
821835
'class' => null,
822836
),
823-
$url_metric_data['lcpElementExternalBackgroundImage']
837+
$request['lcpElementExternalBackgroundImage']
824838
);
825839
$this->assertSame(
826840
array(
827841
'width' => 30303,
828842
'height' => 40404,
829843
),
830-
$url_metric_data['viewport']
844+
$request['viewport']
831845
);
832846
},
833847
),
848+
849+
'not_store_post_request' => array(
850+
'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request {
851+
$url_metric_data = $get_sample_url_metric_data();
852+
$url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/';
853+
$request = $create_request( $url_metric_data );
854+
$request->set_method( 'GET' );
855+
return $request;
856+
},
857+
'assert' => function ( WP_REST_Request $request ): void {
858+
$this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request );
859+
$this->assertSame( 'https://totally-different.example.com/', $request['lcpElementExternalBackgroundImage'] );
860+
},
861+
),
862+
863+
'not_store_request' => array(
864+
'set_up' => static function () use ( $get_sample_url_metric_data, $create_request ): WP_REST_Request {
865+
$url_metric_data = $get_sample_url_metric_data();
866+
$url_metric_data['lcpElementExternalBackgroundImage'] = 'https://totally-different.example.com/';
867+
$request = $create_request( $url_metric_data );
868+
$request->set_route( '/foo/v2/bar' );
869+
return $request;
870+
},
871+
'assert' => function ( WP_REST_Request $request ): void {
872+
$this->assertArrayHasKey( 'lcpElementExternalBackgroundImage', $request );
873+
$this->assertSame( 'https://totally-different.example.com/', $request['lcpElementExternalBackgroundImage'] );
874+
},
875+
),
834876
);
835877
}
836878

837879
/**
838-
* Tests image_prioritizer_filter_store_url_metric_data().
880+
* Tests image_prioritizer_filter_rest_request_before_callbacks().
839881
*
840-
* @dataProvider data_provider_to_test_image_prioritizer_filter_store_url_metric_data
882+
* @dataProvider data_provider_to_test_image_prioritizer_filter_rest_request_before_callbacks
841883
*
842-
* @covers ::image_prioritizer_filter_store_url_metric_data
884+
* @covers ::image_prioritizer_filter_rest_request_before_callbacks
843885
* @covers ::image_prioritizer_validate_background_image_url
844886
*/
845-
public function test_image_prioritizer_filter_store_url_metric_data( Closure $set_up, Closure $assert ): void {
846-
$url_metric_data = $set_up( $this->get_sample_url_metric( array() )->jsonSerialize() );
847-
848-
$url_metric_data = image_prioritizer_filter_store_url_metric_data( $url_metric_data );
849-
$assert( $url_metric_data );
887+
public function test_image_prioritizer_filter_rest_request_before_callbacks( Closure $set_up, Closure $assert ): void {
888+
$request = $set_up();
889+
$response = new WP_REST_Response();
890+
$handler = array();
891+
$filtered_response = image_prioritizer_filter_rest_request_before_callbacks( $response, $handler, $request );
892+
$this->assertSame( $response, $filtered_response );
893+
$assert( $request );
850894
}
851895

852896
/**

‎plugins/image-prioritizer/tests/test-hooks.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,6 @@ public function test_hooks_added(): void {
1414
$this->assertEquals( 10, has_action( 'od_init', 'image_prioritizer_init' ) );
1515
$this->assertEquals( 10, has_filter( 'od_extension_module_urls', 'image_prioritizer_filter_extension_module_urls' ) );
1616
$this->assertEquals( 10, has_filter( 'od_url_metric_schema_root_additional_properties', 'image_prioritizer_add_element_item_schema_properties' ) );
17-
$this->assertEquals( 10, has_filter( 'od_store_url_metric_data', 'image_prioritizer_filter_store_url_metric_data' ) );
17+
$this->assertEquals( 10, has_filter( 'rest_request_before_callbacks', 'image_prioritizer_filter_rest_request_before_callbacks' ) );
1818
}
1919
}

‎plugins/optimization-detective/readme.txt

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -273,14 +273,6 @@ The ETag is a unique identifier that changes whenever the underlying data used t
273273

274274
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.
275275

276-
**Filter:** `od_url_metric_data_pre_storage` (default arg: URL Metric data array)
277-
278-
Filters the URL Metric data prior to validation for storage.
279-
280-
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.
281-
282-
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.
283-
284276
**Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`)
285277

286278
Fires whenever a URL Metric was successfully stored.

‎plugins/optimization-detective/storage/rest-api.php

Lines changed: 19 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ function od_handle_rest_request( WP_REST_Request $request ) {
174174
);
175175
}
176176

177-
$data = $request->get_json_params();
177+
$data = $request->get_json_params(); // TODO: Why not just get_params()?
178178
if ( ! is_array( $data ) ) {
179179
return new WP_Error(
180180
'missing_array_json_body',
@@ -186,64 +186,28 @@ function od_handle_rest_request( WP_REST_Request $request ) {
186186
OD_Storage_Lock::set_lock();
187187

188188
try {
189-
$data = array_merge(
190-
$data,
191-
array(
192-
// Now supply the readonly args which were omitted from the REST API params due to being `readonly`.
193-
'timestamp' => microtime( true ),
194-
'uuid' => wp_generate_uuid4(),
195-
'etag' => $request->get_param( 'current_etag' ),
189+
// The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed.
190+
$url_metric = new OD_Strict_URL_Metric(
191+
array_merge(
192+
$data,
193+
array(
194+
// Now supply the readonly args which were omitted from the REST API params due to being `readonly`.
195+
'timestamp' => microtime( true ),
196+
'uuid' => wp_generate_uuid4(),
197+
'etag' => $request->get_param( 'current_etag' ),
198+
)
196199
)
197200
);
198-
199-
/**
200-
* Filters the URL Metric data prior to validation for storage.
201-
*
202-
* This allows for custom sanitization and validation constraints to be applied beyond what can be expressed in
203-
* JSON Schema. This is also necessary because the 'validate_callback' key in a JSON Schema is not respected when
204-
* gathering the REST API endpoint args via the {@see rest_get_endpoint_args_for_schema()} function. Besides this,
205-
* the REST API doesn't support 'validate_callback' for any nested arguments in any case, meaning that custom
206-
* constraints would be able to be applied to multidimensional objects, such as the items inside 'elements'.
207-
*
208-
* This filter only applies when storing a URL Metric via the REST API. It does not run when a stored URL Metric is
209-
* loaded from the od_url_metrics post type. This means that sanitization and validation logic enforced via this
210-
* filter can be more expensive, such as doing filesystem checks or HTTP requests.
211-
*
212-
* To fail validation for the provided URL Metric data, an OD_Data_Validation_Exception exception should be thrown.
213-
* Otherwise, any filter callbacks must return an array consisting of the sanitized URL Metric data.
214-
*
215-
* @since n.e.x.t
216-
*
217-
* @param array<string, mixed> $data URL Metric data. This is the Data type from OD_URL_Metric.
218-
*/
219-
$data = apply_filters( 'od_store_url_metric_data', $data );
220-
221-
// The "strict" URL Metric class is being used here to ensure additionalProperties of all objects are disallowed.
222-
$url_metric = new OD_Strict_URL_Metric( $data );
223-
} catch ( Exception $e ) {
224-
$error_data = array();
225-
if ( $e instanceof OD_Data_Validation_Exception ) {
226-
$error_message = sprintf(
227-
/* translators: %s is exception name */
201+
} catch ( OD_Data_Validation_Exception $e ) {
202+
return new WP_Error(
203+
'rest_invalid_param',
204+
sprintf(
205+
/* translators: %s is exception message */
228206
__( 'Failed to validate URL Metric: %s', 'optimization-detective' ),
229207
$e->getMessage()
230-
);
231-
$error_data['status'] = 400;
232-
} else {
233-
$error_message = sprintf(
234-
/* translators: %s is exception name */
235-
__( 'Failed to validate URL Metric: %s', 'optimization-detective' ),
236-
__( 'An unrecognized exception was thrown', 'optimization-detective' )
237-
);
238-
$error_data['status'] = 500;
239-
if ( WP_DEBUG ) {
240-
$error_data['exception_class'] = get_class( $e );
241-
$error_data['exception_message'] = $e->getMessage();
242-
$error_data['exception_code'] = $e->getCode();
243-
}
244-
}
245-
246-
return new WP_Error( 'rest_invalid_param', $error_message, $error_data );
208+
),
209+
array( 'status' => 400 )
210+
);
247211
}
248212

249213
// 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.

0 commit comments

Comments
 (0)