Posted by grendzy on August 31, 2010 at 6:11pm
| Project: | Cache Router |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
If page_cache_fastpath is enabled, the value cached in $page->headers is ignored. This means, e.g. that 404 pages are served as "200 OK", and Content-Type and Vary headers can be incorrect.
The solution is to call drupal_page_cache_header
| Attachment | Size |
|---|---|
| cacherouter_headers.patch | 1.69 KB |
Comments
#1
I confirm the bug. I tested a 404 page (set to a custom node, which shouldn't matter), with cacherouter running on File engine and fast page caching on. I viewed response headers in Firefox "Web Developer Toolbar" add-on (note that this tool probably never shows the initial [uncached] page load, seems to re-fetch the page for headers display, so I flushed the page cache (by manually calling cron.php in another tab) between the initial page load and opening of the headers display - then everything works as expected). The initial (uncached) page load shows response 404 Not Found, reloads show 200 OK (which is a bug, indeed).
The patch didn't apply. The reason is, that it was actually rolled against the outdated 6.x-1.0-rc1 version (I guess). I reproduced the changes manually and proceeded to further testing.
The patch didn't fix the problem - I still got a 200 OK response on cached 404 pages. The reason was the headers-unserializing code in the patch, which actually removed the cached headers - they are not serialized in the cache object, so there's no need for that. I checked most (not all) engines, and the headers seems to be nowhere serialized - where did the idea of unserializing headers come from?
After removing the unserialize() call, the problem is most probably solved. I'm no expert on headers, but now both cached/uncached pages show correct response (i.e. 404), headers seems to be fine in both (i.e. nothing visibly missing, although there are some natural differences and I don't really know what should be there), and the new X-Drupal-Cache header shows on cached pages as well.
Attaching the modified patch, rolled against current 6.x-1.x-dev release.
#2
Ah, interesting. In pressflow $cache->headers is serialized; with a stock core it is not. By calling page_get_cache() we can sidestep this problem entirely. I've benchmarked this and confirmed there's no measurable performance hit by going this route.
How's this patch? (using the latest -dev this time)
#3
No time to test today, but on first glance it seems fine to me. I'm a bit confused about the fact that there seems to be some Drupal config altering this bit of caching (i.e. the serialized headers issue), but if this helps, it's likely the way to go.
The page_get_cache() function seems to be doing the same thing already, plus have further conditions (PHP CLI) and might get more in future, so it's desirable to re-use that. It seems to already include the test for POST requests - although we check for empty($_POST) and core for request method GET, I think that's the same actually, so we might be able to remove that bit from earlier if() conditional. The drupal_set_message() check won't work without sessions, but that's no worse than what we have already, and the drupal_set_message() call itself returns nothing in this case, so no harm done.
I hope to be able to test the new patch in a few days...