-
Notifications
You must be signed in to change notification settings - Fork 842
Improve Post Stats cache handling for invalid or error data #46211
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
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
| update_post_meta( $post_id, $meta_name, array( time() => $wpcom_stats ) ); | ||
|
|
||
| // Don't write error or empty results to cache. | ||
| if ( ! is_wp_error( $wpcom_stats ) && ! empty( $wpcom_stats ) ) { |
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 would lean to caching even if it's an error, so that we don't put too much pressure on the backend when something goes wrong. However, we need to make sure the data expires no matter it's an error or not. What do you think?
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, that's also what I thought about as an alternative. We can always respect the cache for 5 minutes, regardless of whether it was an error or invalid data. That means even if the data is incorrect, it would last at most 5 minutes, wouldn't it?
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.
Updated in 4ba2b0e.
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
projects/packages/stats/src/class-wpcom-stats.php:560
- The new
refresh_post_stats_cache()function and the cache recovery logic infetch_post_stats()lack test coverage. The existing test suite has comprehensive tests for other stats methods (e.g.,test_get_post_views,test_get_stats, etc.) but none for the post meta caching path.
Consider adding tests for:
- Cache recovery when malformed data exists in post meta
- Cache recovery when WP_Error is cached
- Cache recovery when non-array, non-time-keyed data exists
- The new
refresh_post_stats_cache()function behavior
protected function fetch_post_stats( $args, $post_id ) {
$endpoint = $this->build_endpoint();
$meta_name = '_' . self::STATS_CACHE_TRANSIENT_PREFIX;
$stats_cache = get_post_meta( $post_id, $meta_name, false );
if ( $stats_cache ) {
$data = reset( $stats_cache );
// Check if we have a valid cache structure with a time key.
if ( is_array( $data ) && ! empty( $data ) ) {
$time = key( $data );
// If we have a numeric time, check if cache is still valid.
if ( is_numeric( $time ) ) {
/** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */
$expiration = apply_filters(
'jetpack_fetch_stats_cache_expiration',
self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS
);
// If within cache period, return cached data regardless of validity.
if ( ( time() - $time ) < $expiration ) {
$cached_value = $data[ $time ];
// If it's a WP_Error, return it directly.
if ( is_wp_error( $cached_value ) ) {
return $cached_value;
}
// If it's an array, merge with cached_at timestamp.
if ( is_array( $cached_value ) ) {
return array_merge( array( 'cached_at' => $time ), $cached_value );
}
// For any other type, return as-is.
return $cached_value;
}
}
}
}
// Cache doesn't exist, is expired, or is malformed - refresh it.
return $this->refresh_post_stats_cache( $endpoint, $args, $post_id, $meta_name );
}
/**
* Force fetch stats from WPCOM, and update cache if needed.
*
* @param string $endpoint The stats endpoint.
* @param array $args The query arguments.
* @param int $post_id The post ID.
* @param string $meta_name The meta name.
*
* @return array|WP_Error
*/
protected function refresh_post_stats_cache( $endpoint, $args, $post_id, $meta_name ) {
$wpcom_stats = $this->fetch_remote_stats( $endpoint, $args );
// Always cache the result, even if it's an error or empty.
update_post_meta( $post_id, $meta_name, array( time() => $wpcom_stats ) );
return $wpcom_stats;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $time = key( $data ); | ||
| $views = $data[ $time ] ?? null; | ||
| // If within cache period, return cached data regardless of validity. |
Copilot
AI
Dec 9, 2025
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.
The comment "return cached data regardless of validity" is misleading. The code immediately validates the cached value by checking if it's a WP_Error or array (lines 522-530). Consider updating the comment to: "If within cache period, return cached data after type validation."
| // If within cache period, return cached data regardless of validity. | |
| // If within cache period, return cached data after type validation. |
| // For any other type, return as-is. | ||
| return $cached_value; |
Copilot
AI
Dec 9, 2025
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.
Lines 532-533 will return any non-array, non-WP_Error cached value as-is, which could perpetuate invalid cached data (e.g., null, string, boolean) for the entire cache period. This contradicts the PR's goal of "detecting and recovering from invalid cache entries."
Consider either:
- Only returning the cached value if it's an array or WP_Error, falling through to refresh otherwise
- Adding explicit validation that cached_value is an expected type
Example:
// For any other unexpected type, treat as malformed cache.
// Fall through to refresh.Then remove the return statement and let execution continue to line 540.
| // For any other type, return as-is. | |
| return $cached_value; | |
| // For any other unexpected type, treat as malformed cache. | |
| // Fall through to refresh. |
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.
Agree with Copilot here - other cases should all fall to malformed types?
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.
other cases should all fall to malformed types?
Looks reasonable to me, like only leave expected WP_Error thrown from cache.
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.
Updated in 68d3fe3.
| // If it's a WP_Error, return it directly. | ||
| if ( is_wp_error( $cached_value ) ) { | ||
| return $cached_value; | ||
| } | ||
|
|
||
| /** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */ | ||
| $expiration = apply_filters( | ||
| 'jetpack_fetch_stats_cache_expiration', | ||
| self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS | ||
| ); | ||
| // If it's an array, merge with cached_at timestamp. | ||
| if ( is_array( $cached_value ) ) { | ||
| return array_merge( array( 'cached_at' => $time ), $cached_value ); | ||
| } |
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 guess we don't have to tell them apart - for those two cases we could both return:
return array_merge( array( 'cached_at' => $time ), $cached_value );
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.
| // If it's a WP_Error, return it directly. | |
| if ( is_wp_error( $cached_value ) ) { | |
| return $cached_value; | |
| } | |
| /** This filter is already documented in projects/packages/stats/src/class-wpcom-stats.php */ | |
| $expiration = apply_filters( | |
| 'jetpack_fetch_stats_cache_expiration', | |
| self::STATS_CACHE_EXPIRATION_IN_MINUTES * MINUTE_IN_SECONDS | |
| ); | |
| // If it's an array, merge with cached_at timestamp. | |
| if ( is_array( $cached_value ) ) { | |
| return array_merge( array( 'cached_at' => $time ), $cached_value ); | |
| } | |
| // If it's an array or WP_Error, add cached time and return to user. | |
| if ( is_array( $cached_value ) || is_wp_error( $cached_value ) ) { | |
| return array_merge( array( 'cached_at' => $time ), (array)$cached_value ); | |
| } |
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.
Updated in 68d3fe3.
Co-authored-by: Copilot <[email protected]>
|
@dognose24 I've opened a new pull request, #46235, to work on those changes. Once the pull request is ready, I'll request review from you. |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
kangzj
left a comment
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.
Looks good and tests well! Thanks so much for all the efforts 👍
Fixes STATS-184.
This PR fixes an issue where the post stats cache could get stuck on invalid data or a WP_Error and never refresh. When corrupted or malformed cache entries exist in post meta, subsequent calls to
fetch_post_stats()would always return that bad data instead of refetching from WPCOM.With this change, invalid or error cache entries trigger a fresh remote fetch and a self-healing cache update, while still honoring the existing cache expiration mechanism for valid data.
Proposed changes:
refresh_post_stats_cache()helper to handle thefetch from WPCOM + update post metaflowfetch_post_stats()Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
This individual postviews count of Blog Stats in any post is correctly aligned with/stats/post/{post-id}/{site-slug}.