This is quite trivial and helps with performance -- various accelerator methods can have a better grip on what Drupal generates. page_set_header will need patching too but this is good as it is.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Needs review » Needs work

It looks like drupal_page_cache_header() already does that for the Etag part. The Content-Length might help, thou.

yonailo’s picture

Title: Provide ETag and Content-length headers for cached pages » Why does it help "performance"

Could you please elaborate how the Content-Legth field can help with performance ? if you can give some URLs it would be nice !. Thanks.

yonailo’s picture

Title: Why does it help "performance" » Provide ETag and Content-length headers for cached pages

Sorry I mistakenly changed the title of the thread.

yonailo’s picture

I have found problems applying this patch on my site.

Specifically, I have found incomplete load of our home page when it is served from the cache on Internet Explorer 6, we use a lot of jquery and ajax calls on the web page, and the problem dissapears if I remove the Content-Length header and truncate cache_page.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Revised patch that adds the content-length to drupal_page_cache_header, instead of adding the headers really late in page_set_cache.

Dave Reid’s picture

Title: Provide ETag and Content-length headers for cached pages » Provide Content-length header for cached pages

Revision to title because this patch no longer adds the etag header, since the drupal_page_cache_header function already does that. The patch in #5 also moves some header() calls around so that they can be done together.

yonailo’s picture

I am watching the patch and it seems not right to me. After adding the header Content-Lenght, there is a condition that uncompress cache->data if the browser does not support compression, that would make the value previously set invalid, wouldn't it?

Dave Reid’s picture

No, it sends that header only after $cache->data has been prepared.

yonailo’s picture

Ok, I didn't apply the patch and I was looking at my source code, which is D5:

function drupal_page_cache_header($cache) {
  // Set default values:
  $last_modified = gmdate('D, d M Y H:i:s', $cache->created) .' GMT';
  $etag = '"'.md5($last_modified).'"';

  // See if the client has provided the required HTTP headers:
  // *** Added preg_replace; some browsers append a "; length=..." entity to the header
  $if_modified_since = isset($_SERVER['HTTP_IF_MODIFIED_SINCE']) ? preg_replace('/;.*$/','',stripslashes($_SERVER['HTTP_IF_MODIFIED_SINCE'])) : FALSE;
  $if_none_match = isset($_SERVER['HTTP_IF_NONE_MATCH']) ? stripslashes($_SERVER['HTTP_IF_NONE_MATCH']) : FALSE;

  // *** Added:
  // ***IE6 does not support Entity Tags, so depend on the if_modified_since to guide us.
  if (preg_match('/MSIE\s6\.\d+/i',$_SERVER['HTTP_USER_AGENT']) && $if_modified_since!==false)
        $if_none_match = '"'.md5($if_modified_since).'"';

  if ($if_modified_since && $if_none_match
      && $if_none_match == $etag // etag must match
      && $if_modified_since == $last_modified) {  // if-modified-since must match
    header('HTTP/1.1 304 Not Modified');
    // All 304 responses must send an etag if the 200 response for the same object contained an etag
    header("Etag: $etag");
    exit();
  }

  // Send appropriate response:
  header("Last-Modified: $last_modified");
  header("ETag: $etag");

  // The following headers force validation of cache:
  header("Expires: Sun, 19 Nov 1978 05:00:00 GMT");
  header("Cache-Control: must-revalidate");

  // Determine if the browser accepts gzipped data.
  if (@strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') === FALSE && function_exists('gzencode')) {
    // Strip the gzip header and run uncompress.
    $cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));
  }
  elseif (function_exists('gzencode')) {
    header('Content-Encoding: gzip');
  }

Obvisouly to apply this patch on this code, the header('Content-Length') should be set AFTER the line:

    $cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));

because this line modifies the length.
That was what I was saying. Sorry for the confusion.

lilou’s picture

FileSize
834 bytes

Patch attached according to #9. Add also a comment (from http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html ) :

// Send size of the entity-body, in decimal number of octets:
Dave Reid’s picture

Ok seriously, the patch in #5, sends that header after all the $cache->data manipulation has been done. You need to try the patch. According to the description, "The patch in #5 also moves some header() calls around so that they can be done together." (for clarity).

  if (variable_get('page_compression', TRUE)) {
    // Determine if the browser accepts gzipped data.
    if (@strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') === FALSE && function_exists('gzencode')) {
      // Strip the gzip header and run uncompress.
      $cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));
    }
    elseif (function_exists('gzencode')) {
      header('Content-Encoding: gzip');
    }
  }

+  // Send appropriate response:
+  header("Last-Modified: $last_modified");
+  header("ETag: $etag");
+  header('Content-Length: ' . strlen($cache->data));
+
+  // The following headers force validation of cache:
+  header("Expires: Sun, 19 Nov 1978 05:00:00 GMT");
+  header("Cache-Control: must-revalidate");
+

I'm all for the additional comment in #10, but for readability sake, let's please base off the patch in #5.

lilou’s picture

FileSize
1.44 KB

So ...

lilou’s picture

FileSize
1.44 KB
yonailo’s picture

Dave Reid is absolutely right, his patch applies correctly.

Is it not possible to delete my previous post (#10) ?

I see that I can edit it, but I am a bit ashamed so I would like to remove it...
(you can consider me as a kind of drupal.org newbie :)

c960657’s picture

Status: Needs work » Needs review

Note that setting Content-Length explicitly causes problems with PHPs zlib.output_compression feature, so I suggest not adding the header if this feature is enabled:
http://www.php.net/manual/en/ref.zlib.php#43181

Could you give some examples of situations where this would improve performance?

Status: Needs review » Needs work

The last submitted patch failed testing.

Dave Reid’s picture

Status: Needs work » Needs review

Failed due to #74645: modify file_scan_directory to include a regex for the nomask.. Setting back to code needs review.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs review » Needs work
Issue tags: +Performance

Add tag.

nicholasThompson’s picture

What's the status of this? Is it safe to add the Content-Length header even for gzipped pages?

chx’s picture

Assigned: chx » Unassigned
Issue summary: View changes