Make WordPress Core

Opened 8 years ago

Closed 5 years ago

Last modified 3 years ago

#40653 closed enhancement (fixed)

Use PHP `DateTime` class API in `current_time()`

Reported by: jdgrimes's profile jdgrimes Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 1.0
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The current_time() function has been in WordPress for a long time. It was introduced before PHP offered the robust DateTime class and surrounding API that was introduced in 5.2. This was back in the days of hoary old things like PHP 4, and these were still formative years. Without ceremony, in [549], current_time() was born as a helper function to replace this code from b2:

<?php
$time_difference = get_settings("time_difference");
$now = date("Y-m-d H:i:s",(time() + ($time_difference * 3600)));

In the intervening years the function has remained little changed, as far as the underlying design goes. But the method of calculating local dates by using offsets from UTC is no longer necessary. It is now a hack, solved by PHP a short eternity ago, and is prone to confuse new devs and provide them a bad pattern to follow.

This is made worse by the fact that the function is not very transparent. It appears not to use the site's timezone in its calculations at all. Instead, it uses GMT offsets, which in fact only approximate the site's timezone, without regard for daylight savings and other quirks. Actually though, the wp_timezone_override_offset() function is hooked to the 'pre_option_gmt_offset' filter behind the scenes to correctly calculate the offset for a particular moment in time from the site's timezone string, if set.

The real heart of the problem, though, is that current_time() always calculates dates in UTC time. So passing a custom format will have unexpected results if timezone-related information is included (as pointed out by @Rarst).

I believe that we can make current_time() less confusing and less buggy by utilizing the powerful tools now built into PHP, specifically, the DateTime object and its support for timezone-relative calculations. See the attached patch for an example.

In summary, this will:

  • Fix a bug when passing custom timezone-referencing format strings.
  • Make the function's behavior more transparent.
  • Provide a better pattern for learning devs to follow.

Attachments (7)

current_time.diff (2.5 KB) - added by jdgrimes 8 years ago.
An example of how this could be done
40653.diff (2.4 KB) - added by jdgrimes 8 years ago.
Fixes method of converting offsets to timezone strings
40653.2.diff (1.7 KB) - added by jdgrimes 8 years ago.
Don't use DateTImeZone with offsets
40653.3.diff (1.7 KB) - added by jdgrimes 8 years ago.
Add missing breaks in switch
current-time.patch (3.6 KB) - added by Rarst 6 years ago.
Needs wp_timezone() merged.
current-time1.patch (3.7 KB) - added by Rarst 6 years ago.
Improved test.
current-time2.patch (3.8 KB) - added by Rarst 6 years ago.
Made timestamp always int and added to unit test.

Download all attachments as: .zip

Change History (25)

@jdgrimes
8 years ago

An example of how this could be done

#1 @jdgrimes
8 years ago

  • Keywords has-patch needs-unit-tests added

I've just created #40657, which addresses one legacy feature of this function that wouldn't be retained when using the DateTime API, without the special handling included in the patch.

#2 @jdgrimes
8 years ago

Note that this would also fix #38940, since the server timezone would no longer have any affect on the behavior of current_time().

#3 follow-up: @Rarst
8 years ago

