Problem/Motivation
Recently, our team noticed an issue in which our 404 error pages had an s-max-age of 30 days, despite them being defined as 5 minutes. After doing some sleuthing, we determined that this line was causing the issue: https://git.drupalcode.org/project/http_cache_control/-/blob/8.x-2.x/src...
Because we had the regular max-age (the browser max-age) and the 404 max-age both set to be 5 minutes, this conditional did not pass. Thus, our 404 pages were receiving our 'global' s-max-age, rather than the max-age defined in the settings.
Changing the max-age from 5 minutes to 3 minutes resolved this situation. However, we only came to this solution after reviewing the module's code -- this limitation is not explained anywhere else.
Proposed resolution
Either document on the module page or in the System Performance settings form that these values cannot be the same.
| Comment | File | Size | Author |
|---|
Issue fork http_cache_control-3276742
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
josh waihi commentedThis is a bug rather than a feature to document. Would you be able to write a fix or tests to confirm the failure?
Comment #3
kevineinarsson commentedAttached a test + patch (not a clean patch but it... works(?)) Maybe need to check if these values are null too.
Comment #4
kevineinarsson commentedThe patch doesn't work, s-maxage and max-age are always set to the same value.
The configuration form says that the 3xx/404/5xx max-age value should be the same as 200 responses. If that's the case, why is setClientTtl called?
Comment #7
dieterholvoet commentedI started a MR with the provided test, will look for a fix as well.
Comment #8
dieterholvoet commentedA couple things I don't understand either about the current code:
setClientTtl()used for max-ages andsetSharedMaxAge()for s-maxages? The former considers theAgeheader, the latter doesn't.I'm afraid that fixing the first one would require a new major release. It's undocumented behaviour, but still behavior that people might depend on. I started a MR fixing 1, 2 and 3, which in turn also fixes this issue. I did keep the last one for now, but I decided to document it in the form.
Existing and new tests are passing, but it would be great if @josh waihi could review, just to make sure we're not missing anything here.
Comment #10
dieterholvoet commentedStill no answer, so I'm going to go ahead and merge this. I decided not to create a new major release to fix 404/302/301 TTL's being assigned to the maxage. Custom TTL's for 404/302/301 responses aren't something everyone uses + I highly doubt people really depend on the max-age header, which is only supposed to be read by the browser. CDNs use the s-maxage header, so we should be good here. I'll document it in the release notes anyway.