WordPress.org

Make WordPress Core

#23021 closed defect (bug) (fixed)

Last-Modified header always set to 1970... when it is blank. Make browser cannot refresh cache.

Reported by: slene Owned by: ryan
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: General Keywords: has-patch commit fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In #22258, the Last-Modified header set blank when user login.

A response with a blank Last-Modified header will be set to "Thu, 01 Jan 1970 00:00:00 GMT" in some server. The browser will be always send a If-Modified-Since header "Thu, 01 Jan 1970 00:00:00 GMT". and the server response a 304 status code. Make the browser can not refresh cache.

In php5.2 the Last-Modified header always blank. But php5.3 has header_remove remove it.
The Last-Modified header should be remove form $headers array when it blank.

sorry my bad english...

Attachments (10)

23021.diff (1.4 KB) - added by slene 19 months ago.
unset empty Last-Modified header before @header instead of use php5.3 function header_remove
23021.2.diff (1.6 KB) - added by SergeyBiryukov 19 months ago.
23021-3.patch (2.2 KB) - added by azaozz 19 months ago.
23021.3.diff (1.1 KB) - added by nacin 19 months ago.
23021.4.diff (3.4 KB) - added by nacin 19 months ago.
23021.nospace.diff (1.1 KB) - added by nacin 19 months ago.
23021.5.diff (1.8 KB) - added by slene 19 months ago.
should be unset Last-Modified header when it empty.
wp_header.diff (1.5 KB) - added by andy 19 months ago.
New utility: wp_header($name, $value)
23021.6.diff (2.6 KB) - added by nacin 18 months ago.
23021.6.refreshed.diff (2.7 KB) - added by nacin 18 months ago.

Download all attachments as: .zip

Change History (32)

comment:1 SergeyBiryukov19 months ago

  • Description modified (diff)
  • Keywords reporter-feedback removed

slene19 months ago

unset empty Last-Modified header before @header instead of use php5.3 function header_remove

comment:2 ocean9019 months ago

  • Component changed from General to Cache
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.6
  • Severity changed from critical to normal

Patch looks sane.

comment:3 slene19 months ago

Will this ticket can change Milestone to 3.5.1 ?

SergeyBiryukov19 months ago

comment:4 SergeyBiryukov19 months ago

  • Component changed from Cache to General

I guess Cache component is for the built-in object cache, moving back to General.

Related: [22283]

Would it also make sense to remove the default blank value from wp_get_nocache_headers() (23021.2.diff), since it will be unset anyway?

comment:5 nacin19 months ago

  • Milestone changed from 3.6 to 3.5.1

comment:6 azaozz19 months ago

Yeah, what's the point in setting an empty Last-Modified header and then unsetting it. Also bear in mind that the web server sits between PHP and the client, so it can add/remove/change any header no matter what we do in PHP depending on its settings.

Thinking we should remove Last-Modified from wp_get_nocache_headers() and leave it at that. If a plugin uses the filter to set it back, it may have a good reason to do this, we shouldn't limit/overwrite it.

azaozz19 months ago

comment:7 azaozz19 months ago

