Standardise the return value of `to_time`

This commit standardises the return value of `to_time` to an instance
of `Time` in the local system timezone, matching the Ruby core and
standard library behavior.

The default form for `String#to_time` has been changed from :utc to
:local but research seems to suggest the latter is the more common form.

Also fix an edge condition with `String#to_time` where the string has
a timezone offset in it and the mode is :local. e.g:

  # Before:
  >> "2000-01-01 00:00:00 -0500".to_time(:local)
  => 2000-01-01 05:00:00 -0500

  # After:
  >> "2000-01-01 00:00:00 -0500".to_time(:local)
  => 2000-01-01 00:00:00 -0500

Closes #2453

8 files changed, 97 insertions, 37 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 2c0d1de70f..72f28aefc7 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,5 +1,10 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Standardise on `to_time` returning an instance of `Time` in the local system timezone
+    across `String`, `Time`, `Date`, `DateTime` and `ActiveSupport::TimeWithZone`.
+
+    *Andrew White*
+
 *   Extract `ActiveSupport::Testing::Performance` into https://github.com/rails/rails-perftest
     You can add the gem to your Gemfile to keep using performance tests.
 
@@ -7,7 +12,6 @@
 
     *Yves Senn*
 
-
 *   Hash.from_xml raises when it encounters type="symbol" or type="yaml".
     Use Hash.from_trusted_xml to parse this XML.
 
diff --git a/activesupport/lib/active_support/core_ext/string/conversions.rb b/activesupport/lib/active_support/core_ext/string/conversions.rb
index c795df124b..07495923a2 100644
--- a/activesupport/lib/active_support/core_ext/string/conversions.rb
+++ b/activesupport/lib/active_support/core_ext/string/conversions.rb
@@ -3,26 +3,36 @@ require 'active_support/core_ext/time/calculations'
 
 class String
   # Converts a string to a Time value.
-  # The +form+ can be either :utc or :local (default :utc).
+  # The +form+ can be either :utc or :local (default :local).
   #
-  # The time is parsed using Date._parse method.
-  # If +form+ is :local, then time is formatted using Time.zone
+  # The time is parsed using Time.parse method.
+  # If +form+ is :local, then the time is in the system timezone.
+  # If the date part is missing then the current date is used and if
+  # the time part is missing then it is assumed to be 00:00:00.
   #
-  #   "3-2-2012".to_time                 # => 2012-02-03 00:00:00 UTC
-  #   "12:20".to_time                    # => ArgumentError: invalid date
-  #   "2012-12-13 06:12".to_time         # => 2012-12-13 06:12:00 UTC
-  #   "2012-12-13T06:12".to_time         # => 2012-12-13 06:12:00 UTC
-  #   "2012-12-13T06:12".to_time(:local) # => 2012-12-13 06:12:00 +0100
-  def to_time(form = :utc)
-    unless blank?
-      date_values = ::Date._parse(self, false).
-        values_at(:year, :mon, :mday, :hour, :min, :sec, :sec_fraction, :offset).
-        map! { |arg| arg || 0 }
-      date_values[6] *= 1000000
-      offset = date_values.pop
+  #   "13-12-2012".to_time               # => 2012-12-13 00:00:00 +0100
+  #   "06:12".to_time                    # => 2012-12-13 06:12:00 +0100
+  #   "2012-12-13 06:12".to_time         # => 2012-12-13 06:12:00 +0100
+  #   "2012-12-13T06:12".to_time         # => 2012-12-13 06:12:00 +0100
+  #   "2012-12-13T06:12".to_time(:utc)   # => 2012-12-13 05:12:00 UTC
+  def to_time(form = :local)
+    parts = Date._parse(self, false)
+    return if parts.empty?
 
-      ::Time.send(form, *date_values) - offset
-    end
+    now = Time.now
+    offset = parts[:offset]
+    utc_offset = form == :utc ? 0 : now.utc_offset
+    adjustment = offset ? offset - utc_offset : 0
+
+    Time.send(
+      form,
+      parts.fetch(:year, now.year),
+      parts.fetch(:mon, now.month),
+      parts.fetch(:mday, now.day),
+      parts.fetch(:hour, 0),
+      parts.fetch(:min, 0),
+      parts.fetch(:sec, 0) + parts.fetch(:sec_fraction, 0)
+    ) - adjustment
   end
 
   # Converts a string to a Date value.
diff --git a/activesupport/lib/active_support/time_with_zone.rb b/activesupport/lib/active_support/time_with_zone.rb
index d3741845d2..ff13efa990 100644
--- a/activesupport/lib/active_support/time_with_zone.rb
+++ b/activesupport/lib/active_support/time_with_zone.rb
@@ -317,9 +317,9 @@ module ActiveSupport
     end
     alias_method :tv_sec, :to_i
 
