Introduce ILO_URL_Metric to encapsulate core data structure #988

westonruter · 2024-02-15T00:15:33Z

Summary

This implements these aspects of #933:

Introduce class to represent a single URL metric (e.g. ILO_URL_Metric).

This would mostly be a value object with properties like url, slug, viewport, elements.

The class could potentially have private logic to take care of validation.

The elements entry may deserve some further thought, particularly around how it could work considering additional use-cases than LCP elements in the future.

Functions that today return an array of URL metric associative arrays should going forward return an array of URL metric class instances (e.g. ilo_parse_stored_url_metrics()).

Subsequent points will be addressed in follow-up PRs.

Relevant technical choices

A new class called ILO_URL_Metric is used instead of passing around an array of the same data.
The ILO_URL_Metric class contains the validation logic to ensure that the data is as expected.
The REST API route re-uses the JSON schema defined in the ILO_URL_Metric class to validate requests.
The ILO_URL_Metric class now requires that a timestamp be provided at construction time, which removes this concern from ilo_unshift_url_metrics().
Validation logic is applied to the URL metrics pulled from the ilo_url_metrics post type. This ensures that if the data format changes, any prior entries will just be omitted.
Static analysis is further improved with typing and addressing various inspection issues flagged by PhpStorm.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…stamp

modules/images/image-loading-optimization/class-ilo-url-metric.php

westonruter · 2024-02-22T00:44:44Z

tests/modules/images/image-loading-optimization/storage/post-type-tests.php

-		$this->assertArrayHasKey( 'timestamp', $url_metrics[0] );
-		$this->assertIsFloat( $url_metrics[0]['timestamp'] );
-		$this->assertLessThanOrEqual( microtime( true ), $url_metrics[0]['timestamp'] );


Irrelevant to test now that the timestamp is required at the time of instantiation of ILO_URL_Metric. Previously it would get added by ilo_unshift_url_metrics().

Same goes with the deleted assertions below.

westonruter · 2024-02-22T06:28:10Z

modules/images/image-loading-optimization/storage/data.php

-						if ( ! isset( $a['timestamp'] ) || ! isset( $b['timestamp'] ) ) {
-							return 0;
-						}


No need for this now that the timestamp is guaranteed to be set.

westonruter · 2024-02-22T06:28:49Z

modules/images/image-loading-optimization/storage/data.php

 */
