#40653 closed enhancement (fixed)
Use PHP `DateTime` class API in `current_time()`
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (25)
#1
@
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
@
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:
↓ 4
@
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
#4
in reply to:
↑ 3
;
follow-up:
↓ 6
@
8 years ago
Replying to Rarst:
That
Etc/*
conversion method forgmt_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
@
8 years ago
Replying to jdgrimes:
Replying to Rarst:
That
Etc/*
conversion method forgmt_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.
#7
@
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.
#8
@
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
@
7 years ago
Having done some work on current_time()
today in #37440 and #40657 here are my current thoughts:
- In GMT mode
current_time()
is not much of a problem. It becomes an alias of upstreamtime()
andgmdate()
. Where that is the case we should be just eliminating it in favor of calling those directly. - 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 (tentativelywp_date()
) then we can getcurrent_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
@
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.
#13
@
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.
An example of how this could be done