-    # A TimeWithZone acts like a Time, so just return +self+.
+    # Return an instance of Time in the system timezone.
     def to_time
-      utc
+      utc.to_time
     end
 
     def to_datetime
diff --git a/activesupport/test/core_ext/date_ext_test.rb b/activesupport/test/core_ext/date_ext_test.rb
index ec47d0632c..f3fa96ec6f 100644
--- a/activesupport/test/core_ext/date_ext_test.rb
+++ b/activesupport/test/core_ext/date_ext_test.rb
@@ -33,8 +33,12 @@ class DateExtCalculationsTest < ActiveSupport::TestCase
   end
 
   def test_to_time
-    assert_equal Time.local(2005, 2, 21), Date.new(2005, 2, 21).to_time
-    assert_equal Time.local(2039, 2, 21), Date.new(2039, 2, 21).to_time
+    with_env_tz 'US/Eastern' do
+      assert_equal Time, Date.new(2005, 2, 21).to_time.class
+      assert_equal Time.local(2005, 2, 21), Date.new(2005, 2, 21).to_time
+      assert_equal Time.local(2005, 2, 21).utc_offset, Date.new(2005, 2, 21).to_time.utc_offset
+    end
+
     silence_warnings do
       0.upto(138) do |year|
         [:utc, :local].each do |format|
diff --git a/activesupport/test/core_ext/date_time_ext_test.rb b/activesupport/test/core_ext/date_time_ext_test.rb
index 54bbdbb18f..f74b7b9917 100644
--- a/activesupport/test/core_ext/date_time_ext_test.rb
+++ b/activesupport/test/core_ext/date_time_ext_test.rb
@@ -41,11 +41,14 @@ class DateTimeExtCalculationsTest < ActiveSupport::TestCase
   end
 
   def test_to_time
-    assert_equal Time.utc(2005, 2, 21, 10, 11, 12), DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time
-    assert_equal Time.utc(2039, 2, 21, 10, 11, 12), DateTime.new(2039, 2, 21, 10, 11, 12, 0).to_time
-    # DateTimes with offsets other than 0 are returned unaltered
-    assert_equal DateTime.new(2005, 2, 21, 10, 11, 12, Rational(-5, 24)), DateTime.new(2005, 2, 21, 10, 11, 12, Rational(-5, 24)).to_time
-    # Fractional seconds are preserved
+    with_env_tz 'US/Eastern' do
+      assert_equal Time, DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time.class
+      assert_equal Time.local(2005, 2, 21, 5, 11, 12), DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time
+      assert_equal Time.local(2005, 2, 21, 5, 11, 12).utc_offset, DateTime.new(2005, 2, 21, 10, 11, 12, 0).to_time.utc_offset
+    end
+  end
+
+  def test_to_time_preserves_fractional_seconds
     assert_equal Time.utc(2005, 2, 21, 10, 11, 12, 256), DateTime.new(2005, 2, 21, 10, 11, 12 + Rational(256, 1000000), 0).to_time
   end
 
diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb
index e0ddeab548..951e6bac07 100644
--- a/activesupport/test/core_ext/string_ext_test.rb
+++ b/activesupport/test/core_ext/string_ext_test.rb
@@ -286,14 +286,37 @@ end
 
 class StringConversionsTest < ActiveSupport::TestCase
   def test_string_to_time
