WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 4 weeks ago

#24730 reviewing enhancement

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:

  1. Filter timezone_string and work out the correct string - This will likely break the admin interface for it.
  2. Introduce a new function to get the timezone from timezone_string, and fall back to gmt_offset if needed.

Patch has implementation of the second option (also using DateTimeZone), can be adapted as necessary for the first.

Attachments (4)

24730.diff (983 bytes) - added by rmccue 6 years ago.
Add a wp_get_timezone() function
wp-timezone-simple-date-i18n.diff (4.8 KB) - added by remcotolsma 12 months ago.
Introduced wp_timezone() function, simplified date_i18n() function.
wp-timezone-simple-date-i18n-v2.diff (4.8 KB) - added by remcotolsma 12 months ago.
Retry: Introduced wp_timezone() function, simplified date_i18n() function.
wp-timezone.patch (4.2 KB) - added by Rarst 4 weeks ago.

Download all attachments as: .zip

Change History (29)

@rmccue
6 years ago

Add a wp_get_timezone() function

#1 @johnbillion
6 years ago

  • Cc johnbillion added

#2 @mordauk
6 years ago

  • Cc pippin@… added

This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.


4 years ago

#4 @rmccue
4 years ago

Related: #24730

#5 @Rarst
16 months 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 @remcotolsma
12 months 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 @Rarst
12 months 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 @Rarst
12 months 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).

#10 @Rarst
12 months 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 @remcotolsma
12 months 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:

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;
}
Last edited 12 months ago by remcotolsma (previous) (diff)

#12 @Rarst
12 months 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 @remcotolsma
12 months 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 since date() 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.

@remcotolsma
12 months ago

Introduced wp_timezone() function, simplified date_i18n() function.

#14 @Rarst
12 months ago

Warnings about undefined value $format (I assume that should be $req_format?) and multiple core unit tests failing there.

@remcotolsma
12 months ago

Retry: Introduced wp_timezone() function, simplified date_i18n() function.

#15 @Rarst
12 months 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 @Rarst
6 months 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 @Rarst
4 weeks 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 from get_option('timezone_string')
  • wp_timezone() unified retrieval of timezone as object

Includes unit tests for all numeric offsets currently used by core.

#18 @SergeyBiryukov
4 weeks ago

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

#19 @johnjamesjacoby
4 weeks 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.)

Last edited 4 weeks ago by johnjamesjacoby (previous) (diff)

#20 @Rarst
4 weeks 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 @johnjamesjacoby
4 weeks 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 );
 	$tz_offset = sprintf( '%s%02d:%02d', $sign, $abs_hour, $abs_mins );

 	return $tz_offset;

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.

Last edited 4 weeks ago by johnjamesjacoby (previous) (diff)

#22 @Rarst
4 weeks 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.

@Rarst
4 weeks ago

#23 @ocean90
4 weeks ago

@Rarst Please do not remove/replace patches which already got feedback. They should be kept for history.

#24 follow-up: @Rarst
4 weeks 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 @SergeyBiryukov
4 weeks 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.

Note: See TracTickets for help on using tickets.