Introduce ILO_URL_Metric to encapsulate core data structure #988
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| <?php | ||
| /** | ||
| * Image Loading Optimization: ILO_URL_Metric class | ||
| * | ||
| * @package performance-lab | ||
| * @since n.e.x.t | ||
| */ | ||
|
|
||
| /** | ||
| * Representation of the measurements taken from a single client's visit to a specific URL. | ||
| * | ||
| * @since n.e.x.t | ||
| * @access private | ||
| */ | ||
| 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; | ||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @param array $data URL metric data. | ||
westonruter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * @param bool $validated Whether the data was already validated. | ||
| * | ||
| * @throws Exception When the input is invalid. | ||
| */ | ||
| public function __construct( array $data, bool $validated = false ) { | ||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ( ! $validated ) { | ||
| $valid = rest_validate_object_value_from_schema( $data, self::get_json_schema(), self::class ); | ||
| if ( is_wp_error( $valid ) ) { | ||
| throw new Exception( esc_html( $valid->get_error_message() ) ); | ||
| } | ||
| } | ||
| $this->data = $data; | ||
| } | ||
|
|
||
| /** | ||
| * Gets JSON schema for URL Metric. | ||
| * | ||
| * @return array | ||
| */ | ||
| public static function get_json_schema(): array { | ||
| $dom_rect_schema = array( | ||
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'width' => array( | ||
| 'type' => 'number', | ||
| 'minimum' => 0, | ||
| ), | ||
| 'height' => array( | ||
| 'type' => 'number', | ||
| 'minimum' => 0, | ||
| ), | ||
| ), | ||
| // TODO: There are other properties to define if we need them: x, y, top, right, bottom, left. | ||
| 'additionalProperties' => true, | ||
| ); | ||
|
|
||
| return array( | ||
| '$schema' => 'http://json-schema.org/draft-04/schema#', | ||
| 'title' => 'ilo-url-metric', | ||
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'viewport' => array( | ||
| 'description' => __( 'Viewport dimensions', 'performance-lab' ), | ||
| 'type' => 'object', | ||
| 'required' => true, | ||
| 'properties' => array( | ||
| 'width' => array( | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| 'minimum' => 0, | ||
| ), | ||
| 'height' => array( | ||
| 'type' => 'integer', | ||
| 'required' => true, | ||
| 'minimum' => 0, | ||
| ), | ||
| ), | ||
| ), | ||
| 'timestamp' => array( | ||
| 'description' => __( 'Timestamp at which the URL metric was captured.', 'performance-lab' ), | ||
| 'type' => 'number', | ||
| 'required' => true, | ||
| 'minimum' => 0, | ||
| ), | ||
| 'elements' => array( | ||
| 'description' => __( 'Element metrics', 'performance-lab' ), | ||
| 'type' => 'array', | ||
| 'required' => true, | ||
| 'items' => array( | ||
| // See the ElementMetrics in detect.js. | ||
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'isLCP' => array( | ||
| 'type' => 'boolean', | ||
| 'required' => true, | ||
| ), | ||
| 'isLCPCandidate' => array( | ||
| 'type' => 'boolean', | ||
| ), | ||
| 'xpath' => array( | ||
| 'type' => 'string', | ||
| 'required' => true, | ||
| 'pattern' => ILO_HTML_Tag_Processor::XPATH_PATTERN, | ||
| ), | ||
| 'intersectionRatio' => array( | ||
| 'type' => 'number', | ||
| 'required' => true, | ||
| 'minimum' => 0.0, | ||
| 'maximum' => 1.0, | ||
| ), | ||
| 'intersectionRect' => $dom_rect_schema, | ||
| 'boundingClientRect' => $dom_rect_schema, | ||
| ), | ||
| 'additionalProperties' => false, | ||
| ), | ||
| ), | ||
| ), | ||
| 'additionalProperties' => false, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Gets viewport width. | ||
| * | ||
| * @return array{ width: int, height: int } | ||
| */ | ||
| public function get_viewport(): array { | ||
| return $this->data['viewport']; | ||
| } | ||
|
|
||
| /** | ||
| * Gets timestamp. | ||
| * | ||
| * @return float | ||
| */ | ||
| public function get_timestamp(): float { | ||
| return $this->data['timestamp']; | ||
| } | ||
|
|
||
| /** | ||
| * Gets elements. | ||
| * | ||
| * @return array<array{ | ||
| * isLCP: bool, | ||
| * isLCPCandidate: bool, | ||
| * xpath: string, | ||
| * intersectionRatio: float, | ||
| * intersectionRect: array{ width: int, height: int }, | ||
| * boundingClientRect: array{ width: int, height: int }, | ||
| * }> | ||
| */ | ||
| public function get_elements(): array { | ||
| return $this->data['elements']; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the JSON representation of the object. | ||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * @return array | ||
| */ | ||
| public function jsonSerialize(): array { | ||
| return $this->data; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be valuable to store the
$urland$slugin 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_Metricwe'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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, let me think about this some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this as a todo item on #869
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, I think it would actually make sense to store a
urlwith 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$_GETand not for a registered query var.But I want to address this in a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
slugused 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1043