Merge pull request #28147 from kmcphillips/master-time-freeze

Allow Time#to_time on frozen objects. Return frozen time rather than "RuntimeError: can't modify frozen Time"

activesupport/CHANGELOG.md

@@ -208,6 +208,13 @@
 
 ## Rails 5.1.0.beta1 (February 23, 2017) ##
 
+*   Fixed bug in `DateAndTime::Compatibility#to_time` that caused it to
+    raise `RuntimeError: can't modify frozen Time` when called on any frozen `Time`.
+    Properly pass through the frozen `Time` or `ActiveSupport::TimeWithZone` object
+    when calling `#to_time`.
+
+    *Kevin McPhillips* & *Andrew White*
+
 *   Cache `ActiveSupport::TimeWithZone#to_datetime` before freezing.
 
     *Adam Rice*

activesupport/lib/active_support/core_ext/date_and_time/compatibility.rb

@@ -12,11 +12,7 @@ module Compatibility
     mattr_accessor(:preserve_timezone, instance_writer: false) { false }
 
     def to_time
-      if preserve_timezone
-        @_to_time_with_instance_offset ||= getlocal(utc_offset)
-      else
-        @_to_time_with_system_offset ||= getlocal
-      end
+      preserve_timezone ? getlocal(utc_offset) : getlocal
     end
   end
 end

activesupport/lib/active_support/core_ext/time/compatibility.rb

@@ -1,5 +1,12 @@
 require "active_support/core_ext/date_and_time/compatibility"
+require "active_support/core_ext/module/remove_method"
 
 class Time
-  prepend DateAndTime::Compatibility
+  include DateAndTime::Compatibility
+
+  remove_possible_method :to_time
+
+  def to_time
+    preserve_timezone ? self : getlocal
+  end
 end

activesupport/lib/active_support/time_with_zone.rb

@@ -411,6 +411,15 @@ def to_datetime
       @to_datetime ||= utc.to_datetime.new_offset(Rational(utc_offset, 86_400))
     end
 
+    # Returns an instance of <tt>Time</tt>
+    def to_time
+      if preserve_timezone
+        @to_time_with_instance_offset ||= getlocal(utc_offset)
+      else
+        @to_time_with_system_offset ||= getlocal
+      end
+    end
+
     # So that +self+ <tt>acts_like?(:time)</tt>.
     def acts_like_time?
       true
@@ -429,7 +438,7 @@ def blank?
 
     def freeze
       # preload instance variables before freezing
-      period; utc; time; to_datetime
+      period; utc; time; to_datetime; to_time
       super
     end
 

activesupport/test/core_ext/date_and_time_compatibility_test.rb

@@ -16,31 +16,66 @@ def setup
   def test_time_to_time_preserves_timezone
     with_preserve_timezone(true) do
       with_env_tz "US/Eastern" do
-        time = Time.new(2016, 4, 23, 15, 11, 12, 3600).to_time
+        source = Time.new(2016, 4, 23, 15, 11, 12, 3600)
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
         assert_equal @utc_offset, time.utc_offset
+        assert_equal source.object_id, time.object_id
       end
     end
   end
 
   def test_time_to_time_does_not_preserve_time_zone
     with_preserve_timezone(false) do
       with_env_tz "US/Eastern" do
-        time = Time.new(2016, 4, 23, 15, 11, 12, 3600).to_time
+        source = Time.new(2016, 4, 23, 15, 11, 12, 3600)
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
         assert_equal @system_offset, time.utc_offset
+        assert_not_equal source.object_id, time.object_id
+      end
+    end
+  end
+
+  def test_time_to_time_frozen_preserves_timezone
+    with_preserve_timezone(true) do
+      with_env_tz "US/Eastern" do
+        source = Time.new(2016, 4, 23, 15, 11, 12, 3600).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+        assert_equal source.object_id, time.object_id
+        assert_predicate time, :frozen?
+      end
+    end
+  end
+
+  def test_time_to_time_frozen_does_not_preserve_time_zone
+    with_preserve_timezone(false) do
+      with_env_tz "US/Eastern" do
+        source = Time.new(2016, 4, 23, 15, 11, 12, 3600).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+        assert_not_equal source.object_id, time.object_id
+        assert_not_predicate time, :frozen?
       end
     end
   end
 
   def test_datetime_to_time_preserves_timezone
     with_preserve_timezone(true) do
       with_env_tz "US/Eastern" do
-        time = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24)).to_time
+        source = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24))
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
@@ -52,7 +87,8 @@ def test_datetime_to_time_preserves_timezone
   def test_datetime_to_time_does_not_preserve_time_zone
     with_preserve_timezone(false) do
       with_env_tz "US/Eastern" do
-        time = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24)).to_time
+        source = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24))
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
@@ -61,17 +97,47 @@ def test_datetime_to_time_does_not_preserve_time_zone
     end
   end
 
