Skip to content
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.

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

Send post ID of queried object or first post in loop in URL Metric storage request to schedule page cache validation #1641

Merged
merged 11 commits into from
Nov 15, 2024
Merged
16 changes: 16 additions & 0 deletions plugins/optimization-detective/class-od-url-metric.php
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ public function set_group( OD_URL_Metric_Group $group ): void {
/**
* Gets JSON schema for URL Metric.
*
* @since 0.1.0
*
* @todo Cache the return value?
*
* @return array<string, mixed> Schema.
Expand Down Expand Up @@ -407,6 +409,8 @@ public function get( string $key ) {
/**
* Gets UUID.
*
* @since 0.6.0
*
* @return string UUID.
*/
public function get_uuid(): string {
Expand All @@ -416,6 +420,8 @@ public function get_uuid(): string {
/**
* Gets URL.
*
* @since 0.1.0
*
* @return string URL.
*/
public function get_url(): string {
Expand All @@ -425,6 +431,8 @@ public function get_url(): string {
/**
* Gets viewport data.
*
* @since 0.1.0
*
* @return ViewportRect Viewport data.
*/
public function get_viewport(): array {
Expand All @@ -434,6 +442,8 @@ public function get_viewport(): array {
/**
* Gets viewport width.
*
* @since 0.1.0
*
* @return int Viewport width.
*/
public function get_viewport_width(): int {
Expand All @@ -443,6 +453,8 @@ public function get_viewport_width(): int {
/**
* Gets timestamp.
*
* @since 0.1.0
*
* @return float Timestamp.
*/
public function get_timestamp(): float {
Expand All @@ -452,6 +464,8 @@ public function get_timestamp(): float {
/**
* Gets elements.
*
* @since 0.1.0
*
* @return OD_Element[] Elements.
*/
public function get_elements(): array {
Expand All @@ -469,6 +483,8 @@ function ( array $element ): OD_Element {
/**
* Specifies data which should be serialized to JSON.
*
* @since 0.1.0
*
* @return Data Exports to be serialized by json_encode().
*/
public function jsonSerialize(): array {
Expand Down
23 changes: 18 additions & 5 deletions plugins/optimization-detective/detect.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ function extendElementData( xpath, properties ) {
* @param {string} args.restApiEndpoint URL for where to send the detection data.
* @param {string} args.currentUrl Current URL.
* @param {string} args.urlMetricSlug Slug for URL Metric.
* @param {number|null} args.cachePurgePostId Cache purge post ID.
* @param {string} args.urlMetricHMAC HMAC for URL Metric storage.
* @param {URLMetricGroupStatus[]} args.urlMetricGroupStatuses URL Metric group statuses.
* @param {number} args.storageLockTTL The TTL (in seconds) for the URL Metric storage lock.
Expand All @@ -253,6 +254,7 @@ export default async function detect( {
restApiEndpoint,
currentUrl,
urlMetricSlug,
cachePurgePostId,
urlMetricHMAC,
urlMetricGroupStatuses,
storageLockTTL,
Expand Down Expand Up @@ -457,16 +459,21 @@ export default async function detect( {
continue;
}

const isLCP =
elementIntersection.target === lcpMetric?.entries[ 0 ]?.element;
const element = /** @type {Element|null} */ (
lcpMetric?.entries[ 0 ]?.element
);
const isLCP = elementIntersection.target === element;

/** @type {ElementData} */
const elementData = {
isLCP,
isLCPCandidate: !! lcpMetricCandidates.find(
( lcpMetricCandidate ) =>
lcpMetricCandidate.entries[ 0 ]?.element ===
elementIntersection.target
( lcpMetricCandidate ) => {
const candidateElement = /** @type {Element|null} */ (
lcpMetricCandidate.entries[ 0 ]?.element
);
return candidateElement === elementIntersection.target;
}
Comment on lines -460 to +476
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just to work around a static analysis issue I get complained about in my IDE. It's because web-vitals defines the Element interface but it is not the same Element defined in the IDE's DOM types.

),
xpath,
intersectionRatio: elementIntersection.intersectionRatio,
Expand Down Expand Up @@ -532,6 +539,12 @@ export default async function detect( {

const url = new URL( restApiEndpoint );
url.searchParams.set( 'slug', urlMetricSlug );
if ( typeof cachePurgePostId === 'number' ) {
url.searchParams.set(
'cache_purge_post_id',
cachePurgePostId.toString()
);
}
url.searchParams.set( 'hmac', urlMetricHMAC );
navigator.sendBeacon(
url,
Expand Down
53 changes: 52 additions & 1 deletion plugins/optimization-detective/detection.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,54 @@
exit; // Exit if accessed directly.
}

/**
* Obtains the ID for a post related to this response so that page caches can be told to invalidate their cache.
*
* If the queried object for the response is a post, then that post's ID is used. Otherwise, it uses the ID of the first
* post in The Loop.
*
* When the queried object is a post (e.g. is_singular, is_posts_page, is_front_page w/ show_on_front=page), then this
* is the perfect match. A page caching plugin will be able to most reliably invalidate the cache for a URL via
* this ID if the relevant actions are triggered for the post (e.g. clean_post_cache, save_post, transition_post_status).
*
* Otherwise, if the response is an archive page or the front page where show_on_front=posts (i.e. is_home), then
* there is no singular post object that represents the URL. In this case, we obtain the first post in the main
* loop. By triggering the relevant actions for this post ID, page caches will have their best shot at invalidating
* the related URLs. Page caching plugins which leverage surrogate keys will be the most reliable here. Otherwise,
* caching plugins may just resort to automatically purging the cache for the homepage whenever any post is edited,
* which is better than nothing.
*
* There should not be any situation by default in which a page optimized with Optimization Detective does not have such
* a post available for cache purging. As seen in {@see od_can_optimize_response()}, when such a post ID is not
* available for cache purging then it returns false, as it also does in another case like if is_404().
*
* @since n.e.x.t
* @access private
*
* @return int|null Post ID or null if none found.
Copy link
Member

Choose a reason for hiding this comment

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

Nit-pick, but why not stick with int and return 0 if none found? 0 can never be an ID anyway and is false-y too.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. But null seems to be more universally semantic to indicate nothing was found. If null then you can check that case with either is_null() or !is_int(). And null is also false-y.

Copy link
Member

@felixarntz felixarntz Nov 15, 2024

Choose a reason for hiding this comment

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

My counter-argument is that int is a single type, and the check can simply be made with ! $post_id either way. Not a big deal either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Except not with our strict PHPStan rules in place 😄

*/
function od_get_cache_purge_post_id(): ?int {
$queried_object = get_queried_object();
if ( $queried_object instanceof WP_Post ) {
return $queried_object->ID;
}

global $wp_query;
if (
$wp_query instanceof WP_Query
&&
$wp_query->post_count > 0
&&
isset( $wp_query->posts[0] )
&&
$wp_query->posts[0] instanceof WP_Post
) {
return $wp_query->posts[0]->ID;
}

return null;
}

/**
* Prints the script for detecting loaded images and the LCP element.
*
Expand All @@ -32,6 +80,8 @@ function od_get_detection_script( string $slug, OD_URL_Metric_Group_Collection $
*/
$extension_module_urls = (array) apply_filters( 'od_extension_module_urls', array() );

$cache_purge_post_id = od_get_cache_purge_post_id();

$current_url = od_get_current_url();
$detect_args = array(
'minViewportAspectRatio' => od_get_minimum_viewport_aspect_ratio(),
Expand All @@ -41,7 +91,8 @@ function od_get_detection_script( string $slug, OD_URL_Metric_Group_Collection $
'restApiEndpoint' => rest_url( OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ),
'currentUrl' => $current_url,
'urlMetricSlug' => $slug,
'urlMetricHMAC' => od_get_url_metrics_storage_hmac( $slug, $current_url ),
'cachePurgePostId' => od_get_cache_purge_post_id(),
'urlMetricHMAC' => od_get_url_metrics_storage_hmac( $slug, $current_url, $cache_purge_post_id ),
'urlMetricGroupStatuses' => array_map(
static function ( OD_URL_Metric_Group $group ): array {
return array(
Expand Down
5 changes: 4 additions & 1 deletion plugins/optimization-detective/optimization.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ function od_can_optimize_response(): bool {
// users, additional elements will be present like the script from wp_customize_support_script() which will
// interfere with the XPath indices. Note that od_get_normalized_query_vars() is varied by is_user_logged_in()
// so membership sites and e-commerce sites will still be able to be optimized for their normal visitors.
current_user_can( 'customize' )
current_user_can( 'customize' ) ||
// Page caching plugins can only reliably be told to invalidate a cached page when a post is available to trigger
// the relevant actions on.
null !== od_get_cache_purge_post_id()
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was wrong!!! It should have been:

null === od_get_cache_purge_post_id()

See #1659

);

/**
Expand Down
12 changes: 12 additions & 0 deletions plugins/optimization-detective/readme.txt
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,18 @@ add_filter(

See also [example usage](https://github.com/WordPress/performance/blob/6bb8405c5c446e3b66c2bfa3ae03ba61b188bca2/plugins/embed-optimizer/hooks.php#L128-L144) in Embed Optimizer. Note in particular the structure of the plugin’s [detect.js](https://github.com/WordPress/performance/blob/trunk/plugins/embed-optimizer/detect.js) script module, how it exports `initialize` and `finalize` functions which Optimization Detective then calls when the page loads and when the page unloads, at which time the URL Metric is constructed and sent to the server for storage. Refer also to the [TypeScript type definitions](https://github.com/WordPress/performance/blob/trunk/plugins/optimization-detective/types.ts).

**Action:** `od_url_metric_stored` (argument: `OD_URL_Metric_Store_Request_Context`)

Fires whenever a URL Metric was successfully stored.

The supplied context object includes these properties:

* `$request`: The `WP_REST_Request` for storing the URL Metric.
* `$post_id`: The post ID for the `od_url_metric` post.
* `$url_metric`: The newly-stored URL Metric.
* `$url_metric_group`: The viewport group that the URL Metric was added to.
* `$url_metric_group_collection`: The `OD_URL_Metric_Group_Collection` instance to which the URL Metric was added.

== Installation ==

= Installation from within WordPress =
Expand Down
23 changes: 12 additions & 11 deletions plugins/optimization-detective/storage/data.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,15 @@ function od_get_url_metrics_slug( array $query_vars ): string {
*
* @see od_verify_url_metrics_storage_hmac()
* @see od_get_url_metrics_slug()
* @todo This should also include an ETag as a parameter. See <https://github.com/WordPress/performance/issues/1466>.
*
* @param string $slug Slug (hash of normalized query vars).
* @param string $url URL.
*
* @param string $slug Slug (hash of normalized query vars).
* @param string $url URL.
* @param int|null $cache_purge_post_id Cache purge post ID.
* @return string HMAC.
*/
function od_get_url_metrics_storage_hmac( string $slug, string $url ): string {
$action = "store_url_metric:$slug:$url";
function od_get_url_metrics_storage_hmac( string $slug, string $url, ?int $cache_purge_post_id = null ): string {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

Also, should this maybe be a required parameter? That would emphasize that it's needed to optimize responses anyway, as you already check above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, should this maybe be a required parameter? That would emphasize that it's needed to optimize responses anyway, as you already check above.

The check I added means that by default it will not offer to optimize responses when od_get_cache_purge_post_id() returns null. But this can be overridden with the od_can_optimize_response filter. So we're not guaranteed that it will always be not-null.

Copy link
Member

Choose a reason for hiding this comment

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

What would be a use-case for that? I'm thinking it might be better to not make that possible. For example, we could make it so that the filter only is applied when there is a post ID. That feels more robust to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes I've made sites where I use author archive pages to create team bio pages, for example. In such a case, there often isn't any blog post actually authored by that user. Nevertheless, if I go to their author page it doesn't actually return a 404. It serves a page with the author's name in the document title and if the theme has an author.php template which is configured to display the user bio even when there are no posts, then it will work as intended. For example, the Twenty Ten theme shows the user bio on author.php even when there are no posts by that author:

image

$action = "store_url_metric:$slug:$url:$cache_purge_post_id";
return wp_hash( $action, 'nonce' );
}

Expand All @@ -170,14 +171,14 @@ function od_get_url_metrics_storage_hmac( string $slug, string $url ): string {
* @see od_get_url_metrics_storage_hmac()
* @see od_get_url_metrics_slug()
*
* @param string $hmac HMAC.
* @param string $slug Slug (hash of normalized query vars).
* @param String $url URL.
*
* @param string $hmac HMAC.
* @param string $slug Slug (hash of normalized query vars).
* @param String $url URL.
* @param int|null $cache_purge_post_id Cache purge post ID.
* @return bool Whether the HMAC is valid.
*/
function od_verify_url_metrics_storage_hmac( string $hmac, string $slug, string $url ): bool {
return hash_equals( od_get_url_metrics_storage_hmac( $slug, $url ), $hmac );
function od_verify_url_metrics_storage_hmac( string $hmac, string $slug, string $url, ?int $cache_purge_post_id = null ): bool {
Copy link
Member

Choose a reason for hiding this comment

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

See above. Maybe make this required?

return hash_equals( od_get_url_metrics_storage_hmac( $slug, $url, $cache_purge_post_id ), $hmac );
}

/**
Expand Down
72 changes: 67 additions & 5 deletions plugins/optimization-detective/storage/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,28 @@
*/
function od_register_endpoint(): void {

// The slug and cache_purge_post_id args are further validated via the validate_callback for the 'hmac' parameter,
// they are provided as input with the 'url' argument to create the HMAC by the server.
$args = array(
'slug' => array(
'slug' => array(
'type' => 'string',
'description' => __( 'An MD5 hash of the query args.', 'optimization-detective' ),
'required' => true,
'pattern' => '^[0-9a-f]{32}$',
// This is further validated via the validate_callback for the 'hmac' parameter, as it is provided as input
// with the 'url' argument to create the HMAC by the server. which then is verified to match in the REST API request.
),
'hmac' => array(
'cache_purge_post_id' => array(
'type' => 'integer',
'description' => __( 'Cache purge post ID.', 'optimization-detective' ),
'required' => false,
'minimum' => 1,
),
'hmac' => array(
'type' => 'string',
'description' => __( 'HMAC originally computed by server required to authorize the request.', 'optimization-detective' ),
'required' => true,
'pattern' => '^[0-9a-f]+$',
'validate_callback' => static function ( string $hmac, WP_REST_Request $request ) {
if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request->get_param( 'slug' ), $request->get_param( 'url' ) ) ) {
if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request['slug'], $request['url'], $request['cache_purge_post_id'] ?? null ) ) {
return new WP_Error( 'invalid_hmac', __( 'URL Metrics HMAC verification failure.', 'optimization-detective' ) );
}
return true;
Expand Down Expand Up @@ -202,6 +208,16 @@ function od_handle_rest_request( WP_REST_Request $request ) {
}
$post_id = $result;

// Schedule an event in 10 minutes to trigger an invalidation of the page cache (hopefully).
$cache_purge_post_id = $request->get_param( 'cache_purge_post_id' );
if ( is_int( $cache_purge_post_id ) && false === wp_next_scheduled( 'od_trigger_page_cache_invalidation', array( $cache_purge_post_id ) ) ) {
wp_schedule_single_event(
time() + 10 * MINUTE_IN_SECONDS,
'od_trigger_page_cache_invalidation',
array( $cache_purge_post_id )
);
}

/**
* Fires whenever a URL Metric was successfully stored.
*
Expand All @@ -226,3 +242,49 @@ function od_handle_rest_request( WP_REST_Request $request ) {
)
);
}

/**
* Triggers actions for page caches to invalidate their caches related to the supplied cache purge post ID.
*
* This is intended to flush any page cache for the URL after the new URL Metric was submitted so that the optimizations
* which depend on that URL Metric can start to take effect.
*
* @since n.e.x.t
* @access private
*
* @param int $cache_purge_post_id Cache purge post ID.
*/
function od_trigger_page_cache_invalidation( int $cache_purge_post_id ): void {
$post = get_post( $cache_purge_post_id );
if ( ! ( $post instanceof WP_Post ) ) {
return;
}

// Fire actions that page caching plugins listen to flush caches.

/*
* The clean_post_cache action is used to flush page caches by:
* - Pantheon Advanced Cache <https://github.com/pantheon-systems/pantheon-advanced-page-cache/blob/e3b5552b0cb9268d9b696cb200af56cc044920d9/pantheon-advanced-page-cache.php#L185>
* - WP Super Cache <https://github.com/Automattic/wp-super-cache/blob/73b428d2fce397fd874b3056ad3120c343bc1a0c/wp-cache-phase2.php#L1615>
* - Batcache <https://github.com/Automattic/batcache/blob/ed0e6b2d9bcbab3924c49a6c3247646fb87a0957/batcache.php#L18>
*/
/** This action is documented in wp-includes/post.php. */
do_action( 'clean_post_cache', $post->ID, $post );

/*
* The transition_post_status action is used to flush page caches by:
* - Jetpack Boost <https://github.com/Automattic/jetpack-boost-production/blob/4090a3f9414c2171cd52d8a397f00b0d1151475f/app/modules/optimizations/page-cache/pre-wordpress/Boost_Cache.php#L76>
* - WP Super Cache <https://github.com/Automattic/wp-super-cache/blob/73b428d2fce397fd874b3056ad3120c343bc1a0c/wp-cache-phase2.php#L1616>
* - LightSpeed Cache <https://github.com/litespeedtech/lscache_wp/blob/7c707469b3c88b4f45d9955593b92f9aeaed54c3/src/purge.cls.php#L68>
*/
/** This action is documented in wp-includes/post.php. */
do_action( 'transition_post_status', $post->post_status, $post->post_status, $post );
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this works well. I could see how caching plugins compare the before and after value, and this here would be a weird scenario that Core I believe never triggers (the same value in both).

Probably still better to keep this than not have it, but this may not work reliably.

Copy link
Member Author

Choose a reason for hiding this comment

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

Core does actually trigger when it is the same value for both. If you have published a post and then you update the post again, then this same transition_post_status action is triggered with the same publish value for both of those arguments. I just tested to confirm this.

And we need to keep this anyway because some caching plugins only listen for this action to decide whether to invalidate their caches.


/*
* The clean_post_cache action is used to flush page caches by:
* - W3 Total Cache <https://github.com/BoldGrid/w3-total-cache/blob/ab08f104294c6a8dcb00f1c66aaacd0615c42850/Util_AttachToActions.php#L32>
* - WP Rocket <https://github.com/wp-media/wp-rocket/blob/e5bca6673a3669827f3998edebc0c785210fe561/inc/common/purge.php#L283>
*/
/** This action is documented in wp-includes/post.php. */
do_action( 'save_post', $post->ID, $post, /* $update */ true );
}
Loading
Loading