Allow URL metric schema to be extended

[westonruter]

Discussed previously:

• Capture userAgent in URL metric when debugging #1490 (comment)
• [POC] Performance Dashboard (Web Vitals) #1098 (comment)
• [POC] Performance Dashboard (Web Vitals) #1098 (comment)

This PR introduces extensibility for the URL Metric. Two new filters are introduced:

• od_url_metric_schema_root_additional_properties
• od_url_metric_schema_element_item_additional_properties

These allow for new properties to be added to the root of the URL Metric and to the schema for an element, respectively.

For example, to capture whether a user agent was using a touch interface:

add_filter(
	'od_url_metric_schema_root_additional_properties',
	static function ( $additional_properties ) {
		$additional_properties['isTouch'] = array(
			'type' => 'boolean',
		);
		return $additional_properties;
	}
);

Or for example, to add a resizedHeight property to an element, you can do:

add_filter(
	'od_url_metric_schema_element_item_additional_properties',
	static function ( $additional_properties ) {
		$additional_properties['resizedHeight'] = array(
			'type' => 'number',
		);
		return $additional_properties;
	}
);

Additional properties are not allowed to be required and they must not conflict with existing core properties. When a URL Metric is stored with additional properties, these properties will persist even when the plugin that added the schema for the additional property is deactivated. This is to avoid invalidating many URL metrics immediately upon deactivating an extension. Additional unrecognized properties are always disallowed when submitting new URL metrics for storage (via the new OD_Strict_URL_Metric class), but existing stored URL metrics allow unrecognized properties to persist.

(In order to populate the extended properties in the URL Metric, see #1373 for the client-side module extensions.)

This PR also introduces improved ergonomics for accessing properties of the OD_URL_Metric class, including a get( $property ) method (which is key for accessing extended properties) as well as magic getters for core properties:

• $url_metric->uuid
• $url_metric->url
• $url_metric->timestamp
• $url_metric->viewport
• $url_metric->elements

Additionally, the OD_URL_Metric class's validation logic has now been extended to also apply sanitization to the inputs. This is to ensure that the expected PHP types are being passed around, since the string 'false' is a valid boolean according to the WordPress REST API, but it is not a PHP boolean false. This could cause PHP type errors at some point. So now rest_sanitize_value_from_schema() is called after first doing rest_validate_object_value_from_schema().

Lastly, a od_url_metric_collected action now fires whenever a new URL Metric is stored. This would be useful for extensions to perhaps aggregate URL Metrics data in another table such as to power a dashboard showing real user metrics, see #1324.

To do

• Add filters so that additional properties can be added to the URL metric root schema and the schema for elements items.
• Add get() method to OD_URL_Metric to allow arbitrary parts of the URL metric data to be obtained. See [POC] Performance Dashboard (Web Vitals) #1098 (comment).
• Make sure that filters that add additional root and element item properties don't cause existing URL metrics to all become invalid. An update to a URL metric should cause the additional properties to be removed.
• Add filter to allow new server-side data to be amended to a URL metric when it is created in od_handle_rest_request(). (Data will be able to be amended client-side via extension module scripts in Leverage URL metrics to reserve space for embeds to reduce CLS #1373.)

[swissspidy]

The whole method is a bit complex with all the validation in it. Is there some existing WP function we could use for schema validation instead? Or does WP do this validation later on anyway?

I don't see similar validation in register_rest_field for instance.

[westonruter]

WordPress does have a _doing_it_wrong() check which emits:

Function rest_validate_value_from_schema was called incorrectly. The "type" schema keyword for OD_URL_Metric[timestamp] is required. (This message was added in version 5.5.0.)

But then it goes ahead and tries to access it anyway, resulting in a warning:

Maybe the validation is excessive.

[felixarntz]

I think the validation here makes sense, specifically because some of those requirements we have here don't apply to the WordPress validation (for instance, we want to prohibit required, but that's only relevant here, not in general WordPress validation of REST properties).

[github-actions]

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: swissspidy <swissspidy@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 The logic looks good mostly, I left a few comments though mostly on architecture bits.

[felixarntz]

I'd say either yes, or we should at least automatically set it based on the type (e.g. '' for type string etc). Since all of these parameters need to be optional, there needs to be a default I guess?

[westonruter]

See #1492 (comment). I think we should force the default to always be null.

[westonruter]

Hummm. I had thought that rest_sanitize_value_from_schema() would supply the default value when it is absent, but it doesn't. Therefore, I think default should just be disallowed.

[westonruter]

I've just disallowed the default altogether in aa1e663.

[felixarntz]

Not sure about the value of this and __get(). Seems unrelated to the purpose of this PR anyway, so maybe let's discuss that separately? I think the get() method is good enough?

[westonruter]

In #1098 (comment) you suggested __call() and I responded there.

[felixarntz]

That's fair. This works.

My only concern with such an approach is that it only works reasonably well if another plugin extending it would also have its own class that it annotated with these @property-read annotations. Otherwise, it becomes an incomplete documentation, and in that case I feel it might even be better to just have get(), so that it's more obvious to a developer they need to "manually" look at what's available - whereas these annotations may imply that you can simply rely on them, when as soon as an extension happens, this may not longer be the case.

So I think I still prefer not to have this, but not a big deal if you want to keep it.

[westonruter]

That's true. Plugins that extend the schema wouldn't have type definitions for these properties. However, having them for the core properties is useful so that we don't have to add get_*() methods for every root property. I just added 384fa53 & ed58151 to make use of these. So it's more ergonomic and feels better. (I think we can deprecate get_uuid(), get_timestamp(), get_elements(), etc.)

For plugins that extend the schema, this ties back to the discussion of having default values for the properties. This makes me think that we should force the default to be null so that isset() checks will be false. It doesn't make sense to have a non-null default defined when the client never submitted data for that URL metric in the first place. Having a default value of null will avoid cases where array indices will be undefined, but at the same time isset() checks will be false. Update: See #1492 (comment).

As for plugins that extend the schema, I'm thinking about Embed Optimizer adding a resizedBoundingClientRect property for the element. This isn't actually directly connected to the @property-read discussion, but it is related in static analysis. The Embed Optimizer plugin could access it from an element via:

foreach ( $url_metric->elements as $element ) {
	if ( null !== $element['resizedBoundingClientRect'] ) {
		// ...
	}
}

However, PHPStan here complains:

phpstan: Offset 'resizedBoundingClie…' does not exist on array{isLCP: bool, isLCPCandidate: bool, xpath: string, intersectionRatio: float, intersectionRect: array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}, boundingClientRect: array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}}.

