Use correct timezone when parsing date in json #23011

Merged
merged 1 commit into from Jun 25, 2016

Conversation

Projects
None yet
7 participants
@arnvald
Contributor

arnvald commented Jan 11, 2016

Fixes #22171

Time specified in ISO 8601 format without Z should be parsed as local time, yet until now it was treated as UTC.

This commit fixes problem by parsing time using timezone specified in application config.

The downside of this solution is performance hit (Time.zone.parse is ~ 1.6x slower than Time.parse), so maybe there's a better solution.

Additionally, YYYY-MM-DD format is parsed as Date not DateTime as it was until now (recommended by @pixeltrix #22171 (comment))

/cc @maclover7 @pixeltrix @chancancode

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Jan 11, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Jan 18, 2016

Member

@arnvald this looks good 👍

Can we add a CHANGELOG entry - thanks.

Member

pixeltrix commented Jan 18, 2016

@arnvald this looks good 👍

Can we add a CHANGELOG entry - thanks.

@arnvald

This comment has been minimized.

Show comment
Hide comment
@arnvald

arnvald Jan 21, 2016

Contributor

@pixeltrix thank you! I've rebased the branch and updated changelog

Contributor

arnvald commented Jan 21, 2016

@pixeltrix thank you! I've rebased the branch and updated changelog

+ Before such time was considered in UTC timezone, now, to comply with standard, it uses local timezone.
+
+ *Grzegorz Witek*
+

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 21, 2016

Member

@arnvald Newly added CHANGELOG should be added at the top of the file. Please move this entry to the top of CHANGELOG file.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 21, 2016

Member

@arnvald Newly added CHANGELOG should be added at the top of the file. Please move this entry to the top of CHANGELOG file.

activesupport/test/json/decoding_test.rb
@@ -70,12 +74,15 @@ def self.json_create(object)
%q(null) => nil
}
+

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 21, 2016

Member

Can you remove this line 😄

@prathamesh-sonpatki

prathamesh-sonpatki Jan 21, 2016

Member

Can you remove this line 😄

Use correct timezone when parsing date in json
Fixes #22171

Time specified in ISO 8601 format without `Z` should be considered as local
time, yet until now it was treated as UTC.

This commit fixes problem by parsing time using timezone specified in
application config.

The downside of this solution is performance hit (`Time.zone.parse` is ~ 1.6x
slower than `Time.parse`), so maybe there's a better solution.
@arnvald

This comment has been minimized.

Show comment
Hide comment
@arnvald

arnvald Jan 22, 2016

Contributor

@prathamesh-sonpatki updated, thanks for information! :)

Contributor

arnvald commented Jan 22, 2016

@prathamesh-sonpatki updated, thanks for information! :)

@chancancode chancancode merged commit a3ddd5f into rails:master Jun 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

chancancode added a commit that referenced this pull request Jun 25, 2016

Merge pull request #23011 from arnvald/bugfix/correctly_parse_date
Use correct timezone when parsing date in json
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jun 25, 2016

Member

@arnvald omg! so sorry for the delay, I have merged this in 3a9428d

@rafaelfranca @sgrif @eileencodes should this be backported to 5-0-0 or 5-0-stable? I would like to get this on 5.0.0 if possible because it is technically a breaking change (the class changed from Time to Date in some cases), and I would like to not do that in 5.0.0 rather than 5.0.1 (or we can leave this for 5.1.0 I suppose)

Member

chancancode commented Jun 25, 2016

@arnvald omg! so sorry for the delay, I have merged this in 3a9428d

@rafaelfranca @sgrif @eileencodes should this be backported to 5-0-0 or 5-0-stable? I would like to get this on 5.0.0 if possible because it is technically a breaking change (the class changed from Time to Date in some cases), and I would like to not do that in 5.0.0 rather than 5.0.1 (or we can leave this for 5.1.0 I suppose)

chancancode added a commit that referenced this pull request Jun 27, 2016

chancancode added a commit to chancancode/rails that referenced this pull request Jun 27, 2016

Merge pull request #23011 from arnvald/bugfix/correctly_parse_date
Use correct timezone when parsing date in json

chancancode added a commit to chancancode/rails that referenced this pull request Jun 27, 2016

chancancode added a commit to chancancode/rails that referenced this pull request Jun 27, 2016

Merge pull request #23011 from arnvald/bugfix/correctly_parse_date
Use correct timezone when parsing date in json

chancancode added a commit to chancancode/rails that referenced this pull request Jun 27, 2016

Merge pull request #23011 from arnvald/bugfix/correctly_parse_date
Use correct timezone when parsing date in json

rafaelfranca added a commit that referenced this pull request Jun 27, 2016

rafaelfranca added a commit that referenced this pull request Jun 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment