Make WordPress Core

Opened 16 years ago

Closed 15 years ago

#9588 closed defect (bug) (fixed)

time() is not GMT time()

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8
Component: Date/Time Keywords: has-patch needs-testing
Focuses: Cc:

Description

this ticket comes in addition to #8662 and #9586. Various patches fix time() calls that really should be GMT.

Attachments (23)

9588.diff (513 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.2.diff (608 bytes) - added by Denis-de-Bernardy 16 years ago.
this one should be using current_time()
9588.3.diff (770 bytes) - added by Denis-de-Bernardy 16 years ago.
third one in template.php
9588.4.diff (631 bytes) - added by Denis-de-Bernardy 16 years ago.
this one needs testing
9588.5.diff (1.2 KB) - added by Denis-de-Bernardy 16 years ago.
9588.6.diff (865 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.7.diff (558 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.8.diff (591 bytes) - added by Denis-de-Bernardy 16 years ago.
this one for akismet
9588.9.diff (1.1 KB) - added by Denis-de-Bernardy 16 years ago.
9588.10.diff (534 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.11.diff (603 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.12.diff (2.3 KB) - added by Denis-de-Bernardy 16 years ago.
9588.13.diff (595 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.14.diff (818 bytes) - added by Denis-de-Bernardy 16 years ago.
9588.15.diff (1.7 KB) - added by Denis-de-Bernardy 16 years ago.
this one fixes potential cron issues
Picture 1.png (20.8 KB) - added by Denis-de-Bernardy 15 years ago.
calendar bug
9588.16.diff (406 bytes) - added by Denis-de-Bernardy 15 years ago.
9588.17.diff (1.7 KB) - added by ryan 15 years ago.
9588.18.diff (2.2 KB) - added by ryan 15 years ago.
9588.19.diff (6.5 KB) - added by ryan 15 years ago.
It gets uglier.
9588.20.diff (8.7 KB) - added by ryan 15 years ago.
9588.21.diff (8.7 KB) - added by ryan 15 years ago.
9588.22.diff (448 bytes) - added by ryan 15 years ago.
Always use UTC as the default timezone.

Download all attachments as: .zip

Change History (67)

@Denis-de-Bernardy
16 years ago

this one should be using current_time()

@Denis-de-Bernardy
16 years ago

third one in template.php

@Denis-de-Bernardy
16 years ago

this one needs testing

@Denis-de-Bernardy
16 years ago

this one for akismet

@Denis-de-Bernardy
16 years ago

this one fixes potential cron issues

#1 @Denis-de-Bernardy
16 years ago

  • Keywords has-patch needs-testing dev-feedback added

I left a few out:

  • When it's compared to filesystem times, it's left untouched
  • When it's for internal use only (never interacts with user, e.g. ftp class, unzip), it's left untouched

Those that got diff'ed above potentially affect/correct:

  • Javascripts, in the form of autosave time
  • Cron jobs, in the form of future posting date
  • Date output bugs
  • In short, anything that the user might spot

#2 @Denis-de-Bernardy
16 years ago

Patch number 15 is almost certainly needed, because _future_post_hook() uses the GMT time rather than the server time (or the local time).

#3 @Denis-de-Bernardy
16 years ago

possibly related: #9162

#4 @ryan
16 years ago

  • Component changed from General to Date/Time
  • Owner anonymous deleted

#5 @ryan
16 years ago

9588.10.diff breaks UTC for me. I think gmdate needs straight time() in this case. (This is with #8662 applied).

#6 @ryan
16 years ago

In general, gmdate() seems to need the timestamps passed to it based off of time(). The cases where we add offset to time() and pass that to gmdate() look like they work. gmdate() appears to be compensating for the server's offset, so we shouldn't pass something that already compensates for it..

#7 @Denis-de-Bernardy
16 years ago

Suggestion: We fix #8662 and probably #9586, and move this one to 2.9. I mean, it has been broken for so long that it won't hurt if it's broken for a few more months, and we'll be able to look into it more calmly. :-)

#9 @Denis-de-Bernardy
16 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

#10 @ryan
16 years ago

Testing on various servers and always getting GMT for time(). According to PHP docs, time():

Returns the current time measured in the number of seconds since the Unix Epoch (January 1 1970 00:00:00 GMT).

The timestamp is relative to GMT so it is indeed GMT.

#11 @Denis-de-Bernardy
16 years ago

  • Milestone 2.9 deleted
  • Resolution set to invalid
  • Status changed from new to closed

#12 @Denis-de-Bernardy
15 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

@Denis-de-Bernardy
15 years ago

calendar bug

#13 @Denis-de-Bernardy
15 years ago

attached is a picture of a calendar bug:

http://core.trac.wordpress.org/attachment/ticket/9588/Picture%201.png

The two dumps are in the get_calendar() function:

	dump($unixmonth, date('Y-m-d', $unixmonth));
	// Get days with posts
	$dayswithposts = $wpdb->get_results("SELECT DISTINCT DAYOFMONTH(post_date)
		FROM $wpdb->posts WHERE MONTH(post_date) = '$thismonth'
		AND YEAR(post_date) = '$thisyear'
		AND post_type = 'post' AND post_status = 'publish'
		AND post_date < '" . current_time('mysql') . '\'', ARRAY_N);
	if ( $dayswithposts ) {
		foreach ( (array) $dayswithposts as $daywith ) {
			$daywithpost[] = $daywith[0];
		}
	} else {
		$daywithpost = array();
	}
	dump($unixmonth, date('Y-m-d', $unixmonth));

The laptop is using the CEST timezone vs the site's EST timezone.

I'm nailing this down to the wp_timezone_override_offset() function. Probably, if not certainly:

@date_default_timezone_set( $timezone_string );

#14 @Denis-de-Bernardy
15 years ago

On that calendar bug, using the following throughout get_calendar():

date('Y-m-d', $unixmonth)

instead of:

current_time('mysql')

seems to do the trick, but it's a clunky fix imo -- it works around the symptom instead of fixing the actual bug.

#15 @Denis-de-Bernardy
15 years ago

In case anyone gets it, commenting out the following, in the default filters, also removes the above-mentioned calendar issue:

add_filter('pre_option_gmt_offset','wp_timezone_override_offset');

#16 @Denis-de-Bernardy
15 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone set to 2.9

9588.16.diff also seems to work.

#17 @Otto42
15 years ago

The differences you're seeing between date calls is because the current_time call causes WordPress to load the gmt_offset, which then corrects the default timezone setting. PHP's date is subject to timezone adjustment, it's not supposed to be GMT. If you want GMT, you call gmdate.

In other words, this will demonstrate the problem (if you call it early enough in the execution):

dump($unixmonth, date('Y-m-d', $unixmonth));
get_option( 'gmt_offset' );
dump($unixmonth, date('Y-m-d', $unixmonth));

The correct solution to this is to make WordPress call wp_timezone_override_offset earlier, preferably as soon as the database is available. This allows the date_default_timezone_set function to get run, setting the local timezone properly early on.

#18 follow-up: @Denis-de-Bernardy
15 years ago

as much as I agree on the diagnosis, this ticket and #9586 and #8662 were opened precisely because time() is not a gm time and it occasionally has side effects because WP makes the assumption that it is.

testing revealed that it "automagically" corrects itself due to wp_timezone_override_offset(). further up, I've attached a screenshot where time() is used before the latter function is called, complete with the resulting side effect and bug.

#19 in reply to: ↑ 18 @Otto42
15 years ago

Replying to Denis-de-Bernardy:

as much as I agree on the diagnosis, this ticket and #9586 and #8662 were opened precisely because time() is not a gm time and it occasionally has side effects because WP makes the assumption that it is.

Agreed. Anywhere in WordPress where it's using time() or date() and assuming that that is GMT is wrong. The new timezone handling code for PHP5 installs makes this assumption invalid. All those should definitely be changed to gmtime() and gmdate().

#20 @Denis-de-Bernardy
15 years ago

Side note.... I just ran into this bugger:

http://wordpress.org/support/topic/285337

Adding the following to the wp-config.php fixed things:

date_default_timezone_set('UTC');

#21 @lloydbudd
15 years ago

  • Milestone changed from 2.9 to 3.0

#22 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 3.0 to 2.9
  • Resolution set to fixed
  • Status changed from reopened to closed

I believe this is fixed in r12315

#23 follow-up: @Otto42
15 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Depends, are any of the 14 patches you posted before still using time() where they should be using gmtime()? If so, they still need correcting, since the timezone will change once the real local timezone gets loaded (which happens when the gmt_offset option is used).

I think those patches you posted before still need to be examined, and those replaced with gm* functions if that is the underlying assumption behind them.

#24 in reply to: ↑ 23 @Denis-de-Bernardy
15 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to Otto42:

Depends, are any of the 14 patches you posted before still using time() where they should be using gmtime()? If so, they still need correcting, since the timezone will change once the real local timezone gets loaded (which happens when the gmt_offset option is used).

I think those patches you posted before still need to be examined, and those replaced with gm* functions if that is the underlying assumption behind them.

I'd agree if php4 wasn't unmaintained for over 2 years. re-closing...

it's minor edge cases. we've lived with this bug for years, the patches initially got dismissed; we can live with it until php4 is completely phased out.

#25 follow-up: @Otto42
15 years ago

No, my concern isn't PHP4, it's the fact that the output of time() is going to change midway through WordPress' execution.

If you're setting the default to UTC early on, then resetting it later (for that 23/24ths of the world not in GMT time), then time() will give one answer at one point and a different answer later in the process. If anywhere you happen to compare the values from before and after that point, you're going to get weird results.

This can still cause problems on PHP5 setups. Those instances using time() need to be examined to see if they should be using gmtime() instead.

#26 in reply to: ↑ 25 @Denis-de-Bernardy
15 years ago

Replying to Otto42:

If you're setting the default to UTC early on, then resetting it later (for that 23/24ths of the world not in GMT time), then time() will give one answer at one point and a different answer later in the process. If anywhere you happen to compare the values from before and after that point, you're going to get weird results.

This can still cause problems on PHP5 setups. Those instances using time() need to be examined to see if they should be using gmtime() instead.

It's quite early on:

http://core.trac.wordpress.org/browser/trunk/wp-settings.php?rev=12315#L18

one gets warnings if it's placed anywhere later. I do so your point, though. e.g. #9586...

FWIW, I've been Ryan's two lines in the wp-config.php files of each of my sites for longer than I can remember. I also added it to my WP Fixes plugin, so that my customers can use it on their end. It *fixed* #9586 as a side effect, and I've yet to see reports of fishy sounding odd behaviors.

#27 @ryan
15 years ago

  • Milestone changed from 2.9 to 3.0
  • Resolution fixed deleted
  • Status changed from closed to reopened

Let's address this once and for all in 3.0.

#29 @ryan
15 years ago

There is no gmtime(). For PHP 5.3, I think we can use DateTime::getTimestamp().

#30 @Otto42
15 years ago

Let me investigate this a bit more, something is fishy here and I'm not sure what it is.

@ryan
15 years ago

#31 @ryan
15 years ago

Patch tries gmdate('U') and incorporate patch from #11665. Seems to work here, but still experimenting.

If we get current_time() working reliably, we should use it everywhere instead of time() and friends.

@ryan
15 years ago

#32 @ryan
15 years ago

Branching for DateTime support in current_time() was the easiest and most reliable way to get this working for me. Patch fixes #11672. Converting time() to current_time() still needs to happen in a lot of places.

@ryan
15 years ago

It gets uglier.

@ryan
15 years ago

#33 @ryan
15 years ago

This will only work when there is a timezone_string though. I think we'll have to map a gmt_offset to a timezone when upgrading.

#34 @ryan
15 years ago

Or lose setting ini_get('date.timezone') as the default.

@ryan
15 years ago

#35 follow-up: @ryan
15 years ago

Alternatively, always set the default timezone to UTC and remove the default timezone set in wp_timezone_override_offset(). Then everything behaves as before.

#36 in reply to: ↑ 35 ; follow-up: @Otto42
15 years ago

Replying to ryan:

Alternatively, always set the default timezone to UTC and remove the default timezone set in wp_timezone_override_offset(). Then everything behaves as before.

Before was broken. It's only by setting the correct default timezone that I can get correct behavior.

Short version is that the default timezone should be set to the timezone_string (if set) at the earliest possible moment. Anywhere in core that is expecting the current GMT should be adjusted to use better functions to get that time, instead of using time() calls. current_time() is the obvious choice, it could be adjusted to behave appropriately.

And that's basically that.

Note that time() should always returns an absolute value of seconds in GMT. It is supposed to be unaffected by timezone settings. The problem is that sometimes we apply offset to it and sometimes not. We just need to remember to apply the offset correctly.

What cases still exist that create incorrect results?

#37 in reply to: ↑ 36 @ryan
15 years ago

Replying to Otto42:

Replying to ryan:

Alternatively, always set the default timezone to UTC and remove the default timezone set in wp_timezone_override_offset(). Then everything behaves as before.

Before was broken. It's only by setting the correct default timezone that I can get correct behavior.

Before as in the PHP4 case, which worked. Forcing the default timezone to UTC brings us back to what has always worked for us. We can always add our offset since times are UTC. This requires less code churn but probably isn't the best way forward.

Short version is that the default timezone should be set to the timezone_string (if set) at the earliest possible moment. Anywhere in core that is expecting the current GMT should be adjusted to use better functions to get that time, instead of using time() calls. current_time() is the obvious choice, it could be adjusted to behave appropriately.

The latest patch sets it early. However, there's a choice of setting it to UTC and continuing to do our offsets as mentioned above or setting it to timezone_string and conditionally doing the offsets only if the default timezone isn't set or supported by the PHP version. The patch sets it to timezone_string and conditionally adds the offset for older versions but omits adding it for newer versions with a proper timezone set.

And that's basically that.

Note that time() should always returns an absolute value of seconds in GMT. It is supposed to be unaffected by timezone settings. The problem is that sometimes we apply offset to it and sometimes not. We just need to remember to apply the offset correctly.

What cases still exist that create incorrect results?

Indeed, time() is independent of the timezone. The trouble is finding all of the little places where we were relying on the old behavior and making them work with both the old and new ways. I think I've got most of them.

#38 @ryan
15 years ago

Summary of latest patch:

  • If timezone_string is set, make that the default timezone in wp-settings.php
  • If timezone_string is not set, make UTC the default timezone. This way we can add our offsets.
  • If running a version of PHP without timezone support, no default can be set.
  • In current_time(), add the offset if timezone_string is not set or if the PHP version does not support timezones.
  • If the default timezone is set, do not add offsets since date(), gmdate(), etc. do the right thing for us.
  • Add compate_date() to work around the differences between the old assumptions and the new. Only touch_time() uses this at the moment. There may be a way to update touch_time() to work correctly either way without an ugly workaround.
  • current_time() is used in more places.

#39 @ryan
15 years ago

The changes to current_time() in that patch mean the same timestamp is returned whether the gmt flag is true or false when the default timezone is set. That's kinda lame but necessary to work with date() when a default timezone is set since date() expects UTC time and does its own offsetting. The alternative would be to continue adding the offset and wrap all of the date functions so that they expect an offset when a default timezone is not set and don't expect one when there is no default timezone support. Ugh.

#40 @ryan
15 years ago

That should read: So that they expect an offset when a default timezone is not set and don't expect one when a default timezone is set.

@ryan
15 years ago

Always use UTC as the default timezone.

#41 @ryan
15 years ago

Alternative approach that removes date_default_timezone_set() from wp_timezone_override_offset(). UTC is always the default timezone and we always do our own offset adding. This seems to be the only way to have date() behave the same way across different versions of PHP and allows plugin authors to continue passing current_time() to date() without us having to hack current_time() to return the same timestamp whether gmt is requested or not or without them having to call date differently depending on the PHP version.

#42 @ryan
15 years ago

Another advantage to always using UTC is it makes things easier in multisite (MU) environments where switching to a blog (via switch_to_blog, for example) with a different timezone could stomp on default timezone settings.

#43 @ryan
15 years ago

See also #11665

#44 @ryan
15 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.