This makes sense because $element is typed as ElementData which does not have resizedBoundingClientRect. (Aside: I think ElementData should perhapsbe renamed to ElementMetrics.)

I tried adding the following to the Embed_Optimizer_Tag_Visitor class:

 * @phpstan-import-type DOMRect from OD_URL_Metric
 * @phpstan-import-type ElementData from OD_URL_Metric
 * @phpstan-type ExtendedElementData ElementData & array{ resizedBoundingClientRect: ?DOMRect }

And then explicitly overriding the type for $element:

/**
 * Extended element.
 *
 * @var ExtendedElementData $element
 */

But it's not working. I'm getting either errors like:

PHPDoc tag @var for variable $element contains unresolvable type.

Or when moving @phpstan-type ExtendedElementData to the method, I instead get:

phpstan: Access to offset 'resizedBoundingClie…' on an unknown class ExtendedElementData.
phpstan: PHPDoc tag @var for variable $element contains unknown class ExtendedElementData.
phpstan: PHPDoc tag @var with type ExtendedElementData is not subtype of type array{isLCP: bool, isLCPCandidate: bool, xpath: string, intersectionRatio: float, intersectionRect: array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}, boundingClientRect: array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}}.

I'm nearing my PHPStan knowledge limits. Maybe it just can't do what I want.

[felixarntz]

I think it comes down to the aforementioned problem with magic getters and @property-read. You can't programmatically guarantee that any of this will work. Technically every single use of the magic getter would need to be wrapped in a magic "isset-er". Otherwise, the code is not "safe" and you may at any point unexpectedly work with null values.

That's related to my previous concern, and I don't think we should deprecate the getter methods in favor of magic properties. I think the win in ergonomics is tiny - only about not having to write get_, which IMO is not a good reason to change this, given that using the methods is a more reliable approach, which PHPStan should be fine with.

For any custom properties, we can use the get( ... ) method, which is not ideal either, but still could facilitate a cleaner approach where an extension would implement its own class that implements additional get_...() methods for new properties.

[westonruter]

Actually I think the problem here is not related to @property-read, since this is regarding accessing keys of the items in the elements array. It's the same problem if using get_elements() with this code:

foreach ( $url_metric->get_elements() as $element ) {
	if ( null !== $element['resizedBoundingClientRect'] ) {
		// ...
	}
}

The problem is that $element is typed as ElementData which doesn't include the resizedBoundingClientRect key. But this code here is assuming the null was provided as a default. Let's consider that it may not be set at all. In that case, consider this code:

foreach ( $url_metric->elements as $element ) {
	if ( isset( $element['resizedBoundingClientRect'] ) ) {
		/**
		 * Resized bounding client rect.
		 *
		 * @var DOMRect $rect
		 */
		$rect = $element['resizedBoundingClientRect'];
		PHPStan\dumpType( $rect );
	}
}

Here we have the isset() check and considering that the schema has enforced the expected type for resizedBoundingClientRect, PHPStan here outputs:

Dumped type: array{width: float, height: float, x: float, y: float, top: float, right: float, bottom: float, left: float}

This is what we want. However, it would be ideal if we didn't have to use @var and PHPStan could be told directly that $element['resizedBoundingClientRect'] is of type DOMRect|null.

Otherwise, for built-in root-level keys like elements, uuid, timestamp they will never return null because the schema enforces them to have a value. For the non-builtin properties, however, they indeed may not be set. So let's say there's a new root property called cwv_metrics which the Dashboard (#1324) may introduce. This could be accessed in this PR in the following ways:

PHPStan\dumpType( $url_metric->get( 'web_vitals' ) );

Dumped type: mixed

No type information as expected since get() returns mixed|null. However, this with the magic getter is worse, even though __get() is defined the same:

PHPStan\dumpType( $url_metric->web_vitals )

Access to an undefined property OD_URL_Metric::$web_vitals.
💡 Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property  
Dumped type: *ERROR*

That helpful Learn more link may have the answer and I'll investigate further tomorrow. In particular, it may come down to writing our own PHPStan extension. This could ensure that PHPStan detects the correct types for both get() and magic getters, as well as for extended elements item schemas.

[felixarntz]

Writing a PHPStan extension sounds like overkill to me. I think we should try to make it work against its constraints.

Regarding the mixed return value of the get() method, I don't think that's a problem. Wouldn't an extension still be able to introduce a wrapper methods where it casts it to what's expected and comes with proper return type hint and doc? I think that's sufficient to alleviate the concerns.

And for extension that don't want to go the extra mile, they can still do it the "dirty" way of directly calling get() wherever they need an extra property and then checking for existence and cast the value as needed.

On the other hand, undefined properties is a real problem, as reported by PHPStan. That's why I would continue to advise against using a magic getter. I just don't see the value.

[westonruter]

The magic getter is only relevant to the root properties. The issue exists for extensions to element data. An extension plugin could indeed do its own type declarations in helper wrapper functions, for example:

	/**
	 * Gets the resized bounding client rect.
	 *
	 * @phpstan-param ElementData $element
	 *
	 * @param array $element Element.
	 * @return DOMRect|null Whether visible or null if unknown.
	 */
	public static function get_resized_bounding_client_rect( array $element ): ?array {
		if ( isset( $element['resizedBoundingClientRect'] ) ) {
			return $element['resizedBoundingClientRect'];
		}
		return null;
	}

	public function do_something( OD_HTML_Tag_Processor $processor, OD_URL_Metric $url_metric ): void {
		foreach ( $url_metric->get_elements() as $element ) {
			$rect = self::get_resized_bounding_client_rect( $element );
			if ( isset( $rect ) ) {
				PHPStan\dumpType( $rect['height'] ); // => Dumped type: float
			}
		}
	}

It would be nicer to not have to resort to get_resized_bounding_client_rect and just do:

	public function do_something( OD_HTML_Tag_Processor $processor, OD_URL_Metric $url_metric ): void {
		foreach ( $url_metric->get_elements() as $element ) {
			if ( isset( $element['resizedBoundingClientRect'] ) ) {
				PHPStan\dumpType( $element['resizedBoundingClientRect']['height'] );
			}
		}
	}

But this results in Dumped type: mixed.

Informing PHPStan how to resolve the extended schema would ensure the expected types are used without having to duplicate typing information which is already being enforced by validation and sanitization with the schema.

For root properties it also happens, without magic getters:

	/**
	 * Gets the CWV metrics.
	 *
	 * @param OD_URL_Metric $url_metric URL Metric.
	 * @return array{ lcp: float, inp: ?float, cls: float }|null CWV data or null if not set.
	 */
	public static function get_cwv_metrics( OD_URL_Metric $url_metric ): ?array {
		$cwv_metrics = $url_metric->get( 'cwv_metrics' );
		if ( is_array( $cwv_metrics ) ) {
			/**
			 * CWV metrics.
			 *
			 * @var array{ lcp: float, inp: ?float, cls: float } $cwv_metrics
			 */
			return $cwv_metrics;
		}
		return null;
	}

	public function do_something( OD_URL_Metric $url_metric ): void {
		$cwv_metrics = self::get_cwv_metrics( $url_metric );
		if ( isset( $cwv_metrics ) ) {
			PHPStan\dumpType( $cwv_metrics['lcp'] ); // => Dumped type: float
		}
	}

It would be preferable to just have to do:

	public function do_something( OD_URL_Metric $url_metric ): void {
		$cwv_metrics = $url_metric->get( 'cwv_metrics' );
		if ( isset( $cwv_metrics ) ) {
			PHPStan\dumpType( $cwv_metrics['lcp'] );
		}
	}

But again, this results in Dumped type: mixed.

I don't think the use of a magic getter is necessarily a problem. I believe static analysis would be able to deal with them. But it isn't necessary. I've removed it in 0db296f. I would still like to explore the use of a PHPStan extension. It's designed to be extensible for cases like this.

[felixarntz]

I think the validation here makes sense, specifically because some of those requirements we have here don't apply to the WordPress validation (for instance, we want to prohibit required, but that's only relevant here, not in general WordPress validation of REST properties).

