-
Notifications
You must be signed in to change notification settings - Fork 6
Improve handing of setting the Cache-Control header in Pantheon Page Cache
#94
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
base: main
Are you sure you want to change the base?
Conversation
9b75378 to
52b7f95
Compare
|
Check failures do not appear to be valid. |
|
Pretty sure the failing tests are because the forked PR lacks the right permissions in GitHub Actions. |
jazzsequence
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.
Generally 👍 on this PR.
I'd like to see some unit tests added for the changes (but notably, none of our current tests are breaking which is a good sign).
My only hesitation is how much breaking PHP 5.x support matters. With the way WordPress and mu-plugin updates on Pantheon work, the workaround for a 5.x WP site would be to use an old version of WordPress, although notably, WordPress dropped support of < 7.0 in 6.5, so maybe this is a non-issue.
I'm good to merge beyond that.
|
Thanks for the PR Weston. This is awesome. It's ok to target PHP 7.0+ only. Let's update the readme to notate the requirement, and we'll include this in the next release notes for the mu-plugin. |
This is a follow-up to #37 and its PR #40.
I'm working on WordPress core ticket #63636 to:
In order to take advantage of bfcache, the
no-storedirective needs to be omitted from theCache-Controlheader. In order to prevent proxies from caching authenticated responses, theprivatedirective should be used instead, which WordPress has done since 6.3 via r55968. In this ticket I'm working on, I'm proposing thatno-storeshould be removed while leavingprivateso that bfcache won't be blocked in the browser. I've also developed a No-cache BFCache feature plugin as a way to test compatibility of this change with the WordPress ecosystem. This has resulted in changes to WooCommerce (woocommerce/woocommerce#58445) and Jetpack (Automattic/jetpack#44322), and now for Pantheon Page Cache.The immediate workaround for this is to use plugin code like this:
But having to override this is not ideal, naturally.
Even so, I was looking at
\Pantheon_Cache::filter_rest_post_dispatch_send_cache_control()and I found it was not also applying this filter, so this PR introduces a helper method to apply the filter so that REST API responses will respectpantheon_skip_cache_control.In order to better facilitate filtering and compatibility with other plugins, this PR replaces the
send_headeraction with filteringwp_headers. Nevertheless, unlessrest_send_nocache_headersin core is filtered to be false, whateverCache-Controlheader set by Pantheon ends up getting overridden byWP_REST_Server::serve_request()anyway, where it sends the headers returned bywp_get_nocache_headers(). So this PR does the underlying removal ofmax-age=0by filteringnocache_headerswhere this directive is initially introduced. Similarly, instead of hard-coding theCache-Controldirectives to use, this PR uses the value ofwp_get_nocache_headers()['Cache-Control']from whichmax-age=0is stripped out.