WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 7 days ago

#44985 new enhancement

timezone_string should be valid when offset time zone mode is used

Reported by: Rarst Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Date/Time Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

timezone_string time zone settings mode is backwards compatible, overriding gmt_offset mode with correct offset:

// Kiev
var_dump( get_option( 'gmt_offset' ), get_option( 'timezone_string' ) );
// float(3) string(11) "Europe/Kiev"

However the reverse isn't true, if settings are in offset mode the time zone string is empty and invalid:

// UTC+3
var_dump( get_option( 'gmt_offset' ), get_option( 'timezone_string' ) );
// string(1) "3" string(0) ""

In part this is because on older PHP versions there is no valid string that would correspond to an hours offset. But from PHP 5.5.10 it would work with timezone strings in +03:00 format just fine.

I think generating valid time zone strings in offset mode for new PHP versions is low hanging and makes things much more robust for downstream developers, making it harder for them to unknowingly screw up by relying on timezone_string alone.

We still need unified timezone retrieval in #24730, but that doesn't displace making timezone_string by itself more reliable.

Will work on a patch.

Change History (8)

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


6 months ago

#2 follow-up: @JPry
6 months ago

@Rarst

I've worked on this recently in a plugin with the idea of fully allowing for any GMT offset or timezone string. If it helps, there are a few challenges that you'll find with this:

  1. Not all GMT offsets map to a timezone string
  2. Some (most?) GMT offsets map to more than one timezone string
  3. Beginning with PHP 7, the timezone strings that can be determined with the timezone_name_from_abbr() and timezone_abbreviations_list() functions has changed. You'll get different results for PHP < 7.0 and PHP >= 7.0.

Hopefully that helps save you a few headaches. 🙂

#3 in reply to: ↑ 2 ; follow-up: @Rarst
6 months ago

Replying to JPry:

We are not doing any mapping for this one. It is impossible to reliably inflate the data from less meaningful to more meaningful.

I am merely talking about making timezone_string return +NN:NN format offsets on newer PHP versions, which can use that as valid input, just as any geographical timezone string.

#4 in reply to: ↑ 3 @JPry
6 months ago

Replying to Rarst:

I am merely talking about making timezone_string return +NN:NN format offsets on newer PHP versions, which can use that as valid input, just as any geographical timezone string.

Ah, I misunderstood your description. Looking forward to seeing the enhancement.

#5 @pento
2 months ago

  • Version trunk deleted

#6 @nielsdeblaauw
12 days ago

I've implemented a naive implementation for this, but ran into some warnings that use the timezone_string and can't handle the +HH:MM format.

Notice: date_default_timezone_set(): Timezone ID '+02:00' is invalid in /srv/www/wordpress-trunk/public_html/build/wp-admin/options-general.php on line 264

My incomplete implementation was:

add_filter( 'option_timezone_string', 'wp_timezone_override_string' );
function wp_timezone_override_string($string_offset) {
	if(!empty($string_offset)){
		return $string_offset;
	}
	remove_filter( 'pre_option_gmt_offset', 'wp_timezone_override_offset' ); // Remove filter to prevent infinite loop.
	if ( ! $gmt_offset = get_option( 'gmt_offset' ) ) {
		return $string_offset;
	}
	$offset = gmdate('H:i', floor(abs($gmt_offset) * HOUR_IN_SECONDS));
	$prefix = '+';
	if($gmt_offset < 0){
		$prefix = '-';
	}
	return $prefix . $offset;
}
Last edited 12 days ago by nielsdeblaauw (previous) (diff)

#7 @Rarst
7 days ago

I got conversion logic down with unit tests and everything in https://github.com/Rarst/wpdatetime/blob/master/src/WpDateTimeZone.php

Yeah, those IDs don't work with date_default_timezone_set() but empty string case also doesn't work with it. And if anyone is calling date_default_timezone_set() in WP context they are "doing it wrong". In core that case works because it explicitly checks for empty.

That said we should get rid of that date_default_timezone_set() altogether there. It's one of those really really old pieces of Date/Time. I'll make a ticket for that.

#8 @Rarst
7 days ago

Oh and also after PHP 5.6 requirement bump we can just do this unconditionally, so I am for adding it to the waiting pile for that.

Note: See TracTickets for help on using tickets.