In 23021-3.patch:

  • Remove Last-Modified header from wp_get_nocache_headers(). That gives a chance for a plugin to add it back if needed.
  • Remove the test if ( empty( $headers['Last-Modified'] ) ... in WP::send_headers() in class-wp.php. That header would not be set if the user is logged in, on query error or feed. If not, it's always set.

nacin19 months ago

nacin19 months ago

nacin19 months ago

comment:8 follow-up: nacin19 months ago

Looking at the PHP source, this doesn't look like it'll work, but — slene, can you test 23021.nospace.diff? This is a hunch from andy and me that header('Last-Modified:') may work. We're currently issuing header('Last-Modified: '), and the space might throw things off.

comment:9 in reply to: ↑ 8 slene19 months ago

The patch 23021.nospace.diff cannot solve this problem.

Some cgi component uses apache utils script ap_scan_script_header_err_brigade to valid cgi headers. Set to "1970..." date when Last-Modified header was empty or has wrong date format.

I think the Last-Modified header should not be empty. rfc2616

slene19 months ago

should be unset Last-Modified header when it empty.

comment:10 nacin19 months ago

23021.4.diff is the direction I think I want to go, after a long conversation with andy (who contributed #22258 and knows far more than I about insane caching setups).

It is basically 23021.5.diff but it does some extra effort to verify that absolutely no Last-Modified header is sent. Basically, it revokes a Last-Modified header if it is sent by other means (a plugin, for example). In 5.3, that's easy with header_remove().

In 5.2, we would continue to use header('Last-Modified:') as it is the best option, if imperfect. It is better than letting a Last-Modified header sneak through (intended for a cached resource, presumably). It is better than the current time based on Chrome's aggressive caching as reported in #22258. And, I think problems in Chrome outweigh problems on certain server setups, especially when updating to PHP 5.3 fixes it.

In WP 3.4, we always sent (and replaced) the Last-Modified header, which is what I think we should continue to do. In 3.5, we always sent an empty Last-Modified header in PHP 5.2. With the patch, we'll now only send an empty Last-Modified header to override an existing Last-Modified header, as a last resort.

I think this is good for 3.5.1. Thoughts?

comment:11 follow-up: slene19 months ago

OK, I think it is better but a bit complex.

comment:12 in reply to: ↑ 11 nacin19 months ago

Replying to slene:

OK, I think it is better but a bit complex.

Indeed. We can DRY this up in 3.6 if desired. The reason for the complexity is we must fix #22258 (lest we regress in Chrome) and also keep 3.4's behavior of overriding a previously sent header.

comment:13 andy19 months ago

slene, could you try something? I noticed that our current strategy for deleting the Last-Modified header, sending it as an empty string, actually winds up as a header with trailing whitespace:

header("Last-Modified: ");

On your server that results in a Last-Modified value in 1970. What if you fix it so the last character in the string is the colon? Does it stop sending the Last-Modified header?

comment:14 slene19 months ago

header("Last-Modified:");
header("Last-Modified: ");

It always send the Last-Modified 1970 header when it has trailing whitespace or not.

andy19 months ago

New utility: wp_header($name, $value)

comment:15 ryan18 months ago

Test results with 23021.4.diff.

PHP 5.2.17:

  • Last-Modified is not sent when no plugin is adding Last-Modified.
  • Last-Modified is sent but empty when a plugin adds Last-Modified.

PHP 5.3.14:

  • Last-Modified is not sent when no plugin is adding Last-Modified.
  • Last-Modified is not sent when a plugin adds Last-Modified.

PHP 5.4.4 behaves the same way as 5.3.14.

This conforms to the intent expressed above and seems good to me. wp_header() for 3.6 sounds good.

Last edited 18 months ago by ryan (previous) (diff)

nacin18 months ago

comment:16 nacin18 months ago

23021.4.diff's $nocache_headers variable causes a legitimate Last-Modified header to be removed from feeds, when the user is logged in.

Because it is a feed, I imagine always having a header here is by design, even in a no-cache situation. I could see arguments either way, though.

$nocache_headers is really just an enhancement to further avoid tampering by plugins, but we should probably avoid going near that in a point release. Also, since in PHP 5.2 we'd now be looping through headers_list() on every request (as we normally don't send Last-Modified, not just in no-cache situations), I adopted the false from wp_header(). See 23021.6.diff.

nacin18 months ago

comment:17 follow-up: westi18 months ago

23021.6.refreshed.diff seems to make sense to me for 3.5.1

For 3.6 wp_header looks nice - might be good to see if we can avoid using @ suppression there by checking headers_sent().

comment:18 ryan18 months ago

23021.6.refreshed.diff​ behaves the same as .4.diff except feeds retain the Last-Modified set in WP::set_headers(). Looks good.

comment:19 in reply to: ↑ 17 nacin18 months ago

Replying to westi:

For 3.6 wp_header looks nice - might be good to see if we can avoid using @ suppression there by checking headers_sent().

If there is a speed component, I could go for it. However I think I like @ over headers_sent(), because then if you have the error suppression operator disabled ("scream" mode), possibly valid warnings present themselves, while headers_sent() would prevent any warning.

comment:20 ryan18 months ago

In 23267:

Try not to send Last-Modified, even with an empty value. Some servers interpret an empty value as the epoch.

Props nacin, slene, SergeyBiryukov, andy
see #23021 for trunk

comment:21 nacin18 months ago

  • Keywords commit fixed-major added

I think this is ready for 3.5.1.

comment:22 ryan18 months ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In 23281:

Try not to send Last-Modified, even with an empty value. Some servers interpret an empty value as the epoch.

Props nacin, slene, SergeyBiryukov, andy
fixes #23021 for 3.5

Note: See TracTickets for help on using tickets.