-function ilo_unshift_url_metrics( array $url_metrics, array $validated_url_metric, array $breakpoints, int $sample_size ): array {
-	$validated_url_metric['timestamp'] = microtime( true );


This is now set more naturally in ilo_handle_rest_request()

westonruter · 2024-02-22T06:29:13Z

modules/images/image-loading-optimization/storage/data.php

-		if ( ! isset( $url_metric['viewport']['width'] ) ) {
-			continue;
-		}
-


No need for this anymore since the property is guaranteed to exist.

westonruter · 2024-02-22T06:31:06Z

modules/images/image-loading-optimization/storage/post-type.php

 	}

 	return array_values(
 		array_filter(
-			$url_metrics,
-			static function ( $url_metric ) use ( $trigger_error ) {
-				// TODO: If we wanted, we could use the JSON Schema to validate the stored metrics.


This TODO is now done

westonruter · 2024-02-22T06:31:52Z

modules/images/image-loading-optimization/storage/rest-api.php

-						return true;
-					},
-				),
-				'viewport' => array(


Schema definition is moved to ILO_URL_Metrics.

github-actions · 2024-02-22T06:42:34Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

modules/images/image-loading-optimization/class-ilo-url-metric.php

adamsilverstein · 2024-02-27T14:44:01Z

modules/images/image-loading-optimization/storage/post-type.php

+					}
+
+					try {
+						// TODO: This is re-validating the data which has been stored in the post type. This ensures it remains valid, but is it overkill?


feels fine to me, defensive coding

My only hesitation is that this additional validation adds latency, so it negatively impacts TTFB. However, I haven't profiled the JSON Schema validation logic, so this may not be a big deal.

adamsilverstein

Looks good, left one comment

felixarntz

@westonruter This mostly looks great. Just a few small points of feedback.

modules/images/image-loading-optimization/class-ilo-url-metric.php

modules/images/image-loading-optimization/storage/post-type.php

modules/images/image-loading-optimization/storage/rest-api.php

…trics

modules/images/image-loading-optimization/class-ilo-url-metric.php

modules/images/image-loading-optimization/storage/rest-api.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

felixarntz

@westonruter A few additional thoughts for consideration.

felixarntz · 2024-02-28T23:06:23Z

modules/images/image-loading-optimization/class-ilo-url-metric.php

+final class ILO_URL_Metric implements JsonSerializable {
+
+	/**
+	 * Data.
+	 *
+	 * @var array{
+	 *          timestamp: int,
+	 *          viewport: array{ width: int, height: int },
+	 *          elements: array<array{
+	 *              isLCP: bool,
+	 *              isLCPCandidate: bool,
+	 *              xpath: string,
+	 *              intersectionRatio: float,
+	 *              intersectionRect: array{ width: int, height: int },
+	 *              boundingClientRect: array{ width: int, height: int },
+	 *          }>
+	 *      }
+	 */
+	private $data;


Wouldn't it be valuable to store the $url and $slug in this class as well? Maybe they could be separate properties from $data, but I find it a bit odd to have a class for a URL metric exclude those identifying variables.

I think it could simplify some things by putting the responsibility for this information purely into the class representing the entity. We could still use associative arrays keyed by URL or slug if we need it, but I think it would be cleaner to have it covered by this class in its entirety - unless I'm missing something.

Alternatively, if there are scenarios where URL and slug should be decoupled from the actual metric data, maybe we could separately introduce a class that is a value object representing URL and slug, and it could also provide the methods to create and validate the nonces etc. Just an idea.

Well, the thing is that there are multiple URL metrics stored for a given response. The post type stores this list of URL metrics, based on the number of breakpoints and the sample size. So if we store the URL and slug with each ILO_URL_Metric we're storing multiple copies of the same data. The URL/slug is more higher level metadata which is only needed for the post type. We don't need the URL or slug except to get/set the post. I'd prefer to not include the URL or slug in this class.

Well, let me think about this some more.

Maybe we merge this as-is and I propose the change in another PR? I want to revisit how the URL is handled because in reality it is the request args really that matter, not the end URL. The slug is computed from the request args, not the URL.

I've added this as a todo item on #869

Ultimately, I think it would actually make sense to store a url with each URL Metric because in actuality the URL can vary while the request args change. This could be important for debugging, for example if it turns out that a template is modified by direct access to $_GET and not for a registered query var.

But I want to address this in a subsequent PR.

Oh I see you added more comments in the meantime. Let me know what you think about the above proposal.

But in any case, this would be okay to do in a follow up PR.

Yeah, let's do in a separate PR. It would actually be useful to duplicate the URL and even the request args for each URL metric since it reveals the underlying data for the response that created the URL metric. The slug used in the post type is computed from a normalized set of request args, so having the original request args would still be useful to have on hand.

Sounds great!

felixarntz · 2024-02-28T23:07:34Z

modules/images/image-loading-optimization/storage/post-type.php

 * @return int|WP_Error Post ID or WP_Error otherwise.
 */
-function ilo_store_url_metric( string $url, string $slug, array $validated_url_metric ) {
+function ilo_store_url_metric( string $url, string $slug, ILO_URL_Metric $new_url_metric ) {


See my above comment: I think a function like this could be made more intuitive if ILO_URL_Metric covered the URL and slug as well. This way you'd only pass around the object, not the other two values in addition.

felixarntz · 2024-02-28T23:08:27Z

modules/images/image-loading-optimization/storage/rest-api.php

+		'url'   => array(
+			'type'              => 'string',
+			'description'       => __( 'The URL for which the metric was obtained.', 'performance-lab' ),
+			'required'          => true,
+			'format'            => 'uri',
+			'validate_callback' => static function ( $url ) {
+				if ( ! wp_validate_redirect( $url ) ) {
+					return new WP_Error( 'non_origin_url', __( 'URL for another site provided.', 'performance-lab' ) );
+				}
+				// TODO: This is not validated as corresponding to the slug in any way. True it is not used for anything but metadata.
+				return true;
+			},
+		),
+		'slug'  => array(
+			'type'        => 'string',
+			'description' => __( 'An MD5 hash of the query args.', 'performance-lab' ),
+			'required'    => true,
+			'pattern'     => '^[0-9a-f]{32}$',
+			// This is validated via the nonce validate_callback, as it is provided as input to create the nonce by the server
+			// which then is verified to match in the REST API request.
+		),


See above, this could be tied to the ILO_URL_Metric class.

westonruter added 4 commits February 14, 2024 09:55

Add ILO_URL_Metric to contain JSON schema

0e652fe

Begin to use ILO_URL_Metric as data container and rework setting time…

3c6b153

…stamp

Utilize ILO_URL_Metric and its methods in codebase

0530a69

Eliminate ArrayAccess

d8228c2

westonruter added [Type] Feature A new feature within an existing module [Focus] Images Issues related to the Images focus area no milestone PRs that do not have a defined milestone for release [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Feb 15, 2024

westonruter added 2 commits February 14, 2024 16:53

Modify timestamp schema for REST API request

e87e3ac

Add more strict typing

3f91818

thelovekesh reviewed Feb 21, 2024

View reviewed changes

modules/images/image-loading-optimization/class-ilo-url-metric.php Outdated Show resolved Hide resolved

westonruter added 3 commits February 21, 2024 14:53

Move data member to top of class

0963753

Also move up constructor to top of class

cc9e956

Add typing for get_elements return value

eab177c

westonruter commented Feb 22, 2024

View reviewed changes

westonruter added 5 commits February 21, 2024 16:48

Improve formatting

87f518f

Use integer for timestamp in tests for sake of comparison

56346fd

Replace get_viewport_width with get_viewport to return array

6cafe3a

Tidy up more static analysis issues

db11827

Address additional PhpStorm inspections

af95769

westonruter commented Feb 22, 2024

View reviewed changes

westonruter added 3 commits February 21, 2024 22:38

Remove need for HtmlUnknownTarget suppression

6073274

Add return type for usort callback

4f3119c

Update parameter description

adf37d0

westonruter marked this pull request as ready for review February 22, 2024 06:42

westonruter requested a review from mukeshpanchal27 February 22, 2024 06:42

westonruter requested a review from adamsilverstein February 22, 2024 06:42

westonruter changed the title ~~Use classes to encapsulate data structures used in Image Loading Optimization~~ Introduce ILO_URL_Metric to encapsulate core data structure Feb 22, 2024

westonruter mentioned this pull request Feb 23, 2024

Introduce ILO_URL_Metrics_Group and ILO_URL_Metrics_Group_Collection classes #1010

Merged

3 tasks

adamsilverstein reviewed Feb 27, 2024

View reviewed changes

modules/images/image-loading-optimization/class-ilo-url-metric.php Show resolved Hide resolved

adamsilverstein reviewed Feb 27, 2024

View reviewed changes

adamsilverstein approved these changes Feb 27, 2024

View reviewed changes

westonruter requested a review from felixarntz February 27, 2024 16:31

felixarntz reviewed Feb 28, 2024

View reviewed changes

westonruter added 4 commits February 28, 2024 13:54

Add dedicated test coverage for ILO_URL_Metric

fcbfd42

Include more context in error messages when parsing/validating URL Me…

c5765e2

…trics

Always validate data and introduce dedicated exception class

c19c371

Rework timestamp handling in schema

afc13ff

westonruter requested a review from felixarntz February 28, 2024 22:44

Make endpoint registration code more concise

949642d

felixarntz reviewed Feb 28, 2024

View reviewed changes

modules/images/image-loading-optimization/class-ilo-url-metric.php Outdated Show resolved Hide resolved

modules/images/image-loading-optimization/storage/rest-api.php Outdated Show resolved Hide resolved

Fix PHP doc spacing

e148091

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

felixarntz reviewed Feb 28, 2024

View reviewed changes

westonruter mentioned this pull request Feb 28, 2024

Optimization Detective: Server-applied optimizations informed by client-side detection #869

Open

24 tasks

westonruter requested a review from felixarntz February 29, 2024 00:31

felixarntz approved these changes Feb 29, 2024

View reviewed changes

westonruter merged commit 47a1d0a into feature/image-loading-optimization Feb 29, 2024
37 checks passed

westonruter deleted the add/url-metric-class branch February 29, 2024 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ILO_URL_Metric to encapsulate core data structure #988

Introduce ILO_URL_Metric to encapsulate core data structure #988

westonruter commented Feb 15, 2024 •

edited

westonruter Feb 22, 2024 •

edited

westonruter Feb 22, 2024

westonruter Feb 22, 2024

westonruter Feb 22, 2024

westonruter Feb 22, 2024

westonruter Feb 22, 2024

github-actions bot commented Feb 22, 2024 •

edited

adamsilverstein Feb 27, 2024

westonruter Feb 27, 2024

adamsilverstein left a comment

felixarntz left a comment

felixarntz left a comment

felixarntz Feb 28, 2024

westonruter Feb 28, 2024

westonruter Feb 28, 2024

westonruter Feb 28, 2024

westonruter Feb 28, 2024

westonruter Feb 28, 2024

felixarntz Feb 28, 2024

westonruter Feb 28, 2024

felixarntz Feb 29, 2024

westonruter Mar 11, 2024

felixarntz Feb 28, 2024

felixarntz Feb 28, 2024

Introduce ILO_URL_Metric to encapsulate core data structure #988

Introduce ILO_URL_Metric to encapsulate core data structure #988

Conversation

westonruter commented Feb 15, 2024 • edited

Summary

Relevant technical choices

Checklist

westonruter Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Feb 15, 2024 •

edited

westonruter Feb 22, 2024 •

edited

github-actions bot commented Feb 22, 2024 •

edited