cache control headers are not correctly added to redirects [#1392974]

Some reverse-proxy caches, CDNs, and browsers can cache redirects (e.g 301 or 302) which avoids a drupal bootstrap on the backend. However, in order for this caching to work, we need an expires or max-age header. I suggest that adding such a header be added as an option for each redirect.

In addition, according to the HTTP/1.1 specification, 301 and 302 responses can be cached by the browser. This means that even if the resource itself is not cacheable, the browser can at least look up the correct URL in its local cache. 301 responses are cacheable by default unless otherwise specified. To make a 302 response cacheable, you need to configure your web server to add an Expires or Cache-Control max-age header (see Leverage browser caching for details). The caveat here is that many browsers don't actually honor the spec, and won't cache either 301 or 302 responses; see Browserscope for a list of conforming and non-conforming browsers.

Comment	File	Size	Author
#4	1392974-4.patch	450 bytes	pwolanin
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 3 January 2012 at 19:39

We already support storing redirects in the page cache which adds its default headers into the page cache bin. I'm not sure how much we should be adding extra code to support external cache users? Maybe I just don't have a good idea of how much we need to add to support this.

Comment #2

pwolanin CreditAttribution: pwolanin commented 3 January 2012 at 22:09

Title:	Support the addition of cache control headers to redirects	» cache control headers are not correctly added to redirects
Category:	feature	» bug

Dave Reid tells me this should work in combination with core page cache if the setting/variable redirect_page_cache is true in combination with other settings.

Examining the code compared to core, this is not quite true.

In theory this might currently work to send the right headers the *second* time the redirect is hit if it is cached, but it will certainly not work the 1st time. Thus, if I'm using an external page cache like Varnish, the max-age header is never sent to Varnish.

I think what's missing is a call to drupal_serve_page_from_cache($cache);

Comment #3

pwolanin CreditAttribution: pwolanin commented 4 January 2012 at 00:41

Here's the suggested change per discussion in IRC

--- a/redirect.module
+++ b/redirect.module
@@ -982,7 +982,9 @@ function redirect_goto($redirect) {
     if (variable_get('cache', 0)) {
       // We must output something to allow the request to be cached.
       echo ' ';
-      drupal_page_set_cache();
+      if ($cache = drupal_page_set_cache()) {
+        drupal_serve_page_from_cache($cache);
+      }
     }
   }

Comment #4

pwolanin CreditAttribution: pwolanin commented 4 January 2012 at 01:05

Status:

Active

» Needs review

File	Size
1392974-4.patch	450 bytes

Comment #5

ndeschildre CreditAttribution: ndeschildre commented 20 March 2012 at 16:30

Even though I haven't directly tested this patch on this module, putting these lines directly on drupal_exit (which your redirect_goto function is based on):

      if ($cache = drupal_page_set_cache()) {
        drupal_serve_page_from_cache($cache);
      }

will give the intended behavior (Varnish now caching the redirect).

Comment #6

ndeschildre CreditAttribution: ndeschildre commented 22 March 2012 at 15:57

It seems there is a kind of conflict with the naming of functions and how they interact with external proxies:
For the SQL cache_page cache, obviously:

drupal_page_set_cache() => save the cache
drupal_serve_page_from_cache => serve the cache

But in a varnish configuration:

drupal_page_set_cache() => do nothing
drupal_serve_page_from_cache => save the cache (via the Cache-Control= public, max-age=XX header), but do not "serve cache"

So no problem for HTTP 200 pages for both internal and external caches, since the following code is called:

    if ($cache = drupal_page_set_cache()) {
      drupal_serve_page_from_cache($cache);
    }

But for the 301/302 redirects with drupal_goto(), it may be cached in the internal cache (as Dave Reid seems to imply, haven't found the drupal_page_set_cache() but haven't deeply searched), but drupal_serve_page_from_cache() is definitely not called, and as a consequence it is not cached in the external cache.

I guess one solution for the core would be to add the Cache-Control= public, max-age=XX header in drupal_page_set_cache() to cover both case...

Meanwhile, for the external proxy users, here is a dirty way to cache the redirects (not to be used by SQL cache users).

/**
 * Implementation of drupal_goto_alter()
 */
function XXXX_drupal_goto_alter(&$path, $options, $http_response_code)
{
  //If drupal_goto is asked to do a redirect (essentially via global_redirect),
  //then cache this as well (by default, it isn't, drupal_goto calls drupal_exit
  //which do not cache the result)
  if($http_response_code == 301 || $http_response_code == 302)
  {
    if ($cache = drupal_page_set_cache()) {
      drupal_serve_page_from_cache($cache);
    }
  }
}