Whilst porting some of our HTTP header handling to the kernel patch, I ran into this little gem:
http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal_page_header/7
Have a look at the huge paragraph there. Now read it again to try and understand what it's doing. It may take several tries. :-)
Now, have a look at the Firefox bug that it links to: https://bugzilla.mozilla.org/show_bug.cgi?id=269303
That was marked as a duplicate of another issue in 2009: https://bugzilla.mozilla.org/show_bug.cgi?id=510359
That issue was also resolved in 2009. So it will have been fixed for ~4 years by the time Drupal ships.
OK, but does that mean we don't need to support that browser version?
According to Firefox's release history, the bug would have been fixed before Firefox 3.6 shipped, and was likely also backported to Firefox 3.5. That means the only extant versions of Firefox with this bug should be 3.0 and earlier. Firefox 3.0's market share is, I think, in negative numbers at this point as Mozilla just announced that they have enabled auto-update for Firefox 3.6, so all Firefox 3.6 browsers will overnight turn into Firefox 13 in the not too distant future.
That is, browsers affected by that bug no longer exist in the wild.
We should therefore drop that hack entirely, and free up the use of ETags for something useful, like better cache control.
However, let us NOT do that until after the #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel lands, because I don't want yet another merge conflict. :-) It should be easy to strip out afterward.
Comments
Comment #1
chx CreditAttribution: chx commentedDo we have a "huge can of worms" tag :D ? REQUEST_TIME sucks as an ETag but then again, the "what's a good ETag" is an insanely complicated question that is not easily answered. Hashing the content is not bad... unless the hash itself is a performance hit...
Comment #2
Crell CreditAttribution: Crell commentedTo be honest, I don't fully comprehend how ETags work in the first place. That's one reason I'd rather not use them if we don't need to. Symfony does some futzing with the cache headers on its own to make them sane, so unless we know exactly what we're doing I'm inclined to get out of its way and let it figure things out.
Right now Drupal goes to great lengths to defeat HTTP caching. That's something we need to stop doing. This is just a part of that.
Comment #3
sunhttp://drupalcode.org/project/drupal.git/commit/526401c4c was added in
#147310: Implement better cache headers for reverse proxies
As usual, someone should double-check whether the phpDoc is actually complete and correct, and covers all problems the change intended to fix.
Comment #4
mgiffordI came onto this when looking at a Varnish issue (one page had a different ETag with every load and wasn't caching, while others kept with the same etag no matter how many times we loaded the file).
If @chx & @crell don't understand this, I'm certain we need better docs. More than just:
https://en.wikipedia.org/wiki/HTTP_ETag
I'm assuming the docs here haven't been updated - http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal_...
@sun these are the phpDocs right?
Adding links to other references to ETag best practices:
http://realityloop.com/blog/2009/08/19/optimizing-page-load-times-using-...
http://wimleers.com/article/improving-drupals-page-loading-performance
Comment #4.0
mgiffordfix link
Comment #5
znerol CreditAttribution: znerol commentedMaking this a sub-issue of #2257695: [meta] Modernize the page cache
Comment #6
vijaycs85as per https://www.drupal.org/node/2181523 we don't have drupal_page_header in D8. Is this still valid issue?
Comment #7
vijaycs85Only place we have this in D8 is install_display_output. So this issue is 'Closed (Won't fix)' to me.
Comment #8
mgiffordI looked at the output too and there is no ETag in D8.