WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 months ago

#40653 new enhancement

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

Reported by: jdgrimes Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 1.0
Component: Date/Time Keywords: has-patch needs-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 (4)

current_time.diff (2.5 KB) - added by jdgrimes 7 months ago.
An example of how this could be done
40653.diff (2.4 KB) - added by jdgrimes 7 months ago.
Fixes method of converting offsets to timezone strings
40653.2.diff (1.7 KB) - added by jdgrimes 6 months ago.
Don't use DateTImeZone with offsets
40653.3.diff (1.7 KB) - added by jdgrimes 6 months ago.
Add missing breaks in switch

Download all attachments as: .zip

Change History (11)

@jdgrimes
7 months ago

An example of how this could be done

#1 @jdgrimes
7 months 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
7 months 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
7 months 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
7 months ago

Fixes method of converting offsets to timezone strings

#4 in reply to: ↑ 3 ; follow-up: @jdgrimes
7 months 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.


7 months ago

#6 in reply to: ↑ 4 @jdgrimes
6 months 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
6 months ago

Don't use DateTImeZone with offsets

#7 @jdgrimes
6 months 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
6 months ago

Add missing breaks in switch

Note: See TracTickets for help on using tickets.