Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#44985 closed enhancement (wontfix)

timezone_string should be valid when offset time zone mode is used

Reported by: rarst's profile Rarst Owned by:
Milestone: 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 (10)

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


6 years ago

#2 follow-up: @JPry
6 years 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 years 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 years 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
6 years ago

  • Version trunk deleted

#6 @nielsdeblaauw
6 years 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 6 years ago by nielsdeblaauw (previous) (diff)

#7 @Rarst
6 years 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
6 years 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.

#9 @Rarst
5 years ago

I gave this some more thought and introducing unexpected return format to a very established code path feels risky to me.

Instead I am of mind to introduce wp_timezone_string() (in addition to wp_timezone() returning object) as explicit opt-in upgrade path for existing code.

#10 @Rarst
5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing in favor of new API functions in #24730

Note: See TracTickets for help on using tickets.