This project is archived and is in readonly mode.

#105 ✓resolved
Scott Fleckenstein

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

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

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.

Comments and changes to this ticket

  • Dan Barry

    Dan Barry May 5th, 2008 @ 04:53 PM

    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.

  • Scott Fleckenstein

    Scott Fleckenstein May 5th, 2008 @ 05:02 PM

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

  • Dan Barry

    Dan Barry May 6th, 2008 @ 12:00 AM

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

  • Scott Fleckenstein

    Scott Fleckenstein May 6th, 2008 @ 12:20 AM

    Thanks Dan!

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

  • Dan Barry

    Dan Barry May 6th, 2008 @ 02:57 AM

    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).

  • Kyle Hargraves

    Kyle Hargraves May 6th, 2008 @ 03:21 AM

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

  • Kevin Glowacz

    Kevin Glowacz May 6th, 2008 @ 03:42 PM

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

  • Dan Barry

    Dan Barry May 6th, 2008 @ 04:30 PM

    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.

  • Dan Barry

    Dan Barry May 6th, 2008 @ 05:50 PM

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

  • Geoff Buesing

    Geoff Buesing May 7th, 2008 @ 07:52 AM

    • 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!

  • Scott Fleckenstein

    Scott Fleckenstein May 7th, 2008 @ 03:02 PM

    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.

  • Dan Barry

    Dan Barry May 7th, 2008 @ 08:42 PM

    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?

  • Geoff Buesing

    Geoff Buesing May 8th, 2008 @ 05:19 AM

    @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.

  • Geoff Buesing

    Geoff Buesing May 9th, 2008 @ 03:49 AM

    • State changed from “new” to “resolved”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages