Fix storage of URL Metric when plain non-pretty permalinks are enabled #1574

westonruter · 2024-10-02T22:47:21Z

The REST API merges params from GET, POST, and JSON into one params array. This causes problems when pretty permalinks, as the fetched URL is:

http://localhost:8888/index.php?rest_route=/optimization-detective/v1/url-metrics:store

Where the rest_route is erroneously picked up as one of the parameters.

To fix this, this PR separates meta parameters from the core URL metric parameters. The meta parameters (nonce and slug) are added as GET parameters whereas the parameters that comprise the URL metric data are all exclusively in the content body as JSON. This feels cleaner from a REST API perspective since the entity then becomes truly URL metric data, rather than URL metric with additional meta parameters added to it.

This also tidies up some symbol names, with "URL Metrics" (plural) being changed to "URL Metric" (singular) where appropriate.

Aside: Perhaps OD_URL_Metrics_Group_Collection should be renamed to OD_URL_Metric_Group_Collection as well. The "metrics" plural here is redundant with the group in the name.

github-actions · 2024-10-02T22:47:38Z

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: 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.

felixarntz

@westonruter This makes sense to me and looks good, however I'm curious: Does this actually fix a bug, or is the change only about improving the code?

You were previously removing slug and nonce from the REST get_params() anyway, which seems it would be the same as now using get_json_params(). You mention the rest_route query parameter, but I thought this would not get included anyway since it's not a supported parameter of the endpoint?

Aside: Perhaps OD_URL_Metrics_Group_Collection should be renamed to OD_URL_Metric_Group_Collection as well. The "metrics" plural here is redundant with the group in the name.

+1 for that. Since it seems nobody other than us is adopting the plugin yet, this is probably fine to just rename. Once we document this further and encourage external usage, we'll need to be more careful about this kind of change.

westonruter · 2024-10-02T23:12:50Z

Does this actually fix a bug, or is the change only about improving the code?

Yes, it fixes the bug, as well as improving the code. In reality, the most minimal patch to only fix the code would be to unset rest_route from the $params as well. The bug is that WordPress erroneously includes rest_route in the return value of get_params() even though it is not registered as one of the endpoint args.

So for non-pretty permalinks, using get_json_params() would be functionally the same as get_params() with the addition of the existing unset() of $data['nonce'] and $data['slug'].

Getting the URL Metric data from the JSON exclusively is safer as it's less likely that arbitrary query parameters will be added to the REST API URL to cause a data validation problem in the future. For example, if I had modified the plugin to add some cachebuster query parameter:

--- a/plugins/optimization-detective/detect.js
+++ b/plugins/optimization-detective/detect.js
@@ -392,7 +392,9 @@ export default async function detect( {
 	} );
 
 	try {
-		const response = await fetch( restApiEndpoint, {
+		const restUrl = new URL( restApiEndpoint );
+		restUrl.searchParams.append( 'cachebust', '123' );
+		const response = await fetch( restUrl, {
 			method: 'POST',
 			headers: {
 				'Content-Type': 'application/json',

(Such a parameter could be added by a proxy layer even.)

Then I get the same error even when pretty permalinks are enabled:

Failed to validate URL metric: cachebust is not a valid property of Object.

So again, it seems like a core bug that get_params() returns all of the GET and POST params, even those which are not registered.

westonruter · 2024-10-02T23:51:50Z

+1 for that. Since it seems nobody other than us is adopting the plugin yet, this is probably fine to just rename. Once we document this further and encourage external usage, we'll need to be more careful about this kind of change.

@felixarntz Done in 942b397, although it touched quite a bit of code.

westonruter · 2024-10-03T00:57:18Z

plugins/optimization-detective/class-od-tag-visitor-context.php

-		$this->processor                    = $processor;
-		$this->url_metrics_group_collection = $url_metrics_group_collection;
-		$this->link_collection              = $link_collection;
+	public function __get( string $name ): OD_URL_Metric_Group_Collection {


This is needed to account for someone updating Optimization Detective but not right away updating Image Prioritizer or Embed Optimizer. They are using the url_metrics_group_collection symbol, so without this back-compat code, a fatal error would occur. This is similar to following code in Image Prioritizer:

performance/plugins/image-prioritizer/class-image-prioritizer-img-tag-visitor.php

Lines 69 to 88 in 5952d55

} elseif (

is_string( $processor->get_attribute( 'fetchpriority' ) )

&&

// Temporary condition in case someone updates Image Prioritizer without also updating Optimization Detective.

method_exists( $context->url_metrics_group_collection, 'is_any_group_populated' )

&&

$context->url_metrics_group_collection->is_any_group_populated()

) {

/*

* At this point, the element is not the shared LCP across all viewport groups. Nevertheless, server-side

* heuristics have added fetchpriority=high to the element, but this is not warranted either due to a lack

* of data or because the LCP element is not common across all viewport groups. Since we have collected at

* least some URL metrics (per is_any_group_populated), further below a fetchpriority=high preload link will

* be added for the viewport(s) for which this is actually the LCP element. Some viewport groups may never

* get populated due to a lack of traffic (e.g. from tablets or phablets), so it is important to remove

* fetchpriority=high in such case to prevent server-side heuristics from prioritizing loading the image

* which isn't actually the LCP element for actual visitors.

*/

$processor->remove_attribute( 'fetchpriority' );

}

The is_any_group_populated() method was added to Optimization Detective in a recent release, and to avoid a fatal error if someone updates Optimization Detective without updating Image Prioritizer as well, this prevents the code from causing a fatal error.

All of this code could be removed later once there is no longer this risk.

Let's add a TODO to remove this later.

Added in 5960ff3

westonruter · 2024-10-03T00:59:16Z

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

 		array(
-			'post_id'                      => $post_id,
-			'request'                      => $request,
-			'url_metric'                   => $url_metric,
-			'url_metrics_group'            => $group,
-			'url_metrics_group_collection' => $group_collection,
+			'post_id'                     => $post_id,
+			'request'                     => $request,
+			'url_metric'                  => $url_metric,
+			'url_metric_group'            => $url_metric_group,
+			'url_metric_group_collection' => $url_metric_group_collection,
 		)


Instead of using an array here, it would be better to implement something like OD_Tag_Visitor_Context which has properties instead of array keys. This would allow for autocompletion as well as it would support possible future renaming of the members.

Like this: 1cf24f1

… OD_URL_Metric_Stored_Context instead of array

westonruter · 2024-10-03T01:32:07Z

plugins/optimization-detective/storage/class-od-url-metric-stored-context.php

+	 * @param OD_URL_Metric_Group            $url_metric_group            URL metric group.
+	 * @param OD_URL_Metric                  $url_metric                  URL metric.
+	 */
+	public function __construct( WP_REST_Request $request, int $post_id, OD_URL_Metric_Group_Collection $url_metric_group_collection, OD_URL_Metric_Group $url_metric_group, OD_URL_Metric $url_metric ) {


This class has a terrible amount of redundancy. However, in newer versions of PHP this can be made much simpler, thanks to Constructor Promotion.

So this could be replaced with:

final class OD_URL_Metric_Stored_Context { public function __construct( public WP_REST_Request $request, public int $post_id, public OD_URL_Metric_Group_Collection $url_metric_group_collection, public OD_URL_Metric_Group $url_metric_group, public OD_URL_Metric $url_metric ) {} }

This works in PHP 8.0+: https://3v4l.org/OqvTC

So we can't take advantage of it. But it will greatly simplify this class and OD_Tag_Visitor_Context.

How do you mean redundancy? Just because the properties are declared in several places?

Yes. There are 5 instances of the url_metric_group_collection string in this class file when there could just be 1.

But that's just about how PHP (and many other languages work) right? I wouldn't necessarily call that redundancy, it's just more code to write. :)

Correct. The verbosity can be reduced in the future.

felixarntz · 2024-10-03T15:49:45Z

Does this actually fix a bug, or is the change only about improving the code?

Yes, it fixes the bug, as well as improving the code. In reality, the most minimal patch to only fix the code would be to unset rest_route from the $params as well. The bug is that WordPress erroneously includes rest_route in the return value of get_params() even though it is not registered as one of the endpoint args.

[...]

So again, it seems like a core bug that get_params() returns all of the GET and POST params, even those which are not registered.

Is there a Trac ticket for this? If not, could you please open one? :)

felixarntz · 2024-10-03T15:45:03Z

plugins/optimization-detective/class-od-tag-visitor-context.php

+ *
+ * @property-read OD_URL_Metric_Group_Collection $url_metrics_group_collection


I don't think it's worth adding this, given it's only for backward compatibility.

It's needed for static analysis. In 5960ff3 I've added a note that it is deprecated.

felixarntz · 2024-10-03T15:45:57Z

plugins/optimization-detective/class-od-tag-visitor-context.php

-		$this->processor                    = $processor;
-		$this->url_metrics_group_collection = $url_metrics_group_collection;
-		$this->link_collection              = $link_collection;
+	public function __get( string $name ): OD_URL_Metric_Group_Collection {


Let's add a TODO to remove this later.

felixarntz · 2024-10-03T15:48:09Z

plugins/optimization-detective/readme.txt

@@ -205,7 +205,7 @@ The [plugin source code](https://github.com/WordPress/performance/tree/trunk/plu

 **Enhancements**

-* Log URL metrics group collection to console when debugging is enabled (`WP_DEBUG` is true). ([1295](https://github.com/WordPress/performance/pull/1295))
+* Log URL metric group collection to console when debugging is enabled (`WP_DEBUG` is true). ([1295](https://github.com/WordPress/performance/pull/1295))


Nit-pick, but I don't think we should update older changelogs as they are "history artifacts" (e.g. we probably wouldn't update this in an old GitHub release either). Let's revert this.

Reverted in ef9f3ae

felixarntz · 2024-10-03T15:52:49Z

plugins/optimization-detective/storage/class-od-url-metric-stored-context.php

+	 * @param OD_URL_Metric_Group            $url_metric_group            URL metric group.
+	 * @param OD_URL_Metric                  $url_metric                  URL metric.
+	 */
+	public function __construct( WP_REST_Request $request, int $post_id, OD_URL_Metric_Group_Collection $url_metric_group_collection, OD_URL_Metric_Group $url_metric_group, OD_URL_Metric $url_metric ) {


How do you mean redundancy? Just because the properties are declared in several places?

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

westonruter · 2024-10-03T16:20:35Z

Is there a Trac ticket for this? If not, could you please open one? :)

I've just opened Core-62163

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

felixarntz · 2024-10-03T16:37:34Z

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

-			'url_metrics_group'            => $group,
-			'url_metrics_group_collection' => $group_collection,
+		'od_url_metric_stored',
+		new OD_URL_Metric_Stored_Context(


Since the only reason for this class to exist is the filter, I think we should clarify that in the name, which could be confusing if you look at the class alone.

Either we go with OD_URL_Metric_Context, which sounds like a good name, but could have the risk of sounding more generic than it actually is. Or we go with OD_URL_Metric_Stored_Filter_Context, which keeps the scope narrow as it is now, but clarifies that it's for a filter. I think the latter was unclear to me when I first saw the class, and I was wondering what "stored context" is.

How about eaed94f?

Co-authored-by: Felix Arntz <felixarntz@google.com>

westonruter added 2 commits October 2, 2024 15:23

Exclusively use JSON params to pass into OD_Strict_URL_Metric

bc478e4

Pass nonce and slug as query params since not part of URL Metric

b515f87

westonruter added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Oct 2, 2024

westonruter added this to the optimization-detective n.e.x.t milestone Oct 2, 2024

westonruter requested a review from felixarntz as a code owner October 2, 2024 22:47

westonruter self-assigned this Oct 2, 2024

westonruter mentioned this pull request Oct 2, 2024

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

Merged

2 tasks

felixarntz reviewed Oct 2, 2024

View reviewed changes

Remove the plural from URL Metrics in symbols

942b397

westonruter requested a review from adamsilverstein as a code owner October 2, 2024 23:51

Use non-abbreviated variable names in od_url_metric_collected action

b833e1b

westonruter commented Oct 3, 2024

View reviewed changes

Rename od_url_metric_collected action to od_url_metric_stored and use…

1cf24f1

… OD_URL_Metric_Stored_Context instead of array

westonruter commented Oct 3, 2024

View reviewed changes

felixarntz reviewed Oct 3, 2024

View reviewed changes

westonruter added 2 commits October 3, 2024 09:01

Indicate deprecated nature of url_metrics_group_collection property

5960ff3

Revert changelog change

ef9f3ae

westonruter requested a review from felixarntz October 3, 2024 16:20

Add temporary class aliases

5549091

felixarntz reviewed Oct 3, 2024

View reviewed changes

westonruter and others added 2 commits October 3, 2024 09:56

Use more descriptive description

adc0c2e

Co-authored-by: Felix Arntz <felixarntz@google.com>

Rename context class

eaed94f

felixarntz approved these changes Oct 3, 2024

View reviewed changes

westonruter merged commit 1e82f7b into trunk Oct 3, 2024
18 of 20 checks passed

westonruter deleted the fix/od-plain-permalinks-compat branch October 3, 2024 18:07

westonruter added [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) labels Oct 3, 2024

westonruter mentioned this pull request Feb 6, 2025

Add post ID for the od_url_metrics post to the tag visitor context #1847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix storage of URL Metric when plain non-pretty permalinks are enabled #1574

Fix storage of URL Metric when plain non-pretty permalinks are enabled #1574

westonruter commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

felixarntz left a comment

westonruter commented Oct 2, 2024 •

edited

Loading

westonruter commented Oct 2, 2024

westonruter Oct 3, 2024

felixarntz Oct 3, 2024

westonruter Oct 3, 2024

westonruter Oct 3, 2024

westonruter Oct 3, 2024

westonruter Oct 3, 2024

felixarntz Oct 3, 2024

westonruter Oct 3, 2024 •

edited

Loading

felixarntz Oct 3, 2024

westonruter Oct 3, 2024

felixarntz commented Oct 3, 2024

felixarntz Oct 3, 2024

westonruter Oct 3, 2024

felixarntz Oct 3, 2024

felixarntz Oct 3, 2024

westonruter Oct 3, 2024

felixarntz Oct 3, 2024

westonruter commented Oct 3, 2024

felixarntz Oct 3, 2024

westonruter Oct 3, 2024

felixarntz Oct 3, 2024

	} elseif (
	is_string( $processor->get_attribute( 'fetchpriority' ) )
	&&
	// Temporary condition in case someone updates Image Prioritizer without also updating Optimization Detective.
	method_exists( $context->url_metrics_group_collection, 'is_any_group_populated' )
	&&
	$context->url_metrics_group_collection->is_any_group_populated()
	) {
	/*
	* At this point, the element is not the shared LCP across all viewport groups. Nevertheless, server-side
	* heuristics have added fetchpriority=high to the element, but this is not warranted either due to a lack
	* of data or because the LCP element is not common across all viewport groups. Since we have collected at
	* least some URL metrics (per is_any_group_populated), further below a fetchpriority=high preload link will
	* be added for the viewport(s) for which this is actually the LCP element. Some viewport groups may never
	* get populated due to a lack of traffic (e.g. from tablets or phablets), so it is important to remove
	* fetchpriority=high in such case to prevent server-side heuristics from prioritizing loading the image
	* which isn't actually the LCP element for actual visitors.
	*/
	$processor->remove_attribute( 'fetchpriority' );
	}

		*
		* @property-read OD_URL_Metric_Group_Collection $url_metrics_group_collection

Fix storage of URL Metric when plain non-pretty permalinks are enabled #1574

Fix storage of URL Metric when plain non-pretty permalinks are enabled #1574

Conversation

westonruter commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

westonruter commented Oct 2, 2024 • edited Loading

westonruter commented Oct 2, 2024

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 Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz commented Oct 3, 2024

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 Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

westonruter commented Oct 2, 2024 •

edited

Loading

westonruter Oct 3, 2024 •

edited

Loading