-    assert_equal Time.utc(2005, 2, 27, 23, 50), "2005-02-27 23:50".to_time
-    assert_equal Time.local(2005, 2, 27, 23, 50), "2005-02-27 23:50".to_time(:local)
-    assert_equal Time.utc(2005, 2, 27, 23, 50, 19, 275038), "2005-02-27T23:50:19.275038".to_time
-    assert_equal Time.local(2005, 2, 27, 23, 50, 19, 275038), "2005-02-27T23:50:19.275038".to_time(:local)
-    assert_equal DateTime.civil(2039, 2, 27, 23, 50), "2039-02-27 23:50".to_time
-    assert_equal Time.local(2039, 2, 27, 23, 50), "2039-02-27 23:50".to_time(:local)
-    assert_equal Time.utc(2011, 2, 27, 23, 50), "2011-02-27 22:50 -0100".to_time
-    assert_nil "".to_time
+    with_env_tz "US/Eastern" do
+      assert_equal Time.utc(2005, 2, 27, 23, 50), "2005-02-27 23:50".to_time(:utc)
+      assert_equal Time.local(2005, 2, 27, 23, 50), "2005-02-27 23:50".to_time
+      assert_equal Time.utc(2005, 2, 27, 23, 50, 19, 275038), "2005-02-27T23:50:19.275038".to_time(:utc)
+      assert_equal Time.local(2005, 2, 27, 23, 50, 19, 275038), "2005-02-27T23:50:19.275038".to_time
+      assert_equal Time.utc(2039, 2, 27, 23, 50), "2039-02-27 23:50".to_time(:utc)
+      assert_equal Time.local(2039, 2, 27, 23, 50), "2039-02-27 23:50".to_time
+      assert_equal Time.local(2011, 2, 27, 18, 50), "2011-02-27 22:50 -0100".to_time
+      assert_equal Time.utc(2011, 2, 27, 23, 50), "2011-02-27 22:50 -0100".to_time(:utc)
+      assert_equal Time.local(2005, 2, 27, 23, 50), "2005-02-27 23:50 -0500".to_time
+      assert_nil "".to_time
+    end
+  end
+
+  def test_string_to_time_utc_offset
+    with_env_tz "US/Eastern" do
+      assert_equal 0, "2005-02-27 23:50".to_time(:utc).utc_offset
+      assert_equal(-18000, "2005-02-27 23:50".to_time.utc_offset)
+      assert_equal 0, "2005-02-27 22:50 -0100".to_time(:utc).utc_offset
+      assert_equal(-18000, "2005-02-27 22:50 -0100".to_time.utc_offset)
+    end
+  end
+
+  def test_partial_string_to_time
+    with_env_tz "US/Eastern" do
+      now = Time.now
+      assert_equal Time.local(now.year, now.month, now.day, 23, 50), "23:50".to_time
+      assert_equal Time.utc(now.year, now.month, now.day, 23, 50), "23:50".to_time(:utc)
+      assert_equal Time.local(now.year, now.month, now.day, 18, 50), "22:50 -0100".to_time
+      assert_equal Time.utc(now.year, now.month, now.day, 23, 50), "22:50 -0100".to_time(:utc)
+    end
   end
 
   def test_string_to_datetime
@@ -309,6 +332,14 @@ class StringConversionsTest < ActiveSupport::TestCase
     assert_nil "".to_date
     assert_equal Date.new(Date.today.year, 2, 3), "Feb 3rd".to_date
   end
+
+  protected
+    def with_env_tz(new_tz = 'US/Eastern')
+      old_tz, ENV['TZ'] = ENV['TZ'], new_tz
+      yield
+    ensure
+      old_tz ? ENV['TZ'] = old_tz : ENV.delete('TZ')
+    end
 end
 
 class StringBehaviourTest < ActiveSupport::TestCase
diff --git a/activesupport/test/core_ext/time_ext_test.rb b/activesupport/test/core_ext/time_ext_test.rb
index a2fefee3b8..43c92003dc 100644
--- a/activesupport/test/core_ext/time_ext_test.rb
+++ b/activesupport/test/core_ext/time_ext_test.rb
@@ -526,7 +526,11 @@ class TimeExtCalculationsTest < ActiveSupport::TestCase
   end
 
   def test_to_time
-    assert_equal Time.local(2005, 2, 21, 17, 44, 30), Time.local(2005, 2, 21, 17, 44, 30).to_time
+    with_env_tz 'US/Eastern' do
+      assert_equal Time, Time.local(2005, 2, 21, 17, 44, 30).to_time.class
+      assert_equal Time.local(2005, 2, 21, 17, 44, 30), Time.local(2005, 2, 21, 17, 44, 30).to_time
+      assert_equal Time.local(2005, 2, 21, 17, 44, 30).utc_offset, Time.local(2005, 2, 21, 17, 44, 30).to_time.utc_offset
+    end
   end
 
   # NOTE: this test seems to fail (changeset 1958) only on certain platforms,
diff --git a/activesupport/test/core_ext/time_with_zone_test.rb b/activesupport/test/core_ext/time_with_zone_test.rb
index 6c773770f0..18eca4cd8b 100644
--- a/activesupport/test/core_ext/time_with_zone_test.rb
+++ b/activesupport/test/core_ext/time_with_zone_test.rb
@@ -327,7 +327,11 @@ class TimeWithZoneTest < ActiveSupport::TestCase
   end
 
   def test_to_time
-    assert_equal @twz, @twz.to_time
+    with_env_tz 'US/Eastern' do
+      assert_equal Time, @twz.to_time.class
+      assert_equal Time.local(1999, 12, 31, 19), @twz.to_time
+      assert_equal Time.local(1999, 12, 31, 19).utc_offset, @twz.to_time.utc_offset
+    end
   end
 
   def test_to_date