Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#38940 closed defect (bug) (duplicate)

current_time() expects that date_default_timezone_set is never used

Reported by: marcomarsala's profile marco.marsala Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.6.1
Component: Date/Time Keywords: has-patch dev-feedback 2nd-opinion
Focuses: Cc:

Description (last modified by Rarst)

Currently, the function current_time() expects date_default_timezone_set() is never used.

Inspecting the code of current_time() confirmed this, because it formats the result using date() function (that is affected by timezone).

In fact seems WordPress always resets the timezone to UTC; an echo date_default_timezone_get(); placed at top and at bottom of wp-config.php in a clean WordPress setup will prove this.

So using date_default_timezone_set() anywhere in theme or plugin code will broke current_time().

Attachments (1)

current_time.patch (910 bytes) - added by marco.marsala 8 years ago.

Download all attachments as: .zip

Change History (12)

#1 @marco.marsala
8 years ago

I'm unsure if this is a documentation fault or a code fault. This limitation should at least be mentioned in documentation (documentation fault) or the code fixed (code fault).

#3 @marco.marsala
8 years ago

  • Keywords has-patch added

A possible code fix would be:

<?php
function current_time( $type, $gmt = 0 ) {
        $oldgmt = date_default_timezone_get();
        date_default_timezone_set('UTC');
        switch ( $type ) {
                case 'mysql':
                        return ( $gmt ) ? gmdate( 'Y-m-d H:i:s' ) : gmdate( 'Y-m-d H:i:s', ( time() + ( get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ) ) );
                case 'timestamp':
                        return ( $gmt ) ? time() : time() + ( get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
                default:
                        return ( $gmt ) ? date( $type ) : date( $type, time() + ( get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ) );
        }
        date_default_timezone_set($oldgmt);
}
Version 0, edited 8 years ago by marco.marsala (next)

#4 @jdgrimes
8 years ago

  • Keywords dev-feedback 2nd-opinion added

I'd say it is more of a documentation problem than a code problem. The code has been this way for a long time, and I don't know if there have been any complaints about bugs arising from code outside WordPress core calling date_default_timezone_set(). It would affect a lot of time-related stuff in WordPress, since date() is used many other places where gmdate() would need to be used instead. Ensuring that we always use gmdate() when getting UTC time would be an extra precaution, but it wouldn't immediately make it OK for code to call date_default_timezone_set(), because a lot of plugins would still be assuming date() to be UTC. So, for backward-compatibility reasons, I doubt that we should change the code. Not unless there actually is a significant specific use-case for making WordPress core resilient against calls to date_default_timezone_set().

#5 follow-up: @marco.marsala
8 years ago

I didn't suggest to change date() into gmdate().

The proposed patch would affect only the bcurrent_time() and has no side effect on date() function outside current_time() calls, look at the code above.

#6 in reply to: ↑ 5 ; follow-up: @jdgrimes
8 years ago

Replying to marco.marsala:

I didn't suggest to change date() into gmdate().

The proposed patch would affect only the bcurrent_time() and has no side effect on date() function outside current_time() calls, look at the code above.

I understand that, but if some code outside of WordPress core uses date_default_timezone_set(), it will affect every call to date() in WordPress (and plugins), not just the current_time() function. current_time() is not used everywhere that WordPress retrieves the current date/time, so other places in the code will still be affected by the changed timezone, even though they are expected to be getting the UTC time. They would need to be updated to use current_time() or gmdate() in that case.

In fact, you could avoid changing the timezone in your patch for current_time(), you could just update it to use gmdate() instead of date(), I think.

Last edited 8 years ago by jdgrimes (previous) (diff)

#7 in reply to: ↑ 6 @marco.marsala
8 years ago

Replying to jdgrimes:

Replying to marco.marsala:

I didn't suggest to change date() into gmdate().

The proposed patch would affect only the bcurrent_time() and has no side effect on date() function outside current_time() calls, look at the code above.

I understand that, but if some code outside of WordPress core uses date_default_timezone_set(), it will affect every call to date() in WordPress (and plugins), not just the current_time() function. current_time() is not used everywhere that WordPress retrieves the current date/time, so other places in the code will still be affected by the changed timezone, even though they are expected to be getting the UTC time. They would need to be updated to use current_time() or gmdate() in that case.

It is what I'm saying. The code should use current_time(), but current_time() is bugged if date_default_timezone_set() is used. The patch aim to fix current_time() without adding side effects.

In fact, you could avoid changing the timezone in your patch for current_time(), you could just update it to use gmdate() instead of date(), I think.

You're completely right.

#8 @Rarst
8 years ago

Ticket description gets scope slightly wrong. current_time() is not broken if PHP timezone is changed, it is just broken altogether in context

<?php
var_dump( date_default_timezone_get() );
// "UTC"

var_dump( date('D, j M Y H:i:s O') );
// "Tue, 2 May 2017 11:38:08 +0000" << correct UTC

var_dump( current_time( 'D, j M Y H:i:s O' ) );
// "Tue, 2 May 2017 14:38:08 +0000" << local time, invalid time zone

var_dump( current_time( 'D, j M Y H:i:s O', true ) );
// "Tue, 2 May 2017 11:38:08 +0000" << correct UTC

The logic of feeding "WP timestamp" into date() is just inherently broken and cannot produce correct output.

Effectively any format with timezone will produce invalid results, unless gmt flag is set.

I am against the suggested patch, this is not PHP API problem and shouldn't be approached as one.

#9 @jdgrimes
8 years ago

The more I work with dates and times in WordPress, the more I think that its date and time API needs to be completely reworked. It is usable to a certain degree when working with GMT times, but if you want to do something relative to the site's timezone, it does just about everything wrong. The method of converting between timezones in current_time(), for example, always uses GMT offsets, instead of actual timezones (meaning that if a timezone observes DST, or has any other quirks, things won't come out correctly). What needs to really happen is using PHP's datetime API, instead of all of these hacks. Probably current_time() needs to retain current behavior for back-compat, be deprecated, and a new helper function introduced. But that would really be for a separate ticket.

#10 @jdgrimes
8 years ago

I've created #40653, which would also have the effect of fixing this, by fixing the underlying design of current_time().

#11 @Rarst
6 years ago

  • Description modified (diff)
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #37440.

Note: See TracTickets for help on using tickets.