Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #12278. AR::ConnectionAdapters::Column.string_to_time method respects string with timezone. #12290

Merged
merged 1 commit into from
Sep 25, 2013

Conversation

kennyj
Copy link
Contributor

@kennyj kennyj commented Sep 19, 2013

Closes #12278.

In current implementation, AR::ConnectionAdapters::Column.string_to_time method ignores timezone.

If with timezone (e.g. Wed, 04 Sep 2013 20:30:00 JST), the method should respects its timezone.

@kennyj
Copy link
Contributor Author

kennyj commented Sep 21, 2013

I've updated this PR to simplify testcase.

@kennyj
Copy link
Contributor Author

kennyj commented Sep 21, 2013

The way of this Pull Request solves original reporter's problem #12278 (comment) .

Is this feature unnecessary for us ?

@rafaelfranca
Copy link
Member

cc @pixeltrix

@ghost ghost assigned pixeltrix Sep 22, 2013
@@ -110,6 +110,11 @@ def test_type_cast_duration_to_integer
assert_equal 1800, column.type_cast(30.minutes)
assert_equal 7200, column.type_cast(2.hours)
end

def test_string_to_time_with_timezone
assert_equal Time.utc(2013, 9, 4, 0, 0, 0), Column.string_to_time("Wed, 04 Sep 2013 09:00:00 JST")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if default_timezone is :local ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's working fine, so a string has timezone.
:local setting returns Time.local(2013, 9, 4, 9, 0, 0) (Time.utc(2013, 9, 4, 0,0,0)) in the above testcase.
I'm going to update testcase.

@kennyj
Copy link
Contributor Author

kennyj commented Sep 24, 2013

I've updated a testcase.


time -= offset
time.send("get#{Base.default_timezone}")
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reverse the condition - unless … else … end is hard to parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha. I'll use if ... else ...end. just a moment.

@kennyj
Copy link
Contributor Author

kennyj commented Sep 24, 2013

I've updated again.

return nil unless time

time -= offset
time.send("get#{Base.default_timezone}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a hot spot if you're loading a lot of AR objects so I'd rather not be creating a new string object every time can you change this to:

Base.default_timezone == :utc ? time : time.getlocal

@kennyj
Copy link
Contributor Author

kennyj commented Sep 24, 2013

@pixeltrix Thank you for many responses. I've updated them.

@kennyj
Copy link
Contributor Author

kennyj commented Sep 24, 2013

I've rebased this PR .

pixeltrix added a commit that referenced this pull request Sep 25, 2013
Closes #12278. AR::ConnectionAdapters::Column.string_to_time method respects string with timezone.
@pixeltrix pixeltrix merged commit f398285 into rails:master Sep 25, 2013
@pixeltrix
Copy link
Contributor

@kennyj thanks!

@kennyj kennyj deleted the fix_12278 branch September 25, 2013 10:23
rafaelfranca added a commit that referenced this pull request Dec 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When submitting a datetime in UTC format from a form ActiveRecord loses timezone info
3 participants