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.

Assigning strings with timezone information to timezone aware attributes strips out timezone info

#105

For example, say you have a model User with a column banned_at: 

@user.banned_at = Time.zone.now   # => Sat, 03 May 2008 21:55:45 PDT -07:00
time_string = @user.banned_at.to_s   # => "2008-05-03 21:55:45 -0700"
@user.banned_at = time_string # => Sat, 03 May 2008 21:55:45 UTC +00:00
time_string = @user.banned_at.to_s   # => "2008-05-03 21:55:45 UTC"

Patch is attached.

Reported by Scott Fleckenstein · May 4th, 2008 @ 06:01 AM

State: resolved
Milestone: none
Assigned to: Geoff Buesing Geoff Buesing
Importance: none

Activity

  1. Dan Barry
    Dan Barry

    These tests only pass if your computer's clock is set to the same time zone as Time.zone is set to in the test. Change your computer's clock to any other time zone and the tests fail.

    May 5th, 2008 @ 04:53 PM

  2. Scott Fleckenstein
    Scott Fleckenstein

    LOL. My bad. I'll work on fixes to them :)

    May 5th, 2008 @ 05:02 PM

  3. Dan Barry
    Dan Barry

    I've attached a new patch that works when the machine's clock is set to a different time zone than Time.zone.

    May 6th, 2008 @ 12:00 AM

  4. Scott Fleckenstein
    Scott Fleckenstein

    Thanks Dan!

    I was going to work on fixing the patch tonight but I see that you up and solved the problems without me. :)

    May 6th, 2008 @ 12:20 AM

  5. Dan Barry
    Dan Barry

    I noticed a problem with DST due to TimeZone not knowing which period it is in. This has been fixed in the new patch (as well as the tests using time zones from both hemispheres so that there is always something that must be adjusted).

    May 6th, 2008 @ 02:57 AM

  6. Kyle Hargraves
    Kyle Hargraves

    The tests are now consistently passing for me no matter my system date / time zone / DST value. +1, this is a handy feature.

    May 6th, 2008 @ 03:21 AM

  7. Kevin Glowacz
    Kevin Glowacz

    I've been having trouble with time zones in my project. This helps. +1

    May 6th, 2008 @ 03:42 PM

  8. Dan Barry
    Dan Barry

    I just realized that strings without specified time zones are interpreted as being in UTC. While one could theoretically just use Time.zone.parse, it defeats the purpose of having all of this happen automatically.

    String#to_time should always interpret 'May 5th 2:00pm' as being 2pm in the user's time zone, not UTC. Unless someone can come up with a good reason that this shouldn't be the case, I'll add this as well.

    May 6th, 2008 @ 04:30 PM

  9. Dan Barry
    Dan Barry

    The newly attached patch interprets strings without time zone information as being in the current time zone.

    May 6th, 2008 @ 05:50 PM

  10. Geoff Buesing
    Geoff Buesing
    • Assigned user set to Geoff Buesing

    The time zone enhancement to String#to_time does make sense, but it might break apps that rely on the existing behavior (i.e., ignore any time zone info in the string), so I'm hesitant to change it.

    Fortunately, the stated goal of this patch -- to make ActiveRecord time zone aware attributes work with time zone information in a supplied string -- can be achieved without changing String#to_time. We'd just need to make Time.zone.parse respect time zone info in the string (as has been fixed in the existing patch) and change the AR attribute writer method to call Time.zone.parse whenever a string is passed in -- I'd be happy to pull those changes in.

    Let me know if you want to update this patch, or if you want me to take it from here. Thanks!

    May 7th, 2008 @ 07:52 AM

  11. Scott Fleckenstein
    Scott Fleckenstein

    time_zone_parse.diff is attached, a new patch that works as Geoff advertised, replacing #to_time with Time.zone.parse.

    Thanks for the advice.

    May 7th, 2008 @ 03:02 PM

  12. Dan Barry
    Dan Barry

    Geoff,

    While I'm not sure whether someone would expect a string without an explicit time zone to be interpreted as UTC or the time zone set in Time.zone, I think that you'd expect a string with a time zone specified to be interpreted as being in that time zone.

    I feel that not parsing the time zone given in a string leads to an unnecessary violation of the principle of least surprise: time.to_s.to_time will be different from the original time. It's not even the same time in a different time zone; it's the wrong time.

    Is backwards compatibility worth arguably broken behavior?

    May 7th, 2008 @ 08:42 PM

  13. Geoff Buesing
    Geoff Buesing

    @Scott: thanks for the updated patch; I'll try to pull this in in the next couple of days.

    @Dan: String#to_time has ignored the time zone since it was first added in Rails 0.14, over three years ago, and there have been no attempts to fix this behavior since, which is why I'm hesitant to change it now without a compelling use case.

    If you like, please open a separate ticket about this particular issue -- if there's community enthusiasm for this, and no significant downsides that anyone can think of, then we're probably ok to go ahead with this change.

    May 8th, 2008 @ 05:19 AM

  14. Geoff Buesing
    Geoff Buesing
    • State changed from new to resolved

    May 9th, 2008 @ 03:49 AM