Make WordPress Core

Opened 10 years ago

Closed 6 years ago

#29205 closed enhancement (wontfix)

Remove manual UTC offsets that are not officially used

Reported by: nerrad's profile nerrad Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9.1
Component: Date/Time Keywords: has-patch
Focuses: administration Cc:

Description

In doing some UTC offset to timezone string conversions for a project I came across some interesting anomalies in the possible options for UTC offsets.

There is the UTC+0:30 option... which is really interesting because apparently -> http://en.wikipedia.org/wiki/UTC%2B00:30

So I guess we need to keep back compat for all those folks who used WordPress back in 1936. ;)

Anyways, there are three other UTC offset options that don't appear to be in the official list of UTC offsets:

  • 0:30
  • 6:30
  • 7:30

This definitely isn't a OMGBBQSAUCE type bug, but thought I'd bring it up nevertheless.

Attachments (2)

offset.txt (11.8 KB) - added by nerrad 10 years ago.
Dump of all offset values from php timezone_abbreviations_list()
29205.diff (1.1 KB) - added by ryanduff 9 years ago.

Download all attachments as: .zip

Change History (19)

#1 @nerrad
10 years ago

dang it... auto formatting messed up the UTC offset examples above. That should be:

  • -0:30
  • -6:30
  • -7:30

and as mentioned already (tongue in cheek) +:30

Last edited 10 years ago by nerrad (previous) (diff)

#2 follow-up: @Ipstenu
10 years ago

  • Keywords reporter-feedback added

https://cloudup.com/cdPGi4BCGna

I see all of those on 4.0 and 3.9.2 - Where are you looking that you're not seeing this?

#3 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
10 years ago

Replying to Ipstenu:

I see all of those on 4.0 and 3.9.2 - Where are you looking that you're not seeing this?

I think the suggestion is to actually remove them from the "Manual Offsets" section of wp_timezone_choice().

#4 in reply to: ↑ 3 @nerrad
10 years ago

  • Keywords reporter-feedback removed

Replying to SergeyBiryukov:

Replying to Ipstenu:

I see all of those on 4.0 and 3.9.2 - Where are you looking that you're not seeing this?

I think the suggestion is to actually remove them from the "Manual Offsets" section of wp_timezone_choice().

Exactly, sorry it wasn't clear in my original post. The manual UTC offset list has some invalid UTC offset options. I'm suggesting that they be removed.

#5 @nerrad
10 years ago

For reference: http://en.wikipedia.org/wiki/List_of_UTC_time_offsets

At the bottom of that Wikipedia article you'll notice the UTC offsets they list as historical or unofficial in italics.

Last edited 10 years ago by nerrad (previous) (diff)

#6 @nerrad
10 years ago

Here's another that should be removed:

Last edited 10 years ago by nerrad (previous) (diff)

#7 @helen
10 years ago

  • Summary changed from The "WordPress is really older than we think" UTC offset bug ticket. to Remove manual UTC offsets that are not officially used
  • Type changed from defect (bug) to enhancement
  • Version changed from trunk to 2.9.1

Added in [12507] for #11558, they were whole hours only before that.

I have no idea if somebody would actually be using one of these, but we'd need to know what would happen if they were and the option were removed.

#8 @nerrad
10 years ago

It's *possible* these were added to address potential DST since when a UTC offset is selected automatic DST is not in play. I'll look into and will post back.

#9 @nerrad
10 years ago

How I discovered this, is I needed to convert any user's UTC offset selections to a close equivalent timezone string. To do this I grabbed the cities timezones info array from php's timezone_abbreviations_list() and I looped through it, grabbing the 'offset' key and compared it with the selected UTC offset * 3600. I had some error control in place to detect if there wasn't a valid timezone string matching the given offset and that's where this surfaced.

