Noticed the pre-check and post-check directives for the Cache-control header today. Found this info by Eric Lawrence formerly of Microsoft that says:
If both post-check and pre-check are specified and set to 0, both are entirely ignored.
Not even sure if they are supported in latest IE/Edge.
Either way the way we have them set doesn't do anything for IE like it was intended and wastes bandwidth and header processing cycles.
Doesn't affect the mergeability of this patch but here is a question to find out if they are supported, if for some reason we actually wanted to use them correctly.
http://stackoverflow.com/questions/34663916/are-cache-control-pre-check-...
https://twitter.com/ElijahLynn/status/685195442498473985
Remove the directives from our Cache-control headers.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2646280-pre-check-post-check-headers-d7.patch | 2.74 KB | pietmarcus |
| #7 | interdiff-2-7.txt | 865 bytes | elijah lynn |
| #7 | 2646280-pre-check-post-check-headers-7.patch | 3.69 KB | elijah lynn |
| #2 | 2646280-pre-check-post-check-headers-2.patch | 3.68 KB | elijah lynn |
Comments
Comment #2
elijah lynnComment #3
elijah lynnComment #4
elijah lynnEric just replied on Twitter supporting the removal of them:
https://twitter.com/ericlaw/status/685201170260819968
https://twitter.com/ericlaw/status/685201463987929088
Comment #5
elijah lynnComment #7
elijah lynnYay for tests!
I was a little to aggressive and removed a 'no-cache' directive. It is back now.
Comment #8
cweagansNice!
Comment #9
bleen commentedRTBC++
Comment #10
wim leersOh dear deity, yes, please! :)
In fact, I think this could be applied to 8.0.x too. I'll leave that up to the committer to decide though.
Comment #12
catchTheoretically the header change could break contrib or custom tests looking for the default headers. For that reason, I've committed/pushed to 8.1.x only.
Moving directly to 7.x for backport. If someone feels strongly this should go into 8.0.x then feel free to move it back there for discussion.
Comment #13
wim leersAFAICT this means it cannot be backported to D7 for the same reason it cannot be backported to D8. Even more so because Drupal 7 sites will often be used in strictly controlled environments, which may include the use of IE.
Comment #14
David_Rothstein commentedWell if it's true they serve no purpose for any version of IE (and it sounds like they don't) it's safe to remove from Drupal 7 too.
On the other hand it doesn't sound like they are causing any harm either...?
I'm not too worried about broken contrib tests because it seems like it would be pretty rare (and if you're trying to assert the page was cached there are better, less fragile ways to do it). We would mention this in the release notes too so it would be easy to find if a test was broken.
Comment #15
pietmarcus commentedI've ported the functionality of the patch to D7. There was only one instance in code and two in testscripts.
Comment #16
pietmarcus commentedI've backported the functionality in the patch to d7.
Comment #17
pietmarcus commentedI have backported the patch for drupal 7. That is to say, I've removed all instances of the post-check header that I could find, and updated the tests. Somehow the patch is uploaded but not linked to my original post (which hasn't been posted btw), so I hope that's not a problem.
Comment #18
David_Rothstein commentedI went ahead and published the last few comments (including your patch) - I am not sure why drupal.org forced them to be unpublished. Thanks for posting the patch!
Comment #19
elijah lynnComment #20
elijah lynnJust tested this and applies nicely and removes the headers as expected. RTBCing to move the discussion along.
Comment #21
David_Rothstein commentedThe patch looks good to me too.
I'm going to hold off one more release on this, just in case it does turn out to be disruptive to contrib tests - this would be a good fit for #2598382: [policy] Adopt a 6-month feature release schedule for Drupal 7 (similar to Drupal 8) and use pseudo-semantic versioning if we wind up doing that (in that case it would be included in a release on April 20).
So a little bit of a wait, but this should be able to get committed soon either way.
Comment #22
David_Rothstein commentedCommitted to 7.x - thanks!
Note that the release that includes this will probably be labeled Drupal 7.50 to indicate it contains some big changes