+  def test_datetime_to_time_frozen_preserves_timezone
+    with_preserve_timezone(true) do
+      with_env_tz "US/Eastern" do
+        source = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24)).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
+      end
+    end
+  end
+
+  def test_datetime_to_time_frozen_does_not_preserve_time_zone
+    with_preserve_timezone(false) do
+      with_env_tz "US/Eastern" do
+        source = DateTime.new(2016, 4, 23, 15, 11, 12, Rational(1, 24)).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
+      end
+    end
+  end
+
   def test_twz_to_time_preserves_timezone
     with_preserve_timezone(true) do
       with_env_tz "US/Eastern" do
-        time = ActiveSupport::TimeWithZone.new(@utc_time, @zone).to_time
+        source = ActiveSupport::TimeWithZone.new(@utc_time, @zone)
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
         assert_instance_of Time, time.getutc
         assert_equal @utc_offset, time.utc_offset
 
-        time = ActiveSupport::TimeWithZone.new(@date_time, @zone).to_time
+        source = ActiveSupport::TimeWithZone.new(@date_time, @zone)
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @date_time, time.getutc
@@ -84,27 +150,78 @@ def test_twz_to_time_preserves_timezone
   def test_twz_to_time_does_not_preserve_time_zone
     with_preserve_timezone(false) do
       with_env_tz "US/Eastern" do
-        time = ActiveSupport::TimeWithZone.new(@utc_time, @zone).to_time
+        source = ActiveSupport::TimeWithZone.new(@utc_time, @zone)
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+
+        source = ActiveSupport::TimeWithZone.new(@date_time, @zone)
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @date_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+      end
+    end
+  end
+
+  def test_twz_to_time_frozen_preserves_timezone
+    with_preserve_timezone(true) do
+      with_env_tz "US/Eastern" do
+        source = ActiveSupport::TimeWithZone.new(@utc_time, @zone).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
+
+        source = ActiveSupport::TimeWithZone.new(@date_time, @zone).freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @date_time, time.getutc
+        assert_instance_of Time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
+      end
+    end
+  end
+
+  def test_twz_to_time_frozen_does_not_preserve_time_zone
+    with_preserve_timezone(false) do
+      with_env_tz "US/Eastern" do
+        source = ActiveSupport::TimeWithZone.new(@utc_time, @zone).freeze
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
         assert_instance_of Time, time.getutc
         assert_equal @system_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
 
-        time = ActiveSupport::TimeWithZone.new(@date_time, @zone).to_time
+        source = ActiveSupport::TimeWithZone.new(@date_time, @zone).freeze
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @date_time, time.getutc
         assert_instance_of Time, time.getutc
         assert_equal @system_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
       end
     end
   end
 
   def test_string_to_time_preserves_timezone
     with_preserve_timezone(true) do
       with_env_tz "US/Eastern" do
-        time = "2016-04-23T15:11:12+01:00".to_time
+        source = "2016-04-23T15:11:12+01:00"
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
@@ -116,11 +233,40 @@ def test_string_to_time_preserves_timezone
   def test_string_to_time_does_not_preserve_time_zone
     with_preserve_timezone(false) do
       with_env_tz "US/Eastern" do
-        time = "2016-04-23T15:11:12+01:00".to_time
+        source = "2016-04-23T15:11:12+01:00"
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @system_offset, time.utc_offset
+      end
+    end
+  end
+
+  def test_string_to_time_frozen_preserves_timezone
+    with_preserve_timezone(true) do
+      with_env_tz "US/Eastern" do
+        source = "2016-04-23T15:11:12+01:00".freeze
+        time = source.to_time
+
+        assert_instance_of Time, time
+        assert_equal @utc_time, time.getutc
+        assert_equal @utc_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
+      end
+    end
+  end
+
+  def test_string_to_time_frozen_does_not_preserve_time_zone
+    with_preserve_timezone(false) do
+      with_env_tz "US/Eastern" do
+        source = "2016-04-23T15:11:12+01:00".freeze
+        time = source.to_time
 
         assert_instance_of Time, time
         assert_equal @utc_time, time.getutc
         assert_equal @system_offset, time.utc_offset
+        assert_not_predicate time, :frozen?
       end
     end
   end

activesupport/test/core_ext/time_with_zone_test.rb

@@ -431,11 +431,29 @@ def test_time_at
     assert_equal time, Time.at(time)
   end
 
-  def test_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
+  def test_to_time_with_preserve_timezone
+    with_preserve_timezone(true) do
+      with_env_tz "US/Eastern" do
+        time = @twz.to_time
+
+        assert_equal Time, time.class
+        assert_equal time.object_id, @twz.to_time.object_id
+        assert_equal Time.local(1999, 12, 31, 19), time
+        assert_equal Time.local(1999, 12, 31, 19).utc_offset, time.utc_offset
+      end
+    end
+  end
+
+  def test_to_time_without_preserve_timezone
+    with_preserve_timezone(false) do
+      with_env_tz "US/Eastern" do
+        time = @twz.to_time
+
+        assert_equal Time, time.class
+        assert_equal time.object_id, @twz.to_time.object_id
+        assert_equal Time.local(1999, 12, 31, 19), time
+        assert_equal Time.local(1999, 12, 31, 19).utc_offset, time.utc_offset
+      end
     end
   end
 
@@ -518,6 +536,7 @@ def test_freeze_preloads_instance_variables
       @twz.period
       @twz.time
       @twz.to_datetime
+      @twz.to_time
     end
   end