Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Time object's offset gets screwed up in certain circumstances #6651

Closed
bblack opened this Issue · 5 comments

4 participants

Brian Black Andrew White Elliot Winkler Rafael Mendonça França
Brian Black

Ruby's Time#getlocal takes a UTC offset and returns a time object with an adjusted UTC offset (but no named zone). Such objects have funny behavior in the following circumstances when interacting with ActiveSupport functionality

+ method:

> Time.parse("2012-01-01T00:00Z").getlocal("-08:00") # normal
=> 2011-12-31 16:00:00 -0800
> Time.parse("2012-01-01T00:00Z").getlocal("-08:00") + 0 # normal
=> 2011-12-31 16:00:00 -0800
> Time.parse("2012-01-01T00:00Z").getlocal("-08:00") + 0.days # wtf
=> 2011-12-31 16:00:00 -0500

change method:

> Time.parse("2012-01-01T00:00Z").getlocal("-08:00") # normal
=> 2011-12-31 16:00:00 -0800
> Time.parse("2012-01-01T00:00Z").getlocal("-08:00").change(:min => 59) # wtf
=> 2011-12-31 16:59:00 -0500
Andrew White pixeltrix was assigned
Andrew White
Owner

This is another manifestation of #4847.

Andrew White
Owner

Basically change is broken because of the need to support 1.8 - we don't need to do that anymore.

Elliot Winkler

So I think there's at least four methods where this bug manifests:

  • Time#+ with a Duration
  • Time#- with a Duration
  • Time#advance
  • Time#change
  • Anything else that depends on #advance or #change

I'm going to just look at Time#+ for now because I have a suspicion that it does come down to Time#change but I want to be sure. So here's how that works:

  • This is actually Time#plus_with_duration (dur = …; t.plus_with_duration(dur))
  • Since a Duration is given, Duration#since is called with our Time (dur.since(t))
  • Duration#since calls Duration#sum with our Time (dur.send(:sum, 1, t))
  • Duration#sum creates another Time by starting with the given time, taking the #parts of the duration, and operating on the time
  • Let's assume that our Duration has only one part (therefore there is only one iteration)
  • If the part type is :seconds, Time#since is called with 1 * number of seconds (dur = 5.seconds; t.since(1 * 5))
    • Time#since just adds the given seconds to the time
    • Now we're back to Time#plus_with_duration, except since the given argument is not a Duration, we use Ruby's Time#+ method, so we end up with a time advanced by the given seconds. (t + 5)
  • If the part type is not :seconds, Time#advance is called with {type => 1 * number} (dur = 5.days; t.advance(:days => 1 * 5))
    • Time#advance first takes the given options and determines the change in terms of :weeks, :days and :hours
    • It then converts the given time to a date, calls Date#advance with the determined options to make a temporary Date object…
    • …then uses the month, day, and year of the Date to make a new Time by calling Time#change on the given time (t.change(:year => …, :month => …, :day => …))
    • If :seconds, :minutes, or :hours was passed into Time#advance, then we call Time#since on the new Time to advance it further. (This will not happen here because Durations either produce :seconds, :days, :months, or :years options.)
    • Otherwise, return the time.

Let's take some cases:

  • t + 1.second
    • t.plus_with_duration(1.second)
    • 1.second.since(t)
    • 1.second.parts => [[:seconds, 1]]
    • t.since(1 * 5)
    • t + 5
  • t + 1.hour
    • t.plus_with_duration(1.hour)
    • 1.hour.since(t)
    • 1.hour.parts => [[:seconds, 3600]]
    • t.since(1 * 3600)
  • t + 1.day
    • t.plus_with_duration(1.day)
    • 1.day.since(t)
    • 1.day.parts => [[:days, 1]]
    • t.advance(:days => 1 * 1)
    • d = t.to_date.advance(:days => 1, :hours => 0)
    • If we have t = Time.new(2012, 1, 1, 0, 0, 0, "+02:00"), then t.change(:year => 2012, :month => 1, :day => 2)
    • No :seconds, :minutes, or :hours options were given so that's the Time that gets returned.

So it does come down to Time#change:

>> t = Time.new(2012, 1, 1, 0, 0, 0, "+02:00")
=> 2012-01-01 00:00:00 +0200
>> t.change(:year => 2012, :month => 1, :day => 2)
=> 2012-01-02 00:00:00 -0700
>> Time.local(2012, 1, 2)
=> 2012-01-02 00:00:00 -0700
Elliot Winkler

And Time#change does this:

  • If this time is a UTC time, call Time.utc_time with year, month, day, hour, minute, second, and microsecond values.
  • Otherwise, call Time.local_time.
  • Both Time.utc_time and Time.local_time are also provided by ActiveSupport
  • These are just passthroughs to Time.time_with_datetime_fallback, which gets called with one of :utc or :local, and the time args
  • Time.time_with_datetime_fallback tries to call Time.local or Time.utc with the time args, and if that fails, calls DateTime.civil_from_format with one of :utc or :local
  • DateTime.civil_from_format is yet another AS-provided method which wraps DateTime.civil.

So it seems to come down to either Time.time_with_datetime_fallback, or DateTime.civil_from_format. I'm not sure why either of these exist. If you want to build a time in a time zone, it seems like you should be able to just do one of:

  • Time.local(year, month, day, hour, min, sec, usec, tz)
  • Time.utc(year, month, day, hour, min, sec, usec, tz)

and boom, you're done.

@pixeltrix you mentioned there's some 1.8-compat stuff here, what is that?

Andrew White pixeltrix referenced this issue from a commit
Andrew White pixeltrix Make Time#change work with offsets other than UTC or local
Use Time.new to create times where the current offset is not zero or
not in the local time zone - closes #4847 and #6651.
98b46bf
Rafael Mendonça França
Owner

Closed by 98b46bf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.