WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 weeks ago

#24730 new enhancement

Introduce a timezone-retrieval method

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

24730.diff (983 bytes) - added by rmccue 5 years ago.
Add a wp_get_timezone() function
wp-timezone-simple-date-i18n.diff (4.8 KB) - added by remcotolsma 3 weeks ago.
Introduced wp_timezone() function, simplified date_i18n() function.
wp-timezone-simple-date-i18n-v2.diff (4.8 KB) - added by remcotolsma 3 weeks ago.
Retry: Introduced wp_timezone() function, simplified date_i18n() function.

Download all attachments as: .zip

Change History (18)

@rmccue
5 years ago

Add a wp_get_timezone() function

#1 @johnbillion
5 years ago

  • Cc johnbillion added

#2 @mordauk
5 years ago

  • Cc pippin@… added

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


3 years ago

#4 @rmccue
3 years ago

Related: #24730

#5 @Rarst
4 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
4 weeks 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
4 weeks 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
3 weeks 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 @remcotolsma
3 weeks 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 @Rarst
3 weeks 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
3 weeks 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 3 weeks ago by remcotolsma (previous) (diff)

#12 @Rarst
3 weeks 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
3 weeks 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
3 weeks ago

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

#14 @Rarst
3 weeks ago

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

@remcotolsma
3 weeks ago

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

#15 @Rarst
3 weeks 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.

Note: See TracTickets for help on using tickets.