Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TZInfo with ActiveSupport - Samoa Timezone error #66

Closed
pedrofurtado opened this issue Mar 6, 2017 · 3 comments
Closed

TZInfo with ActiveSupport - Samoa Timezone error #66

pedrofurtado opened this issue Mar 6, 2017 · 3 comments
Labels
bug

Comments

@pedrofurtado
Copy link

@pedrofurtado pedrofurtado commented Mar 6, 2017

I am having the same issue as this user, that created this issue in repository of Rails: (rails/rails#27419)

Steps to reproduce

In rails console.

DateTime.now.in_time_zone("Samoa") Returns a date with a UTC offset of + 14:00
ActiveSupport::TimeZone.new('Samoa').utc_offset/60/60 Returns -11

Expected behavior

ActiveSupport::TimeZone.new('Samoa').utc_offset/60/60 Should return 14 or 13 based on daylight savings time

Actual behavior

ActiveSupport::TimeZone.new('Samoa').utc_offset/60/60 Returns -11

System configuration

Rails version: 5.0.2

Ruby version: 2.3.3

@philr
Copy link
Member

@philr philr commented Mar 7, 2017

It looks like there is a bug with the TZInfo zoneinfo data source that is causing the Pacific/Apia time zone (that Active Support calls Samoa) to be represented with a base offset of -11 hours and a daylight savings offset of either +24 or +25 hours (instead of a base offset of +13 hours and a daylight savings offset of either 0 or +1 hour):

require 'tzinfo'
TZInfo::DataSource.set(:zoneinfo)
period = TZInfo::Timezone.get('Pacific/Apia').period_for_utc(Time.utc(2017, 3, 6))
period.utc_offset / 3600       # => -11
period.std_offset / 3600       # =>  25 
period.utc_total_offset / 3600 # =>  14

The reason for the difference between DateTime.now.in_time_zone("Samoa") and ActiveSupport::TimeZone.new('Samoa').utc_offset/60/60 is that the former is showing the offset including daylight savings time (utc_total_offset) and the latter is returning just the base offset (utc_offset).

The expected behaviour is that ActiveSupport::TimeZone.new('Samoa').utc_offset/60/60 would always return 13 regardless of whether daylight savings time is being observed.

Pacific/Apia switched from one side of the International Date Line to the other in December 2011. I suspect that the code that attempts to derive the daylight savings offset from zoneinfo files is not handling this change correctly.

I'll look into fixing the bug. In the mean time, you can work around the problem by using the tzinfo-data gem as TZInfo's data source instead of the system zoneinfo files.

require 'tzinfo'
TZInfo::DataSource.set(:ruby)
period = TZInfo::Timezone.get('Pacific/Apia').period_for_utc(Time.utc(2017, 3, 6))
period.utc_offset / 3600       # => 13
period.std_offset / 3600       # =>  1 
period.utc_total_offset / 3600 # => 14

To change the data source, you'll need to add a dependency on tzinfo-data to your Gemfile. By default, Rails ships with this dependency, but limits it to Windows and JRuby. You'll either need to remove the platforms: option from the existing gem 'tzinfo-data' line if it is there, or add gem 'tzinfo-data' to the Gemfile.

@philr philr added the bug label Mar 7, 2017
@philr philr closed this in 45dcc3d Mar 16, 2017
@pedrofurtado
Copy link
Author

@pedrofurtado pedrofurtado commented Mar 18, 2017

Thanks @philr !

philr added a commit that referenced this issue Mar 18, 2017
Resolves #66, correctly handling Pacific/Apia switching from one side of
the International Date Line to the other whilst observing daylight savings
time.

The previous algorithm only looked one transition forward and back and
always preferred the prior period to define the utc_offset of a DST
period. The new algorithm considers all transitions forward and back
until a non-DST period is found and defines utc_offset of a DST period
based on the smallest difference.

(cherry picked from commit 45dcc3d)
@philr
Copy link
Member

@philr philr commented Mar 25, 2017

The fix for this issue has now been released in version 1.2.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.