That Etc/* conversion method for gmt_offset case is unreliable (won't handle half–hour cases for example).

For my take on it see my WpDateTime lib: https://github.com/Rarst/wpdatetime/blob/master/php/WpDateTimeZone.php

@jdgrimes
8 years ago

Fixes method of converting offsets to timezone strings

#4 in reply to: ↑ 3 ; follow-up: @jdgrimes
8 years ago

Replying to Rarst:

That Etc/* conversion method for gmt_offset case is unreliable (won't handle half–hour cases for example).

For my take on it see my WpDateTime lib: https://github.com/Rarst/wpdatetime/blob/master/php/WpDateTimeZone.php

Thanks for pointing that out. I suspected that there was an issue there and was planing to take a closer look at that today. I've updated the patch, see 40653.diff.

This ticket was mentioned in Slack in #core by jdgrimes. View the logs.


8 years ago

#6 in reply to: ↑ 4 @jdgrimes
8 years ago

Replying to jdgrimes:

Replying to Rarst:

That Etc/* conversion method for gmt_offset case is unreliable (won't handle half–hour cases for example).

For my take on it see my WpDateTime lib: https://github.com/Rarst/wpdatetime/blob/master/php/WpDateTimeZone.php

Thanks for pointing that out. I suspected that there was an issue there and was planing to take a closer look at that today. I've updated the patch, see 40653.diff.

Unfortunately, this does not work on PHP versions prior to 5.5, because DateTimeZone doesn't accept offset strings in PHP 5.4 and before.

I found an alternative solution using timezone_name_from_abbr(), but when running it over the offsets provided by WordPress on the General Settings screen, it doesn't work for all of them (and doesn't really work correctly in all cases anyway).

Code used to test:

<?php
function convert_from_seconds( $seconds ) {

        // Get timezone name from seconds
        $tz = timezone_name_from_abbr( '', $seconds, 1 );
        // Workaround for bug #44780
        if ( $tz === false ) {
                $tz = timezone_name_from_abbr( '', $seconds, 0 );
        }

        if ( ! $tz ) {
                return false;
        }

        // Set timezone
        date_default_timezone_set( $tz );

        echo $tz . ': ' . date( 'r' );
        echo PHP_EOL;

        return true;
}

$offset_range = array (-12, -11.5, -11, -10.5, -10, -9.5, -9, -8.5, -8, -7.5, -7, -6.5, -6, -5.5, -5, -4.5, -4, -3.5, -3, -2.5, -2, -1.5, -1, -0.5,
        0, 0.5, 1, 1.5, 2, 2.5, 3, 3.5, 4, 4.5, 5, 5.5, 5.75, 6, 6.5, 7, 7.5, 8, 8.5, 8.75, 9, 9.5, 10, 10.5, 11, 11.5, 12, 12.75, 13, 13.75, 14);

foreach ( $offset_range as $hours ) {
        echo $hours . ': ';
        if ( ! convert_from_seconds( $hours * 60 * 60 ) ) {
                echo 'failed!' . PHP_EOL;
        }
}

Output:

-12: failed!
-11.5: failed!
-11: Pacific/Apia: Tue, 09 May 2017 09:35:52 +1300
-10.5: failed!
-10: Pacific/Honolulu: Mon, 08 May 2017 10:35:52 -1000
-9.5: failed!
-9: America/Anchorage: Mon, 08 May 2017 12:35:52 -0800
-8.5: failed!
-8: America/Anchorage: Mon, 08 May 2017 12:35:52 -0800
-7.5: failed!
-7: America/Los_Angeles: Mon, 08 May 2017 13:35:52 -0700
-6.5: failed!
-6: America/Denver: Mon, 08 May 2017 14:35:52 -0600
-5.5: failed!
-5: America/Chicago: Mon, 08 May 2017 15:35:52 -0500
-4.5: America/Caracas: Mon, 08 May 2017 16:35:52 -0400
-4: America/New_York: Mon, 08 May 2017 16:35:52 -0400
-3.5: failed!
-3: America/Halifax: Mon, 08 May 2017 17:35:52 -0300
-2.5: failed!
-2: America/Sao_Paulo: Mon, 08 May 2017 17:35:52 -0300
-1.5: failed!
-1: Atlantic/Azores: Mon, 08 May 2017 20:35:52 +0000
-0.5: failed!
0: Atlantic/Azores: Mon, 08 May 2017 20:35:52 +0000
0.5: failed!
1: Europe/London: Mon, 08 May 2017 21:35:52 +0100
1.5: failed!
2: Europe/Paris: Mon, 08 May 2017 22:35:52 +0200
2.5: failed!
3: Europe/Helsinki: Mon, 08 May 2017 23:35:52 +0300
3.5: failed!
4: Europe/Moscow: Mon, 08 May 2017 23:35:52 +0300
4.5: failed!
5: Asia/Karachi: Tue, 09 May 2017 01:35:52 +0500
5.5: Asia/Kolkata: Tue, 09 May 2017 02:05:52 +0530
5.75: Asia/Katmandu: Tue, 09 May 2017 02:20:52 +0545
6: Asia/Yekaterinburg: Tue, 09 May 2017 01:35:52 +0500
6.5: failed!
7: Asia/Novosibirsk: Tue, 09 May 2017 03:35:52 +0700
7.5: failed!
8: Asia/Krasnoyarsk: Tue, 09 May 2017 03:35:52 +0700
8.5: failed!
8.75: failed!
9: Asia/Tokyo: Tue, 09 May 2017 05:35:52 +0900
9.5: failed!
10: Australia/Melbourne: Tue, 09 May 2017 06:35:52 +1000
10.5: Australia/Adelaide: Tue, 09 May 2017 06:05:52 +0930
11: Australia/Melbourne: Tue, 09 May 2017 06:35:52 +1000
11.5: failed!
12: Pacific/Auckland: Tue, 09 May 2017 08:35:52 +1200
12.75: failed!
13: Pacific/Auckland: Tue, 09 May 2017 08:35:52 +1200
13.75: failed!
14: failed!

In short, there is no way to use DateTimeZone with offsets in versions prior to PHP 5.5.

I will be working on a new patch that avoids that.

@jdgrimes
8 years ago

Don't use DateTImeZone with offsets

#7 @jdgrimes
8 years ago

40653.2.diff avoids using DateTimeZone with the offsets. This has the effect of making the patch seem much more complex. Although some of the complexity was just previously hidden in wp_timezone_override_offset(). Still, I am questioning whether this entire proposal is worth it. Perhaps it would be better to revisit in 2029, when PHP 5.5 is the minimum version supported by WordPress? I suppose though, that some of the bugs in current_time() are really best fixed in this manner, so the added complexity is just the price paid for legacy mistakes.

The patch also ensures that an int is returned when a timestamp is requested, which the previous patches neglected to do.

@jdgrimes
8 years ago

Add missing breaks in switch

#8 @Rarst
7 years ago

By now I believe we need to be deprecating this function. Some minimal effort to patch up the most wrong cases is in order, but it's just one of the worst pieces of WP’s old offset-centric time zone design. The function just does too much and most of it in a wrong way.

#9 @Rarst
7 years ago

Having done some work on current_time() today in #37440 and #40657 here are my current thoughts:

  1. In GMT mode current_time() is not much of a problem. It becomes an alias of upstream time() and gmdate(). Where that is the case we should be just eliminating it in favor of calling those directly.
  2. In "local" mode it "works" as long as there is no time zone in format, because that one will be wrong. Well and WP timestamp is a disaster. However I don't think this is worth fixing in current_time(). We already have another [broken] function that knows how to do this — date_i18n(). If we make a replacement for that one (tentatively wp_date()) then we can get current_time() fixed "for free" by upgrading to use it.

So at the moment I am focusing on reducing the usage of current_time() in core and adding unit tests for it.

I don't think we will be in position to easily drop current_time() or "fix" it (since local time is broken on design level), but I think we are on a good track to downplay its usage and gradually make it a non–issue.

#10 @Rarst
6 years ago

  • Keywords needs-patch added; has-patch removed

Ok, I think this is going to lingering more than I'd like it to, so at least some fixup is in order (which gets easier with upcoming wp_timezone()).

The challenge is how do we fix local time behavior without breaking WP timestamp behavior?

I am of mind that timestamp or U (verbatim) formats without GMT flag should trigger a legacy WP timestamp path, everything else should produce correct local time for any format. We can document timestamp branch as discouraged and potentially formally deprecate it at a later time.

Will take a stab on a patch.

#11 @Rarst
6 years ago

#45193 was marked as a duplicate.

#12 @Rarst
6 years ago

#29063 was marked as a duplicate.

@Rarst
6 years ago

Needs wp_timezone() merged.

#13 @Rarst
6 years ago

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed

Added patch and unit tests with logic suggested above. Needs wp_timezone() from #24730 merged first.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


6 years ago

#15 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@Rarst
6 years ago

Improved test.

@Rarst
6 years ago

Made timestamp always int and added to unit test.

#16 @SergeyBiryukov
5 years ago

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

In 45856:

Date/Time: Use PHP DateTime class API in current_time().

Only use the legacy WP timestamp approach (a sum of timestamp and timezone offset) for timestamp and U formats without the $gmt flag.

Otherwise, make sure the function returns correct local time for any format.

Props Rarst, jdgrimes.
Fixes #40653.

#17 @Rarst
5 years ago

#47494 was marked as a duplicate.

#18 @johnbillion
3 years ago

In 51950:

Date/Time: Improve the docblocks for various date and time related functions.

See #53399, #28992, #40653

Note: See TracTickets for help on using tickets.