Add Site Health check for Cache-Control: no-store page response header which disables bfcache
#1807
Conversation
b1ink0
added
[Type] Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #1807 +/- ##
==========================================
+ Coverage 65.47% 65.82% +0.35%
==========================================
Files 85 87 +2
Lines 6748 6827 +79
==========================================
+ Hits 4418 4494 +76
- Misses 2330 2333 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| $headers_to_check = array( 'no-store', 'no-cache', 'max-age=0' ); | ||
| $flagged_headers = array(); | ||
|
|
||
| foreach ( $headers_to_check as $header_to_check ) { | ||
| if ( is_int( strpos( $header, $header_to_check ) ) ) { | ||
| $flagged_headers[] = $header_to_check; | ||
| } | ||
| } | ||
|
|
||
| $flagged_headers_string = implode( ', ', $flagged_headers ); |
There was a problem hiding this comment.
I understand that Chrome is going to be enabling bfcache when Cache-Control: no-store is present per @tunetheweb's article: https://developer.chrome.com/docs/web-platform/bfcache-ccns
I'm somewhat unclear on the aspects of Cache-Control that we should be testing for. Currently this logic is checking for the presence of no-store, no-cache, and max-age=0. @gilbertococchi What are the conditions we should be testing for to ensure that bfcache won't be thwarted from Cache-Control?
There was a problem hiding this comment.
Hi Weston, it's true that Chrome is working on an effort to make Cache-Control: no-store usage compatible with BFCache but it looks like the logic heuristic will impact only a fraction of CCNS usage overall.
After the Experiment will roll out at 100%, CCNS usage will continue to block BFCache eligibility if there is any Set-Cookie header changes for the HTML document and also in case some APIs are used like WebSocket or WebRTC.
This is the reason that convinced me that is still good to guide developers to reconsider their usage of CCNS on WP CMS.
PS: not all CMS have the same CCNS usage according to the data I've measured in HTTPArchive, there are some that have almost 0% CCNS usage, others there usage is really high and I believe it's because of Caching Best Practices and other effects on the Developers ecosystem of that particular CMS.
westonruter
changed the title
Cache-Control headers to prevent bfcache from being disabled
| */ | ||
| function perflab_cch_check_cache_control_test(): array { | ||
| $result = array( | ||
| 'label' => __( 'Cache settings are optimal for site performance.', 'performance-lab' ), |
There was a problem hiding this comment.
Maybe:
| 'label' => __( 'Cache settings are optimal for site performance.', 'performance-lab' ), | |
| 'label' => __( 'The Cache-Control response header for pages ensures fast back/forward navigation.', 'performance-lab' ), |
| 'label' => __( 'Performance', 'performance-lab' ), | ||
| 'color' => 'blue', | ||
| ), | ||
| 'description' => '<p>' . esc_html__( 'Your site cache settings are configured correctly, helping to improve its performance.', 'performance-lab' ) . '</p>', |
There was a problem hiding this comment.
| 'description' => '<p>' . esc_html__( 'Your site cache settings are configured correctly, helping to improve its performance.', 'performance-lab' ) . '</p>', | |
| 'description' => '<p>' . esc_html__( 'If the Cache-Control response header includes directives like no-store, no-cache, or max-age=0 then it can prevent instant back/forward navigations (using the browser bfcache). Your site is configured properly.', 'performance-lab' ) . '</p>', |
plugins/performance-lab/includes/site-health/cache-control-headers/helper.php
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| $response = wp_remote_get( | ||
| home_url(), |
There was a problem hiding this comment.
To avoid potential redirect:
| home_url(), | |
| home_url( '/' ), |
| $result['description'] = '<p>' . wp_kses( | ||
| sprintf( | ||
| /* translators: %s is the error code */ | ||
| __( 'There was an error while checking your site cache settings. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), |
There was a problem hiding this comment.
Here "site cache settings" can be confused potentially with someone setting up their page caching plugin. Perhaps better to be explicit what is being tested, even if it is technical:
| __( 'There was an error while checking your site cache settings. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), | |
| __( 'There was an error while checking your Cache-Control response header. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), |
| $cache_control_header = wp_remote_retrieve_header( $response, 'cache-control' ); | ||
| if ( is_string( $cache_control_header ) && '' === $cache_control_header ) { | ||
| $result['label'] = __( 'Cache-Control headers are not set correctly', 'performance-lab' ); | ||
| $result['status'] = 'recommended'; | ||
| $result['description'] = sprintf( | ||
| '<p>%s</p>', | ||
| esc_html__( 'Cache-Control headers are not set correctly. This can affect the performance of your site.', 'performance-lab' ) | ||
| ); | ||
| return $result; | ||
| } |
There was a problem hiding this comment.
Is this condition correct? If it is an empty string then this means that no Cache-Control header was present, and this shouldn't cause any problem for bfcache.
Maybe it would be beneficial for it to have a value like public, max-aged=3600 but this wouldn't be related to the purpose of this test, and that is to ensure that bfcache is not broken.
There was a problem hiding this comment.
As the absence of a Cache-Control header does not affect bfcache I think we could just return the default $result in this condition.
| * | ||
| * @return array{label: string, status: string, badge: array{label: string, color: string}, description: string, actions: string, test: string} Result. | ||
| */ | ||
| function perflab_cch_check_cache_control_test(): array { |
There was a problem hiding this comment.
I suggest renaming this test and the labels/descriptions to specifically refer to bfcache somehome. Yes it is examining the Cache-Control header for page responses, but we're checking a subset for that, for specifically if it contains a value that would break bfcache.
There was a problem hiding this comment.
I changed the function names and its description in 7b6d5da.
Currently the test contains in cache-control-header folder should this also be renamed to something like bfcache-compatibility-headers?
There was a problem hiding this comment.
Yeah, I think that works. There are other ways for bfcache to be broken, such as using unload event handlers, so including headers makes sense. Otherwise, it could be just bfcache-compatibility. But since the scope is specifically for the HTTP response headers, I think this is good.
| '<p>%s</p>', | ||
| sprintf( | ||
| /* translators: %s: Cache-Control header value */ | ||
| esc_html__( 'Cache-Control headers are set to %s. This can affect the performance of your site.', 'performance-lab' ), |
There was a problem hiding this comment.
| esc_html__( 'Cache-Control headers are set to %s. This can affect the performance of your site.', 'performance-lab' ), | |
| esc_html__( 'Cache-Control headers are set to %s. This can affect the performance of your site by preventing fast back/forward navigations (via browser bfcache).', 'performance-lab' ), |
| $result['label'] = sprintf( | ||
| /* translators: %s: Cache-Control header value */ | ||
| __( 'Cache-Control headers are set to %s', 'performance-lab' ), | ||
| $flagged_headers_string | ||
| ); |
There was a problem hiding this comment.
| $result['label'] = sprintf( | |
| /* translators: %s: Cache-Control header value */ | |
| __( 'Cache-Control headers are set to %s', 'performance-lab' ), | |
| $flagged_headers_string | |
| ); | |
| $result['label'] = __( 'Cache-Control header is preventing fast back/forward navigations', 'performance-lab' ); |
plugins/performance-lab/includes/site-health/cache-control-headers/helper.php
Outdated
Show resolved
Hide resolved
…overage directives
…-compatibility-headers'
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
@b1ink0 This looks great for the most part, just a few minor points of feedback.
| $result['description'] = '<p>' . wp_kses( | ||
| sprintf( | ||
| /* translators: %s is the error code */ | ||
| __( 'There was an error while checking your Cache-Control response header. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), |
There was a problem hiding this comment.
This message sounds strange. The last bit is like a sentence, but is not actually a sentence.
There was a problem hiding this comment.
| __( 'There was an error while checking your Cache-Control response header. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), | |
| __( 'The request to check the Cache-Control response header responded with error code <code>%s</code> and the following error message:', 'performance-lab' ), |
| __( 'There was an error while checking your Cache-Control response header. Error code: <code>%s</code> and the following error message:', 'performance-lab' ), | ||
| esc_html( (string) $response->get_error_code() ) | ||
| ), | ||
| array( 'code' => array() ) | ||
| ) . '</p><blockquote>' . esc_html( $response->get_error_message() ) . '</blockquote>'; |
There was a problem hiding this comment.
Why do we need the blockquote markup here? This feels odd for an error message. It would also be better for localization to use interpolation via sprintf and put a %s after the error message: bit of the i18n message.
There was a problem hiding this comment.
The prior art for this comes from the Site Health test for Optimization Detective:
performance/plugins/optimization-detective/site-health.php
Lines 117 to 124 in 762e52a
performance/plugins/optimization-detective/site-health.php
Lines 155 to 157 in 762e52a
There was a problem hiding this comment.
I'm not sure what you mean about being better for localization? The markup strings here aren't inside of translation functions.
There was a problem hiding this comment.
The prior art for this comes from the Site Health test for Optimization Detective:
I must have missed that there. I find it an odd choice for markup. The error message could simply be displayed together with the leading text.
I'm not sure what you mean about being better for localization? The markup strings here aren't inside of translation functions.
I mean using sprintf( __( 'This is a message: %s' ), $error_message ) being better than __( 'This is a message:' ) . ' ' . $error_message. It's not about the HTML markup, but about using sprintf.
There was a problem hiding this comment.
I'm not sure how that would work with the existing HTML markup. Could you amend this PR with what you have in mind?
There was a problem hiding this comment.
Well, I guess it does tie in with the markup in the sense that removing it would allow us to use sprintf() in the way I described above, which is better for localization. So I mentioned it as another reason to get rid of the unnecessary blockquote markup and instead simply display a single string that includes the actual error message.
There was a problem hiding this comment.
How is it better for localization? Core doesn't do this in many cases:
Compared with:
There was a problem hiding this comment.
For a specific example from core: https://github.com/WordPress/wordpress-develop/blob/20071e0fefcbd4653aefcacb8f67bd46cb5d28d6/src/wp-includes/class-wpdb.php#L1826-L1852
There was a problem hiding this comment.
I mean we don't need to get hung up on that. My main feedback was that I find the blockquote markup unnecessary. An error message can simply accompany the text that it is prefixed with, no need for another element.
FWIW I don't think there's any situation in Core where a blockquote or other element is used for such a message.
There was a problem hiding this comment.
plugins/performance-lab/includes/site-health/bfcache-compatibility-headers/hooks.php
Outdated
Show resolved
Hide resolved
… add/cache-control-site-health-test * 'trunk' of https://github.com/WordPress/performance: Internalize admin bar processing in tag processor Skip visiting the admin bar when optimizing a page
Co-authored-by: felixarntz <[email protected]>
westonruter
force-pushed
the
add/cache-control-site-health-test
branch
from
05a4dd5 to
96eac80
Compare
|
Here are the current possible states for the test: Pass
Error
Fail
|
westonruter
changed the title
Cache-Control headers to prevent bfcache from being disabledCache-Control page response header which disables bfcache
westonruter
changed the title
Cache-Control page response header which disables bfcacheCache-Control: no-store page response header which disables bfcache
Summary
Fixes #1692
Relevant technical choices
This PR introduces a new Site Health performance test that flags Cache-Control headers (no-store, no-cache, max-age=0) when detected in unauthenticated frontend responses.
