Skip to content
This repository

TimeZone.parse should use zone information in the input string, when available #5770

Closed
shyam-habarakada opened this Issue April 07, 2012 · 7 comments

5 participants

shyam-habarakada Steve Klabnik Andrew White Jamie Gaskins Richard Schneeman
shyam-habarakada

On the server where system timezone is say, CDT,

Time.zone.parse("Mon May 28 2012 00:00:00 GMT-0700 (PDT)")
 => Mon, 28 May 2012 05:00:00 UTC +00:00 

whereas,

"Mon May 28 2012 00:00:00 GMT-0700 (PDT)".to_datetime
 => Tue, 29 May 2012 00:00:00 -0700 

.. produces the correct value.

Is there a good reason why TimeZone.parse is ignoring time-zone information available in the input string? Shouldn't it behave the same as if we had use the string.to_datetime method?

The current implementation suggests that a caller should set Time.zone to the correct value first, before invoking parse, but this seems like an unnecessary step when the information needed is available in the input string itself.

Additional information

The date format is used by chrome, safari and mozilla when serializing a javascript Date object to string. IE(9) ues a different format like Mon May 28 00:00:00 PDT 2012 which is correctly handled by the default Time.zone.parse implementation.

See original issue posted on stack-overflow here http://stackoverflow.com/questions/10056025/rails-time-zone-parsing-doesnt-work-as-expected

shyam-habarakada

I tried the following patch, and it seemed to work as I expect -- are there any issues with taking this in as a fix?

# override the parse method to prefer String.to_time
class ActiveSupport::TimeZone
  alias :original_parse :parse
  def parse(str, now=now)
    begin
      str.to_time
    rescue
      original_parse(str, now)
    end 
  end
end
Jamie Gaskins

An ActiveSupport::TimeWithZone object isn't a DateTime object and shouldn't behave like one. TimeWithZone objects normalize their timestamps to the current Time.zone value specifically for web apps to display times properly when they aren't provided for the same time zone, which is very useful for when you may not have permission to set the system clock in your production environment (such as in shared or cloud hosting).

An example of when this would be useful would be for an API called from various apps that supplies timestamps. You would want all of these timestamps displayed with the same time zone in your app instead of confusing your users with explicit time-zones in the timestamps or displaying 04:00 after 05:00.

If you're using ActiveSupport::TimeWithZone, you should be setting the time zone if you want something specific. If you want to stick with parsed times, feel free to stick with the Time or DateTime classes.

A sensible default if you don't specifically set it is UTC. Rails currently defaults it to that, but the TimeWithZone class itself has no default. Until you set it, Time.zone returns nil (and Time.zone.parse returns a Time rather than TimeWithZone). The only change I would propose here would be to default AS::TWZ to use UTC, but its fallback to Time objects (using system TZ) is perfectly valid, too.

Richard Schneeman
Collaborator

I personally would abolish timezones if i ever became czar of the world, because I hate them so much. So my opinions on Time Zone issues cannot be trusted. Sounds like @jgaskins is saying this isn't a bug, and should be closed.

Steve Klabnik steveklabnik closed this November 30, 2012
Steve Klabnik
Collaborator

I think that's true as well. @pixeltrix if you disagree please re-open.

Andrew White pixeltrix reopened this November 30, 2012
Andrew White
Owner

There is a bug here - Time.zone.parse will return a time in whatever zone the application is configured with (UTC is the Rails default), so the fact it's UTC isn't the problem. The problem is that Time.parse isn't understanding GMT-0700 so sets the offset to 0 and we end up with the incorrect time. We can clean this up now because we don't need the rescue for Time.parse and Date._parse returns the correct offset, e.g:

>> Date._parse("Mon May 28 2012 00:00:00 GMT-0700 (PDT)")
=> {:wday=>1, :zone=>"GMT-0700", :hour=>0, :min=>0, :sec=>0, :year=>2012, :mon=>5, :mday=>28, :offset=>-25200}
Andrew White pixeltrix closed this issue from a commit December 01, 2012
Andrew White Make `Time.zone.parse` to work with JavaScript date strings
Chrome, Safari and Firefox serialize Date objects to strings such
as 'Mon May 28 2012 00:00:00 GMT-0700 (PDT)'. When these strings
are parsed the zone is interpreted as 'GMT-0700' which doesn't
exist in the TzInfo list of timezones.

By taking advantage of the improved date/time handling in 1.9.3
we can use `Date._parse` and the `:offset` value which is parsed
correctly.

Three tests were amended to make them pass:

1.  test_parse_with_old_date

    This needed changing to a different value because the original
    value was before EST was adopted so was being changed to a
    LMT (Local Mean Time) value after the change. It didn't before
    because `DateTime` just has offsets from UTC not timezones.

2.  test_parse_should_not_black_out_system_timezone_dst_jump

    Changed the implementation of this test as the stubs were
    dependent on internal implementation details of the test.
    Confirmed that the modified test still failed when the
    implementation of `parse` was restored to pre-#5571.

3.  test_parse_should_black_out_app_timezone_dst_jump

    Ditto.

Closes #5770.
005d910
Andrew White pixeltrix closed this in 005d910 December 01, 2012
shyam-habarakada

Thanks for fixing that.

Richard Schneeman
Collaborator

:heart: I missed that. Thanks for jumping on that.

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.