WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#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 2 years 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 2 years ago.
23021-3.patch (2.2 KB) - added by azaozz 2 years ago.
23021.3.diff (1.1 KB) - added by nacin 2 years ago.
23021.4.diff (3.4 KB) - added by nacin 2 years ago.
23021.nospace.diff (1.1 KB) - added by nacin 2 years ago.
23021.5.diff (1.8 KB) - added by slene 2 years ago.
should be unset Last-Modified header when it empty.
wp_header.diff (1.5 KB) - added by andy 2 years ago.
New utility: wp_header($name, $value)
23021.6.diff (2.6 KB) - added by nacin 2 years ago.
23021.6.refreshed.diff (2.7 KB) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 @SergeyBiryukov2 years ago

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

@slene2 years ago

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

comment:2 @ocean902 years 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 @slene2 years ago

Will this ticket can change Milestone to 3.5.1 ?

@SergeyBiryukov2 years ago

comment:4 @SergeyBiryukov2 years 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 @nacin2 years ago

  • Milestone changed from 3.6 to 3.5.1

comment:6 @azaozz2 years 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.

@azaozz2 years ago

comment:7 @azaozz2 years 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.

@nacin2 years ago

@nacin2 years ago

@nacin2 years ago

comment:8 follow-up: @nacin2 years 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 @slene2 years 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

@slene2 years ago

should be unset Last-Modified header when it empty.

comment:10 @nacin2 years 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: @slene2 years ago

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

comment:12 in reply to: ↑ 11 @nacin2 years 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 @andy2 years 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 @slene2 years ago

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

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

@andy2 years ago

New utility: wp_header($name, $value)

comment:15 @ryan2 years 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 2 years ago by ryan (previous) (diff)

@nacin2 years ago

comment:16 @nacin2 years 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.

@nacin2 years ago

comment:17 follow-up: @westi2 years 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 @ryan2 years 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 @nacin2 years 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 @ryan2 years 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 @nacin2 years ago

  • Keywords commit fixed-major added

I think this is ready for 3.5.1.

comment:22 @ryan2 years 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.