Opened 16 years ago
Closed 15 years ago
#9588 closed defect (bug) (fixed)
time() is not GMT time()
Reported by: | 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: |
Attachments (23)
Change History (67)
#1
@
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
@
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).
#5
@
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
@
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..
#10
@
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
@
16 years ago
- Milestone 2.9 deleted
- Resolution set to invalid
- Status changed from new to closed
#13
@
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
@
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
@
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
@
15 years ago
- Keywords has-patch added; needs-patch removed
- Milestone set to 2.9
9588.16.diff also seems to work.
#17
@
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:
↓ 19
@
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
@
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
@
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');
#22
@
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:
↓ 24
@
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
@
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:
↓ 26
@
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
@
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
@
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.
#30
@
15 years ago
Let me investigate this a bit more, something is fishy here and I'm not sure what it is.
#31
@
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.
#32
@
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.
#33
@
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.
#35
follow-up:
↓ 36
@
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:
↓ 37
@
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
@
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
@
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
@
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
@
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.
#41
@
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.
this one should be using current_time()