On 5/17/07, Roy T. Fielding <[EMAIL PROTECTED]> wrote:
On May 17, 2007, at 2:53 PM, Justin Erenkrantz wrote:
> BTW, I'm not a fan of us inventing Expires headers in this section of
> code - I'd think it'd be far cleaner to off-load Expires response
> header generation to mod_expires and leave the cache out of it
> entirely - inventing Expires values deep inside of mod_cache seems
> unclean. mod_cache, IMO, should just respect what it is told and not
> change how things appear to downstream folks. (mod_expires is more
> than capable of inserting in the Expires header if the admin wants
> it.)
I agree -- caches are not allowed to invent header fields like Expires.
They can only do so by explicit override in the configuration
(mod_expires).
Setting Expires here is wrong. Changing max-age would be even worse.
Age is the only thing the cache should be setting.
> Does my position make sense? I'm of the opinion that we should go one
> of two ways:
>
> - mod_cache shouldn't touch the response - so it stops setting Expires
> or anything like that which affects cachability
+1
> - mod_cache always tweaks the response - so my CC: max-age fix needs
> to mimic what we do for Expires.
-1
With this patch, the only thing mod_cache ever touches is...Age. =)
I'll let this sit here for a bit and if we agree this is right, we can
commit. -- justin
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c (revision 539156)
+++ modules/cache/mod_cache.c (working copy)
@@ -663,21 +663,8 @@
now = apr_time_now();
if (info->date == APR_DATE_BAD) { /* No, or bad date */
- char *dates;
/* no date header (or bad header)! */
- /* add one; N.B. use the time _now_ rather than when we were checking
- * the cache
- */
- if (date_in_errhdr == 1) {
- apr_table_unset(r->err_headers_out, "Date");
- }
- date = now;
- dates = apr_pcalloc(r->pool, MAX_STRING_LEN);
- apr_rfc822_date(dates, now);
- apr_table_set(r->headers_out, "Date", dates);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "cache: Added date header");
- info->date = date;
+ info->date = now;
}
else {
date = info->date;
@@ -709,7 +696,6 @@
* expire date = date + defaultexpire
*/
if (exp == APR_DATE_BAD) {
- char expire_hdr[APR_RFC822_DATE_LEN];
char *max_age_val;
if (ap_cache_liststr(r->pool, cc_out, "max-age", &max_age_val) &&
@@ -746,13 +732,9 @@
x = conf->maxex;
}
exp = date + x;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
else {
exp = date + conf->defex;
- apr_rfc822_date(expire_hdr, exp);
- apr_table_set(r->headers_out, "Expires", expire_hdr);
}
}
info->expire = exp;
