#24730 closed enhancement (fixed)
Introduce a timezone-retrieval method
Reported by: | rmccue | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Date/Time | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
When working with times and dates, it's useful to have the timezone used in WordPress, however there's no unified way to get it.
get_option('gmt_offset')
is filtered and the current offset is translated from the timezone string, but there's no reverse which is needed when working with relative times and dates.
There are two ways we could handle this:
- Filter
timezone_string
and work out the correct string - This will likely break the admin interface for it. - Introduce a new function to get the timezone from
timezone_string
, and fall back togmt_offset
if needed.
Patch has implementation of the second option (also using DateTimeZone), can be adapted as necessary for the first.
Attachments (4)
Change History (31)
This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.
9 years ago
#5
@
7 years ago
I am against using Etc/
time zones, they are not meant or guaranteed for normal operation in modern PHP code.
PHP can simply instantiate generic time zones from +00:00 format now, but it requires PHP version 5.5 which isn't available to WP core. Frankly I would just do that anyway as progressive enhancement, but that hadn't been an accepted practice in core dev.
Anyone who needs to deal with that in sane fashion should just do that in their own code, in my opinion. I implemented this in my WpDateTime lib https://github.com/Rarst/wpdatetime/blob/master/php/WpDateTimeZone.php#L23-L26
#6
@
6 years ago
Maybe a solution to support PHP > 5.2 < 5.5.10:
<?php function wp_get_timezone() { $timezone_string = get_option( 'timezone_string' ); if ( ! empty( $timezone_string ) ) { return new DateTimeZone( $timezone_string ); } $gmt_offset = get_option( 'gmt_offset' ); $hours = (int) $gmt_offset; $minutes = abs( ( $gmt_offset - (int) $gmt_offset ) * 60 ); $offset = sprintf( '%+03d:%02d', $hours, $minutes ); if ( version_compare( PHP_VERSION, '5.5.10', '<' ) ) { $date = new DateTime( $offset ); return $date->getTimezone(); } return new DateTimeZone( $offset ); }
#7
@
6 years ago
Hm hm, I thought about going through DateTime() before but wrongly assumed that would fail on 5.2. The approach does seem to work on 5.2.17 from a quick test!
#8
@
6 years ago
I am very hopeful about this one now, if we have combined timezone retrieval then awful lot of offset math code can be upgraded to DateTime.
Will work on writing unit tests for this.
PS I would prefer wp_timezone()
for name, since I am working on wp_date()
to replace date_i18n()
(ticket pending, getting there).
#9
@
6 years ago
Cool, we also have some unit tests for this already in place:
https://github.com/pronamic/wp-datetime/blob/develop/tests/tests/DateTimeTest.php
Inspired by your https://github.com/Rarst/wpdatetime library.
#10
@
6 years ago
Will take a look. :) Quick note right away — you absolutely need to include timezones in any date_i18n()
output tests. That's the part that is most broken, and I am starting on correcting core tests not doing it, as I go through fixing things.
#11
@
6 years ago
Good point, just added timezones in the tests. That part was indeed broken ;), but i think it's fixed now:
But there is an issue on PHP 5.3:
<?php $date_1 = new DateTime( '+12:45' ); $timezone = $date_1->getTimezone(); $date_2 = new DateTime( 'now', new DateTimeZone( 'UTC' ) ); $date_2->setTimezone( $timezone );
Will result in the following error:
DateTime::setTimezone(): Can only do this for zones with ID for now
You can easily test it with PHP <= 5.3 on http://sandbox.onlinephpfunctions.com/.
Side Note: I'm not a big fan of all the preg_replace
calls in the date_i18n
function:
- https://github.com/WordPress/WordPress/blob/4.9.6/wp-includes/functions.php#L103-L119
- https://github.com/WordPress/WordPress/blob/4.9.6/wp-includes/functions.php#L120-L137
Maybe we could rewrite this to something like this:
<?php private function format_i18n_translate( $format ) { global $wp_locale; if ( empty( $wp_locale->month ) || empty( $wp_locale->weekday ) ) { return $format; } $month = $wp_locale->get_month( $this->format( 'm' ) ); $weekday = $wp_locale->get_weekday( $this->format( 'w' ) ); $format_length = strlen( $format ); $format_new = ''; for ( $i = 0; $i < $format_length; $i++ ) { switch ( $format[ $i ] ) { case 'D': $format_new .= backslashit( $wp_locale->get_weekday_abbrev( $weekday ) ); break; case 'F': $format_new .= backslashit( $month ); break; case 'l': $format_new .= backslashit( $weekday ); break; case 'M': $format_new .= backslashit( $wp_locale->get_month_abbrev( $month ) ); break; case 'a': $format_new .= backslashit( $wp_locale->get_meridiem( $this->format( 'a' ) ) ); break; case 'A': $format_new .= backslashit( $wp_locale->get_meridiem( $this->format( 'A' ) ) ); break; case '\\': $format_new .= $format[ $i ]; if ( $i < $format_length ) { $i++; } // no break default: $format_new .= $format[ $i ]; break; } } return $format_new; }
I do something similar to prevent date_i18n
to format the timezones characters:
<?php private function format_i18n_timezone( $format ) { $format_length = strlen( $format ); $format_new = ''; for ( $i = 0; $i < $format_length; $i++ ) { switch ( $format[ $i ] ) { case 'P': case 'I': case 'O': case 'T': case 'Z': case 'e': $format_new .= backslashit( $this->format( $format[ $i ] ) ); break; case '\\': $format_new .= $format[ $i ]; if ( $i < $format_length ) { $i++; } // no break default: $format_new .= $format[ $i ]; break; } } return $format_new; }
#12
@
6 years ago
DateTime::setTimezone(): Can only do this for zones with ID for now
Ugh, I new there is a catch after all. Will look into it. Need to think if producing such "incomplete" (?) time zone object on old PHP versions is still worth it. It is, as for me...
Separate issue for formatting thing please. I think there is already one for escaping around, note that there is not just "escaped" case, but "escaped escaping" when slashes themselves are slashed, and so on.
I am not sure what you mean by "preventing formatting". If date_i18n()
doesn't process timezone then output is incorrect since date()
will assume UTC.
#13
@
6 years ago
DateTime::setTimezone(): Can only do this for zones with ID for now
It looks like this issue is only active in PHP < 5.4.26
or > 5.5 < 5.5.10
:
https://3v4l.org/mlZX7
Fixed it like this: https://github.com/pronamic/wp-datetime/blob/develop/src/DateTime.php
<?php ... public function get_local_date() { $wp_timezone = DateTimeZone::get_default(); /** * PHP BUG: DateTime::setTimezone(): Can only do this for zones with ID for now. * PHP version < 5.4.26 * PHP version > 5.5 < 5.5.10 * * @link https://bugs.php.net/bug.php?id=45543 * @link https://3v4l.org/mlZX7 */ if ( version_compare( PHP_VERSION, '5.4.26', '<' ) || ( version_compare( PHP_VERSION, '5.5', '>' ) && version_compare( PHP_VERSION, '5.5.10', '<' ) ) ) { return new DateTime( date( self::MYSQL, $this->get_wp_timestamp() ), $wp_timezone ); } $date = clone $this; $date->setTimezone( $wp_timezone ); return $date; } ...
I am not sure what you mean by "preventing formatting". If
date_i18n()
doesn't process timezone then output is incorrect sincedate()
will assume UTC.
We use a custom function that converts the timezone format characters:
https://github.com/pronamic/wp-datetime/blob/develop/src/DateTime.php
That way the 'timezone_string'
option is not used and we don't have to filter on 'pre_option_timezone_string'
.
Ontopic: I think implementing wp_timezone
is possible, but if plugin/theme developers start using this for DateTime::setTimezone
on PHP < 5.4.26
or > 5.5 < 5.5.10
it could result in PHP warnings if an offset timezone is used. But maybe you can use some of this information in your wp_date
work.
#14
@
6 years ago
Warnings about undefined value $format
(I assume that should be $req_format
?) and multiple core unit tests failing there.
#15
@
6 years ago
- Keywords needs-unit-tests added
This gets test to pass! Though I know for sure we are still missing tests, I am getting to #25768 to deal with escaping issues and get test for that in place.
Would you mind to hold of on date_i18n()
rewrite a bit? I would prefer if we focus this ticket on getting wp_timezone()
in with tests for it. Then build from there.
#16
@
6 years ago
If proposed core bump to PHP 5.6 happens a lot of problems here simply disappear. I am of mind to wait and see if that happens, because it makes major difference to what we can use out of PHP's DateTime, including bunch of important additions that happened across those versions. Parsing numeric time zones especially in context of this ticket.
#17
@
5 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
Added patch introducing two new functions:
wp_timezone_string()
unified retrieval of time zone as string, straightforward upgrade fromget_option('timezone_string')
wp_timezone()
unified retrieval of timezone as object
Includes unit tests for all numeric offsets currently used by core.
#18
@
5 years ago
- Milestone changed from Awaiting Review to 5.3
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#19
@
5 years ago
Can we change this line in the wp_timezone_string()
function:
$minutes = abs( ( $offset - (int) $offset ) * 60 );
This bit is exploiting string vs. int type-casting rounding magic to be multiplied by the remainder.
I think I’d prefer this code to parse the option value as a string directly, or at least contain some inline docs to explain why this way is the best/fastest.
Subtracting a variable from itself reads like a copy/paste coding error, even with the type-cast as a hint that it isn’t an exact-exact subtraction.
Lastly, instead of 60
the WordPress constant MINUTE_IN_SECONDS
should probably be used (unless this is minutes in an hour, which I suppose it probably is, and there is no constant for.)
#20
@
5 years ago
This bit is exploiting string vs. int type-casting rounding magic to be multiplied by the remainder.
There is no string involved here, gmt_offset
is a float value.
I think I’d prefer this code to parse the option value as a string directly
I don't follow your suggestion, would you mind expressing it in code? As above this isn't a string in first place.
unless this is minutes in an hour, which I suppose it probably is, and there is no constant for
It is hour in minutes, yes.
#21
@
5 years ago
There is no string involved here, gmt_offset is a float value.
gmt_offset
is only a float when wp_timezone_override_offset()
finds a matching timezone_string
value that can be successfully opened via timezone_open()
.
When it does not find a match, or when using UTC offset values (like +07:30
), it will be returned as a string (even sanitize_option()
calls preg_replace()
on the value, expecting it to be a string.)
I don't follow your suggestion, would you mind expressing it in code?
// Prepare $offset = (float) get_option( 'gmt_offset' ); $hours = floor( $offset ); $minutes = ( $offset - (int) $hours ); // Calculate $sign = ( $offset < 0 ) ? '-' : '+'; $abs_hour = abs( $hours ); $abs_mins = abs( $minutes * 60 ); $timezone = sprintf( '%s%02d:%02d', $sign, $abs_hour, $abs_mins ); return $timezone;
Recommend splitting code into 2 chunks, to improve code understandability and avoid variable reuse on $offset
.
Recommend floor()
over (int)
because casting with (int)
here expects others to think gmt_offset
is always a float
, and know the int
of a float
is never rounded up.
Recommend moving abs()
calls out ofsprintf()
so the most important math is less hidden & nested.
#22
@
5 years ago
When it does not find a match, or when using UTC offset values (like +07:30), it will be returned as a string
I forgot again, but I stuffed string case into unit tests before anyway so it works... :)
Recommend floor() over (int)
Floor doesn't work in the way we need on negative values.
Otherwise patch refreshed with your suggestion.
#23
@
5 years ago
@Rarst Please do not remove/replace patches which already got feedback. They should be kept for history.
#24
follow-up:
↓ 25
@
5 years ago
Gotcha. I tend to have links to patches elsewhere (e.g. we have ready for merge queue in slack for Date/Time). Should probably adjust that workflow a bit then...
#25
in reply to:
↑ 24
@
5 years ago
Replying to Rarst:
Gotcha. I tend to have links to patches elsewhere (e.g. we have ready for merge queue in slack for Date/Time). Should probably adjust that workflow a bit then...
Just linking to tickets should be enough, assuming the latest patch is ready for review or commit, unless indicated otherwise in the comments.
Add a wp_get_timezone() function