drupal_page_cache_header() has some nice functionality to abort page loads with a '304 Not Modified' response if the page is in the cache.

Unfortunately, private file downloads can't have the same - a module implementing hook_file_download() would need to copy and adapt part of drupal_page_cache_header() and exit inside the hook, which is awful. Neither can modules like imagecache which feature their own download paths make use of Last-Modified headers without duplicating code from drupal_page_cache_header() in the same way.

Therefore, this patch takes an approach by fago as inspiration and splits out the shareable parts from drupal_page_cache_header() into a new function drupal_apply_cache_headers() which can be used by file.inc, imagecache, or whatever download module wants to use it. The code semantics for drupal_page_cache_header() stay the same except that the Last-Modified header is added also when doing the 304 - that's necessary for modules that don't want to use the page creation time as Last-Modified (makes little sense for files, arguably). For drupal_page_cache_header() generated headers, adding Last-Modified should not make any difference in behaviour.

As example application of that new function (which should be quite beneficial to private download performance as well), I used it in file_download() after we know that a file will be downloaded. I also replaced file_exists() with is_file() as the former might also retrieve directories which we rather don't want to pass to file_transfer().

CommentFileSizeAuthor
#7 drupal-factor-out-cache-headers-try3.patch5.14 KBjpetso
#4 drupal-factor-out-cache-headers-try2.patch5.03 KBjpetso
drupal-factor-out-cache-headers.patch5.02 KBjpetso

Comments

jpetso’s picture

jpetso commented
Title: Also use cache headers for private downloads » Use cache headers also for private downloads

Nicer title. Well, nitpicking... you know.

drewish’s picture

drewish commented

subscribing.

jpetso’s picture

jpetso commented

@drewish: This patch has been sitting idle in the queue for nearly two months... any chance you could spare a bit of time to test & review it?

jpetso’s picture

jpetso commented
StatusFileSize
new drupal-factor-out-cache-headers-try2.patch5.03 KB

Rerolled so that it applies to current HEAD again, using the newly added space for the concatenation operator. Unchanged otherwise.

drewish’s picture

drewish commented
Status: Needs review » Needs work 
amorton@paycheck:~/Sites/dh% patch -p0 -i drupal-factor-out-cache-headers-try2.patch 
patching file includes/bootstrap.inc
Hunk #1 FAILED at 578.
Hunk #2 succeeded at 647 (offset 51 lines).
Hunk #3 succeeded at 663 (offset 51 lines).
1 out of 3 hunks FAILED -- saving rejects to file includes/bootstrap.inc.rej
patching file includes/file.inc
Hunk #1 succeeded at 868 (offset 35 lines).
c960657’s picture

c960657 commented 
+      // Set Last-Modified and ETag, or exit with a '304 Not Modified' response.
+      // For Last-Modified, use the mtime of the file as retrieved by stat().
+      if ($fileinfo = stat($full_filepath)) {
+        drupal_apply_cache_headers($headers, $fileinfo[9]);
+      }

Under what cirumstances will stat() return false? A few lines above you just verified that $full_filepath is a file, and this wont change even if the file is deleted in the meantime due to the stat cache, unless some module calls clearstatcache(). A comment explaining the additional check would be helpful.

Also, why not just use filemtime()? That would make the code easier to read.

+function drupal_apply_cache_headers(&$headers, $last_modified_timestamp) {
It seems like a quite unusual approach to modify array specified as a function argument rather than returning the necessary headers or just setting them directly using header(). Any particular reason to use this pattern?

It appears as if drupal_set_header() is the "official" way of setting headers rather than calling header() directly.

jpetso’s picture

jpetso commented
Status: Needs work » Needs review
StatusFileSize
new drupal-factor-out-cache-headers-try3.patch5.14 KB

@c960657: Let's start from the bottom of your comment.

drupal_set_header() is indeed the "official" way of setting headers. So header() should only be used if drupal_set_header() is not yet available, which is during the bootstrap. drupal_page_cache_header() used header() even before my patch, which is because it's invoked during the "late page cache" bootstrapping phase. drupal_set_header() on the other hand is in common.inc, which is only loaded during the last, "full" bootstrapping phase. So we can't use drupal_set_header() there.

Once Drupal is bootstrapped, we do want to use it though, so it would be a bad idea to call either header() or drupal_set_header() directly within drupal_apply_cache_headers() - except for the case when the function exits right away, and in that case the drupal_set_header() functionality is not needed. So in case the function does not exit, we want to enable the caller to use the appropriate header setter function, which is probably drupal_set_header() for everyone but the page cache itself.

As for passing the $headers array by reference, changing that to a simple array_merge() would not make any difference in practice. It's just that if the function possibly exit()s, I'd find it strange to have that happen in an assignment statement, i.e.

$headers = array_merge($headers, drupal_cache_headers($last_modified_timestamp));

That's probably a taste issue, I think... drewish, any comments on that matter?

filemtime() is a good suggestion, I was not aware that there is a specialized function for that. And finally,

Under what cirumstances will stat() return false?

I have no idea, and frankly, I don't care. The API documentation for stat() and filemtime() says,

stat(): In case of error, stat() returns FALSE.
filemtime(): Returns the time the file was last modified, or FALSE in case of an error

It does not specify under which circumstances it might return FALSE, which is reason enough not to assume that it's always going to work. Hence the check. A bit of defensive coding doesn't hurt here, or does it?

Find attached a rerolled version using filemtime(). Patch rolled right after the file.inc cleanup patch (#308434: Clean up file.inc ahead of hook_file and add unit tests.) was applied.

c960657’s picture

c960657 commented
Status: Needs work » Needs review

A bit of defensive coding doesn't hurt here, or does it?

No, definitely not. My thought was just that if FALSE indicates that the file has been deleted by another thread, we might as well skip the file_transfer() call as well and instead return a 404 status. In other words, try to handle the real problem rather than just the symptom. But you are right that the PHP manual isn't very specific about what FALSE actually means, so your suggested behaviour is probably the best we can do.

Your other comments to my comments sounds convincing to me :-) I agree that exit()'ing in an assignment would probably be too weird.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

The last submitted patch failed testing.

Status: Needs review » Needs work

Correcting issue status, see #2125263: Issue status not taken from last comment during D7 migration.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

I suggest closing this issue in favour of

#1715222: Remove almost all file_exists() and is_file() calls from core

and

#752730: Remove file_exists() during bootstrap

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.