Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Time + Duration misbehaves on DST boundary

#2066

adding a Duration to a Time misbehaves, when the calculation crosses a DST boundary and the Duration is longer than 24 hours.

Essentially, it seems that 24.hour are treated as a full day, which gives incorrect (or at least unexpected) results if a day is only 23 hours long.


>> ENV["TZ"] = "CET"
=> "CET"
>> Time.local(2009,3,29,0,0,0) # just before the change to DST
=> Sun Mar 29 00:00:00 +0100 2009
>> Time.local(2009,3,29,0,0,0) + 1.day
=> Mon Mar 30 00:00:00 +0200 2009 # midnight the next day, correct and as expected
>> Time.local(2009,3,29,0,0,0) + 24.hours
=> Mon Mar 30 00:00:00 +0200 2009 # => WRONG, only 23 hours have passed because of DST switchover
>> Time.local(2009,3,29,0,0,0) + 24.hours.to_i
=> Mon Mar 30 01:00:00 +0200 2009 # expected result

If the duration added is less than 24 hours the results are correct:


>> Time.local(2009,3,29,0,0,0) + 23.hours
=> Mon Mar 30 00:00:00 +0200 2009 # correct
>> Time.local(2009,3,29,0,0,0) + 22.hours
=> Sun Mar 29 23:00:00 +0200 2009 # also correct

Using minutes instead of hours makes no difference


>> Time.local(2009,3,29,0,0,0) + (23 * 60 + 1).minutes # less than 24 hours
=> Mon Mar 30 00:01:00 +0200 2009 # correct
>> Time.local(2009,3,29,0,0,0) + (24 * 60 + 1).minutes
=> Mon Mar 30 00:01:00 +0200 2009 # WRONG

this shows that the problem occurs on Durations of more than 24 hours:


>> Time.local(2009,3,28,23,0,0) + 23.hours
=> Sun Mar 29 23:00:00 +0200 2009 # correct
>> Time.local(2009,3,28,23,0,0) + 24.hours
=> Sun Mar 29 23:00:00 +0200 2009 # wrong

Reported by Levin Alexander · February 25th, 2009 @ 05:26 AM

State: resolved
Milestone: 3.0.2
Assigned to: Geoff Buesing Geoff Buesing
Importance: Low

Activity

  1. Levin Alexander
    Levin Alexander

    ActiveSupport::TimeWithTimezone behaves as expected:

    
>> Time.local(2009,3,29,0,0,0).in_time_zone("CET") + 24.hours
=> Mon, 30 Mar 2009 01:00:00 CEST +02:00 # correct

    February 25th, 2009 @ 06:09 AM

  2. Levin Alexander
    Levin Alexander

    There are unit tests in time_ext_test.rb that call for the incorrect behavior:

    
  def test_daylight_savings_time_crossings_forward_start
    with_env_tz 'US/Eastern' do
      # st: US: 2005 April 2nd 7:27pm
      assert_equal Time.local(2005,4,3,19,27,0), Time.local(2005,4,2,19,27,0).since(86400), 'st+1.day=>dt'
      assert_equal Time.local(2005,4,4,19,27,0), Time.local(2005,4,3,19,27,0).since(86400), 'dt+1.day=>dt'
    end

    Is Time#since really supposed to behave this way?

    February 25th, 2009 @ 06:36 AM

  3. Geoff Buesing
    Geoff Buesing
    • State changed from new to open
    • Milestone cleared.

    I agree, this is wonky behavior. Reason it exists at all is because, in the days before Rails had a proper Duration class (i.e., 2006), this was a way to fake a duration of 1 day and have it cross a DST boundary correctly.

    Now that we have Duration, it's not needed, it's confusing, and needs to go. Adding 24.hours should always advance the time exactly 24 hours, and adding 1.day should advance to the same time on the next day, which will be 23-25 hours later, depending upon if a DST boundary was crossed.

    A patch for this would be welcome.

    February 25th, 2009 @ 03:27 PM

  4. Michael Curtis
    Michael Curtis
    • Tag changed from active_support, duration, timezone to active_support, duration, patch, timezone

    This should take care of it. I've updated the unit tests in time_ext_test.rb that were enforcing the incorrect behavior, and added a couple tests to duration_test.rb that test this specific case.

    This is my first patch to rails.. if I did anything wrong please let me know!

    March 10th, 2009 @ 02:54 AM

  5. Manfred Stienstra
    Manfred Stienstra

    Michael, can you remove the commented lines? Also, the f variable doesn't seem necessary.

    March 10th, 2009 @ 04:40 PM

  6. Pratik
    Pratik
    • Assigned user set to Geoff Buesing

    March 10th, 2009 @ 04:55 PM

  7. Geoff Buesing
    Geoff Buesing

    Also, no longer a need to go through Numeric#since -- i.e. seconds.since(self) -- we should now be able to call Time#+ directly, i.e. self + seconds.

    Time#since effectively becomes "plus with datetime fallback".

    March 10th, 2009 @ 05:02 PM

  8. Michael Curtis
    Michael Curtis
    • Assigned user cleared.

    Here's a new patch, reduced to one commit and no longer going through Numeric#since.

    March 10th, 2009 @ 05:19 PM

  9. Michael Curtis
    Michael Curtis
    • Assigned user set to Geoff Buesing

    March 10th, 2009 @ 05:19 PM

  10. Geoff Buesing
    Geoff Buesing

    Revised patch looks good. I'll pull this in after 2.3 final is pushed. Thanks!

    March 11th, 2009 @ 02:30 AM

  11. Repository
    Repository
    • State changed from open to resolved

    (from [5a8b481f717470b952ac7eb890f260ea98428153]) Time.local instances: Adding 24.hours across the DST boundary adds 24 hours instead of one day [#2066 Time + Duration misbehaves on DST boundary state:resolved] http://github.com/rails/rails/co...

    March 29th, 2009 @ 10:47 PM

  12. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2
    • Importance changed from to Low

    October 15th, 2010 @ 11:01 PM