[felixarntz]

What would be an example use-case for this action? Can be an unrealistic example, I'm just curious to see how you envision this to be used.

[swissspidy]

For the performance dashboard POC it's useful as I could then hook into this action, grab my relevant data, and persistently and permanently store it in a new location more suitable for that kind of analytics. Useful since OD metrics are deleted after a while.

[felixarntz]

Okay, so this is mostly for more advanced/custom use-cases then? I was wondering since this PR adds it, whether it's something you should be using just because you add additional metrics. But that's not the case, correct?

[westonruter]

That's right, it's not directly related to extending the schema, but it is related to extending the plugin in general.

[felixarntz]

It's a bit confusing to have "strict" URL metric extend the "extendable" URL metric - would make more sense to be the other way around. This is also highlighted by how the strict class will still run the filters, but without that serving any purpose.

Taking this a bit further, strictly speaking neither of the two makes sense because a strict URL metric is not an extendable URL metric and vice-versa. So it would make more sense to use e.g. a decorator pattern, or make the two classes separate implementations of a OD_URL_Metric interface. Of course that would be a decent amount of refactoring to do - simple, but still some code changes so we could consider that separately.

So at this point I mostly think:

• Should the extension tree be the other way around? If not, why not?
• Maybe there's a more clear name we can use than just "strict"? Maybe include "REST" so that it's clear that this class is only used for REST API requests?

[westonruter]

I'm not sure I understand. The filters are still used. It's just that any objects have additionalProperties set to false in the strict version.

[westonruter]

In other words, in the strict version, it sets additionalProperties to false which refers to the properties which aren't recognized as part of the schema. The filters are still applying, however, so if a plugin adds new properties to the schema these "additional" properties will be recognized. It's just that additional unrecognized properties will be rejected in the strict verison.

[felixarntz]

Ah right, I had something wrong in my logic here. Some of my other feedback on the class hierarchy still applies, but that's not a big deal in regards to this PR. I do think we should separately consider using a OD_URL_Metric interface and use that in all (or at least most of) the type hints so that the class implementations can be independent.

[westonruter]

This could make use of generics to specify the expected return type given the input $key.

[westonruter]

Maybe not, as then attempting to access an extended property will result in a PHPStan error.

[felixarntz]

Per https://github.com/WordPress/performance/pull/1492/files#r1752966800, I'd prefer if we removed this and continued to rely on getter methods, plus the get( $key ) method as a somewhat okay solution for properties added by an extension.

[felixarntz]

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]

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:

[westonruter]

Undone in 0db296f

[felixarntz]

@westonruter I opened #1532 because of the failing tests unrelated to this PR.

[westonruter]

@felixarntz I don't think what I want to explore further with a PHPStan extension to add type information from the extended schema need block this PR from being merged. So if you're happy with this PR as-is, we can merge.