I've attached a dump of all the offsets that are returned via timezone_abbreviations_list() and as you'll see, there are no equivalent timezone strings found for -:30, +:30, -6:30, -7:30, and -8:30 (there may be others but those are the definite ones I've found).

As for:

I have no idea if somebody would actually be using one of these, but we'd need to know what would happen if they were and the option were removed.

I think to mitigate that, we'd likely have to have some sort of check to see if any invalid offset is saved in user db and then just bump it up to the closest offset matching it.

Really tho, I suspect this has gone unnoticed so long precisely because no one IS using these offsets, why would they if they don't actually exist in the real world :).

I'm willing to do a patch for this if desired... I just didn't want to do it until I knew it had any traction cause I've easily worked around it for my use case.

Last edited 10 years ago by nerrad (previous) (diff)

@nerrad
10 years ago

Dump of all offset values from php timezone_abbreviations_list()

#10 @nerrad
10 years ago

another thought regarding how to handle users who have an invalid offset selected is to keep it in the db but when they visit the General Settings page, throw up a warning that their current offset is no longer valid and prompt to select a "correct" one or a timezone string. We could even preselect an offset closest to theirs (rounded up) in case they ignore the warning and just save.

#11 follow-up: @ryanduff
9 years ago

  • Keywords has-patch added

I've gone ahead and created a patch that trims the manual offsets down to the valid set. One of the issues I found with people setting invalid manual offsets is that if you're doing some DateTime things in PHP and passed it an invalid offset to convert to timezone, it throws a fatal error. So I'm certainly in the camp of only providing a valid set of options.

To address Helen's concerns over "what if someone is using an invalid setting" ... nothing would change immediately. The setting persists in the database. The point it would change would be when someone goes to Options > General then saves something on that page. This would overwrite their current invalid timezone setting in the database. With an invalid option, the select will default to the first value so I added an additional check to reset this to 'UTC+0'. Otherwise it was selecting Africa/Abidjan which was likely far more undesirable as a replacement than UTC+0

@ryanduff
9 years ago

#12 @pbearne
9 years ago

We should remove them all as they don't have time zone strings that are need in the code e.g. date_i18n()

#13 @pbearne
8 years ago

Added a patch in #36591 to hide them all if a timezone is selected. It would good to add this patch as well

#14 in reply to: ↑ 11 @Rarst
7 years ago

Replying to ryanduff:

I've gone ahead and created a patch that trims the manual offsets down to the valid set. One of the issues I found with people setting invalid manual offsets is that if you're doing some DateTime things in PHP and passed it an invalid offset to convert to timezone, it throws a fatal error. So I'm certainly in the camp of only providing a valid set of options.

Any more details on how you managed fatal? I was able to instance DateTimeZone from these "wrong" offsets just now without any issue.

More generally the main issue with numeric offsets in WP isn't that some of them are wrong, but that all of them are broken in how time zones are handled in localization. See #34835

#15 @nerrad
7 years ago

I'm guessing you instantiated DateTimeZone with a UTC offset in a PHP 5.6+ box? Earlier versions of PHP do not provide that option. Also there's a PHP bug (https://events.codebasehq.com/redirect?https://bugs.php.net/bug.php?id=73489) which affects DateTime instantiated with a DateTimeZone object using UTC offset.

#16 @Rarst
6 years ago

By itself I don't think these are worth messing with (and it doesn't look like we are dropping all offsets ever). However since we are trying to introduce unified time zone retrieval in #24730, if these explode that it would be very Not Good.

I am getting to writing unit tests for that, will see if these cause issues when it's done. If so I would probably be in favor of removing.

#17 @Rarst
6 years ago

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

I unit tested all offsets used by core for #24730 and there doesn't seem to be any technical issue.

Given that we don't really have visibility in how these might be used in the wild, I think messing with them is just asking for trouble and complicating the logic for no significant upside.

Also given that timezones are pretty impermanent and Murphy's law, I am guessing as soon as we remove them we would have to put them back in. :)

We can revisit if any technical problems surface and/or there is a change of circumstances with handling numeric offsets overall.

Note: See TracTickets for help on using tickets.