Skip to content

Allow URL metric schema to be extended #1492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
westonruter merged 17 commits into from
Sep 13, 2024
Merged

Allow URL metric schema to be extended #1492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a52381d
Allow URL metric schema to be extended
westonruter Aug 21, 2024
e244b22
Add get() and __get() methods to OD_URL_Metric
westonruter Aug 21, 2024
079e6d9
Account for type being an array
westonruter Aug 22, 2024
bbc97dc
Add od_url_metric_collected action
westonruter Aug 22, 2024
70fd825
Add todos related to store_url_metric()
westonruter Aug 22, 2024
381c829
Adopt Yoast SEO scheme for filter _doing_it_wrong
westonruter Sep 6, 2024
e85c683
Fix and clarify message for disallowed required property in extended …
westonruter Sep 6, 2024
0cc9103
Add sanitization in addition to validation to ensure expected PHP types
westonruter Sep 6, 2024
00d57b4
Persist unknown properties in stored URL Metrics but reject in new UR…
westonruter Sep 7, 2024
22711e4
Add return description
westonruter Sep 10, 2024
384fa53
Use properties instead of get methods
westonruter Sep 10, 2024
ed58151
Replace remaining use of get_timestamp
westonruter Sep 10, 2024
aa1e663
Disallow default value for schema
westonruter Sep 10, 2024
8917ce9
Explicitly fork rest_default_additional_properties_to_false()
westonruter Sep 10, 2024
907d8c3
Add return description for magic getter
westonruter Sep 10, 2024
0db296f
Eliminate magic getter
westonruter Sep 11, 2024
ea28aa1
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Use properties instead of get methods
  • Loading branch information
@westonruter
westonruter committed Sep 10, 2024
commit 384fa53f9de60b3080b515ab95a06070b50b1a04
4 changes: 2 additions & 2 deletions plugins/optimization-detective/class-od-url-metrics-group-collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ private function create_groups(): array {
*/
public function add_url_metric( OD_URL_Metric $new_url_metric ): void {
foreach ( $this->groups as $group ) {
if ( $group->is_viewport_width_in_range( $new_url_metric->get_viewport_width() ) ) {
if ( $group->is_viewport_width_in_range( $new_url_metric->viewport['width'] ) ) {

@felixarntz felixarntz Sep 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to what I said in https://github.com/WordPress/performance/pull/1492/files#r1752966800, this here is an example where I would even argue it was more ergonomical before. get_viewport_width() is more intuitive to use than viewport['width'] since for the latter you have to know which keys are available within the viewport array. So that means it's not even always more ergonomical to use read-only properties.

@westonruter westonruter Sep 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this isn't necessarily more ergonomical. Nevertheless, the IDE should know that $new_url_metric->viewport is of type ViewportRect so it should autocomplete the keys:

image

@westonruter westonruter Sep 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undone in 0db296f

$group->add_url_metric( $new_url_metric );
return;
}
Expand Down Expand Up @@ -416,7 +416,7 @@ public function get_all_element_max_intersection_ratios(): array {
*/
foreach ( $this->groups as $group ) {
foreach ( $group as $url_metric ) {
foreach ( $url_metric->get_elements() as $element ) {
foreach ( $url_metric->elements as $element ) {
$element_max_intersection_ratios[ $element['xpath'] ] = array_key_exists( $element['xpath'], $element_max_intersection_ratios )
? max( $element_max_intersection_ratios[ $element['xpath'] ], $element['intersectionRatio'] )
: $element['intersectionRatio'];
Expand Down
6 changes: 3 additions & 3 deletions plugins/optimization-detective/class-od-url-metrics-group.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public function is_viewport_width_in_range( int $viewport_width ): bool {
* @param OD_URL_Metric $url_metric URL metric.
*/
public function add_url_metric( OD_URL_Metric $url_metric ): void {
if ( ! $this->is_viewport_width_in_range( $url_metric->get_viewport_width() ) ) {
if ( ! $this->is_viewport_width_in_range( $url_metric->viewport['width'] ) ) {
throw new InvalidArgumentException(
esc_html__( 'URL metric is not in the viewport range for group.', 'optimization-detective' )
);
Expand All @@ -201,7 +201,7 @@ public function add_url_metric( OD_URL_Metric $url_metric ): void {
usort(
$this->url_metrics,
static function ( OD_URL_Metric $a, OD_URL_Metric $b ): int {
return $b->get_timestamp() <=> $a->get_timestamp();
return $b->timestamp <=> $a->timestamp;
}
);

Expand Down Expand Up @@ -283,7 +283,7 @@ public function get_lcp_element(): ?array {
$breadcrumb_element = array();

foreach ( $this->url_metrics as $url_metric ) {
foreach ( $url_metric->get_elements() as $element ) {
foreach ( $url_metric->elements as $element ) {
if ( ! $element['isLCP'] ) {
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions plugins/optimization-detective/storage/class-od-url-metrics-post-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public static function store_url_metric( string $slug, OD_URL_Metric $new_url_me
// multiple URL Metric instances, each of which also contains the URL for which the metric was captured. The URL
// appearing in the post title is therefore the most recent URL seen for the URL Metrics which have the same
// normalized query vars among them.
'post_title' => $new_url_metric->get_url(),
'post_title' => $new_url_metric->url,
);

$post = self::get_post( $slug );
Expand All @@ -221,7 +221,7 @@ public static function store_url_metric( string $slug, OD_URL_Metric $new_url_me
);

try {
$group = $group_collection->get_group_for_viewport_width( $new_url_metric->get_viewport_width() );
$group = $group_collection->get_group_for_viewport_width( $new_url_metric->viewport['width'] );
$group->add_url_metric( $new_url_metric );
} catch ( InvalidArgumentException $e ) {
return new WP_Error( 'invalid_url_metric', $e->getMessage() );
Expand Down