Open strarsis opened 8 months ago
Note: Should this X-Cache-Enabled HTTP header only be added to loopback requests? On the other hand, the Fastcgi-Cache HTTP header is also already added to all requests.
If possible (and relatively easy) I might prefer this just to avoid having "duplicate" response headers. In the long run it might be better to unify on the more generic X-Cache-Enabled
header but I'd rather not deal with that breaking change.
🤔 X-Cache-Enabled
is a bit of a weird name thinking about it more. It says cache is enabled, but not necessarily if its a cache hit? 🤷♂️ X-Cache
is the more common header for cache status
Another reason to try and limit this to loopback if possible.
@swalkinshaw: Ideally the X-Cache
HTTP header is also checked in WordPress, but until this is added, X-Cache-Enabled
appears to be the least intrusive HTTP header.
Yes, constraining the header to loopback requests only would be a good idea, it should be relatively easy using the map.
@swalkinshaw: Alternative proposal: Add the X-Cache
header name (with check callback) to the checked headers by using the site_status_page_cache_supported_cache_headers
filter, as in a Bedrock site mu-plugin.
Related WordPress trac issue (alternative/improved methods of detecting page cache): https://core.trac.wordpress.org/ticket/57349
We definitely won't add this in Bedrock by default. I'm not against supporting it in Trellis if we can limit to loopback. I think proxy_set_header
is what we want instead of add_header
? I'd prefer using X-Cache
as well if that works.
Side note: The already added Fastcgi-Cache
header is not added with proxy_set_header
because it can be useful to the outside, e.g. reverse proxy frontends?
proxy_set_header
After some experimentation I noticed that WordPress does not check for the upstream HTTP header, but rather does a loopback HTTP request and checks the HTTP headers from it. So the cache-specific header must be added to the reverse proxy frontend, it could still be added conditionally, for those WordPress loopback requests only, but this would not work as an added upstream HTTP header.
@swalkinshaw: It was not as trivial as I thought first, but now the configuration only adds the X-Cache-Enabled
header for loopback-requests. Checking for localhost origin (127.0.0.1
or ::1
) did not work, as WordPress loopback requests use the public IP address of the web server itself. Luckily comparing server_addr
with $remote_addr
offers a robust and elegant solution to this issue. At first I had to use volatile
in some of the maps, but now it works without it. Apparently the caching works out correctly in the current configuration. In practice this should result in zero performance difference, even at very high loads.
This PR adds the
X-Cache-Enabled
HTTP header tonginx
responses. It will not be added when upstream cache status isBYPASS
as then no caching is used for the request.WordPress Site Health Page Caching uses specific HTTP header values in the loopback HTTP response in order to detect that page caching is indeed enabled.
The related
Fastcgi-Cache
HTTP header is added by Trellis, but not used by the WordPress site health check.Adding this to Trellis by default appears to be a good idea (no obvious side-effects, improving compatibility with WordPress core (site health check)).
See roots discussion: https://discourse.roots.io/t/site-health-check-page-cache-not-detected/25640/2
Note: Should this
X-Cache-Enabled
HTTP header only be added to loopback requests? On the other hand, theFastcgi-Cache
HTTP header is also already added to all requests.Possible alternative: A Bedrock MU plugin that uses the
Fastcgi-Cache
HTTP header to setX-Cache-Enabled
in PHP code.