From 75924c4517c8f87712d3f59c11f10152ed57b9d8 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 27 Feb 2017 18:31:35 +0000 Subject: Deprecate implicit coercion of `ActiveSupport::Duration` Currently `ActiveSupport::Duration` implicitly converts to a seconds value when used in a calculation except for the explicit examples of addition and subtraction where the duration is the receiver, e.g: >> 2 * 1.day => 172800 This results in lots of confusion especially when using durations with dates because adding/subtracting a value from a date treats integers as a day and not a second, e.g: >> Date.today => Wed, 01 Mar 2017 >> Date.today + 2 * 1.day => Mon, 10 Apr 2490 To fix this we're implementing `coerce` so that we can provide a deprecation warning with the intent of removing the implicit coercion in Rails 5.2, e.g: >> 2 * 1.day DEPRECATION WARNING: Implicit coercion of ActiveSupport::Duration to a Numeric is deprecated and will raise a TypeError in Rails 5.2. => 172800 In Rails 5.2 it will raise `TypeError`, e.g: >> 2 * 1.day TypeError: ActiveSupport::Duration can't be coerced into Integer This is the same behavior as with other types in Ruby, e.g: >> 2 * "foo" TypeError: String can't be coerced into Integer >> "foo" * 2 => "foofoo" As part of this deprecation add `*` and `/` methods to `AS::Duration` so that calculations that keep the duration as the receiver work correctly whether the final receiver is a `Date` or `Time`, e.g: >> Date.today => Wed, 01 Mar 2017 >> Date.today + 1.day * 2 => Fri, 03 Mar 2017 Fixes #27457. --- actionmailer/test/message_delivery_test.rb | 2 +- actionview/test/template/date_helper_test.rb | 2 +- activesupport/CHANGELOG.md | 52 +++++++++++++++++++ .../lib/active_support/cache/mem_cache_store.rb | 2 +- activesupport/lib/active_support/duration.rb | 41 ++++++++++++++- activesupport/test/core_ext/duration_test.rb | 58 ++++++++++++++++++++-- 6 files changed, 149 insertions(+), 8 deletions(-) diff --git a/actionmailer/test/message_delivery_test.rb b/actionmailer/test/message_delivery_test.rb index a79d77e1e5..f4c4f43bdc 100644 --- a/actionmailer/test/message_delivery_test.rb +++ b/actionmailer/test/message_delivery_test.rb @@ -76,7 +76,7 @@ class MessageDeliveryTest < ActiveSupport::TestCase test "should enqueue a delivery with a delay" do travel_to Time.new(2004, 11, 24, 01, 04, 44) do - assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current.to_f + 600.seconds, args: ["DelayedMailer", "test_message", "deliver_now", 1, 2, 3]) do + assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current.to_f + 600, args: ["DelayedMailer", "test_message", "deliver_now", 1, 2, 3]) do @mail.deliver_later wait: 600.seconds end end diff --git a/actionview/test/template/date_helper_test.rb b/actionview/test/template/date_helper_test.rb index bfd3ecd6fd..d257147e1f 100644 --- a/actionview/test/template/date_helper_test.rb +++ b/actionview/test/template/date_helper_test.rb @@ -832,7 +832,7 @@ class DateHelperTest < ActionView::TestCase def test_select_date_with_too_big_range_between_start_year_and_end_year assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: 2000, end_year: 20000, prefix: "date[first]", order: [:month, :day, :year]) } - assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: Date.today.year - 100.years, end_year: 2000, prefix: "date[first]", order: [:month, :day, :year]) } + assert_raise(ArgumentError) { select_date(Time.mktime(2003, 8, 16), start_year: 100, end_year: 2000, prefix: "date[first]", order: [:month, :day, :year]) } end def test_select_date_can_have_more_then_1000_years_interval_if_forced_via_parameter diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 2999820c42..6ca227f40a 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,55 @@ +* Deprecate implicit coercion of `ActiveSupport::Duration` + + Currently `ActiveSupport::Duration` implicitly converts to a seconds + value when used in a calculation except for the explicit examples of + addition and subtraction where the duration is the receiver, e.g: + + >> 2 * 1.day + => 172800 + + This results in lots of confusion especially when using durations + with dates because adding/subtracting a value from a date treats + integers as a day and not a second, e.g: + + >> Date.today + => Wed, 01 Mar 2017 + >> Date.today + 2 * 1.day + => Mon, 10 Apr 2490 + + To fix this we're implementing `coerce` so that we can provide a + deprecation warning with the intent of removing the implicit coercion + in Rails 5.2, e.g: + + >> 2 * 1.day + DEPRECATION WARNING: Implicit coercion of ActiveSupport::Duration + to a Numeric is deprecated and will raise a TypeError in Rails 5.2. + => 172800 + + In Rails 5.2 it will raise `TypeError`, e.g: + + >> 2 * 1.day + TypeError: ActiveSupport::Duration can't be coerced into Integer + + This is the same behavior as with other types in Ruby, e.g: + + >> 2 * "foo" + TypeError: String can't be coerced into Integer + >> "foo" * 2 + => "foofoo" + + As part of this deprecation add `*` and `/` methods to `AS::Duration` + so that calculations that keep the duration as the receiver work + correctly whether the final receiver is a `Date` or `Time`, e.g: + + >> Date.today + => Wed, 01 Mar 2017 + >> Date.today + 1.day * 2 + => Fri, 03 Mar 2017 + + Fixes #27457. + + *Andrew White* + * Update `DateTime#change` to support `:usec` and `:nsec` options. Adding support for these options now allows us to update the `DateTime#end_of` diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index e09cee3335..5eee04a34e 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -156,7 +156,7 @@ module ActiveSupport expires_in = options[:expires_in].to_i if expires_in > 0 && !options[:raw] # Set the memcache expire a few minutes in the future to support race condition ttls on read - expires_in += 5.minutes + expires_in += 300 end rescue_error_with false do @data.send(method, key, value, expires_in, options) diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb index 70cf78519d..b92891c511 100644 --- a/activesupport/lib/active_support/duration.rb +++ b/activesupport/lib/active_support/duration.rb @@ -1,5 +1,7 @@ require "active_support/core_ext/array/conversions" require "active_support/core_ext/object/acts_like" +require "active_support/core_ext/string/filters" +require "active_support/deprecation" module ActiveSupport # Provides accurate date and time measurements using Date#advance and @@ -88,6 +90,25 @@ module ActiveSupport @parts.default = 0 end + def coerce(other) #:nodoc: + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Implicit coercion of ActiveSupport::Duration to a Numeric + is deprecated and will raise a TypeError in Rails 5.2. + MSG + + [other, value] + end + + # Compares one Duration with another or a Numeric to this Duration. + # Numeric values are treated as seconds. + def <=>(other) + if Duration === other + value <=> other.value + elsif Numeric === other + value <=> other + end + end + # Adds another Duration or a Numeric to this Duration. Numeric values # are treated as seconds. def +(other) @@ -109,6 +130,24 @@ module ActiveSupport self + (-other) end + # Multiplies this Duration by a Numeric and returns a new Duration. + def *(other) + if Numeric === other + Duration.new(value * other, parts.map { |type, number| [type, number * other] }) + else + value * other + end + end + + # Devides this Duration by a Numeric and returns a new Duration. + def /(other) + if Numeric === other + Duration.new(value / other, parts.map { |type, number| [type, number / other] }) + else + value / other + end + end + def -@ #:nodoc: Duration.new(-value, parts.map { |type, number| [type, -number] }) end @@ -212,8 +251,6 @@ module ActiveSupport ISO8601Serializer.new(self, precision: precision).serialize end - delegate :<=>, to: :value - private def sum(sign, time = ::Time.current) diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb index 6facb04f1f..2b1a715b7a 100644 --- a/activesupport/test/core_ext/duration_test.rb +++ b/activesupport/test/core_ext/duration_test.rb @@ -84,8 +84,46 @@ class DurationTest < ActiveSupport::TestCase assert_nothing_raised { Date.today - Date.today } end + def test_plus + assert_equal 2.seconds, 1.second + 1.second + assert_instance_of ActiveSupport::Duration, 1.second + 1.second + assert_equal 2.seconds, 1.second + 1 + assert_instance_of ActiveSupport::Duration, 1.second + 1 + end + + def test_minus + assert_equal 1.second, 2.seconds - 1.second + assert_instance_of ActiveSupport::Duration, 2.seconds - 1.second + assert_equal 1.second, 2.seconds - 1 + assert_instance_of ActiveSupport::Duration, 2.seconds - 1 + end + + def test_multiply + assert_equal 7.days, 1.day * 7 + assert_instance_of ActiveSupport::Duration, 1.day * 7 + + assert_deprecated do + assert_equal 86400, 1.day * 1.second + end + end + + def test_divide + assert_equal 1.day, 7.days / 7 + assert_instance_of ActiveSupport::Duration, 7.days / 7 + + assert_deprecated do + assert_equal 1, 1.day / 1.day + end + end + + def test_date_added_with_multiplied_duration + assert_equal Date.civil(2017, 1, 3), Date.civil(2017, 1, 1) + 1.day * 2 + end + def test_plus_with_time - assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration" + assert_deprecated do + assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration" + end end def test_time_plus_duration_returns_same_time_datatype @@ -104,6 +142,13 @@ class DurationTest < ActiveSupport::TestCase assert_equal 'expected a time or date, got ""', e.message, "ensure ArgumentError is not being raised by dependencies.rb" end + def test_implicit_coercion_is_deprecated + assert_deprecated { 1 + 1.second } + assert_deprecated { 1 - 1.second } + assert_deprecated { 1 * 1.second } + assert_deprecated { 1 / 1.second } + end + def test_fractional_weeks assert_equal((86400 * 7) * 1.5, 1.5.weeks) assert_equal((86400 * 7) * 1.7, 1.7.weeks) @@ -241,13 +286,20 @@ class DurationTest < ActiveSupport::TestCase def test_comparable assert_equal(-1, (0.seconds <=> 1.second)) assert_equal(-1, (1.second <=> 1.minute)) - assert_equal(-1, (1 <=> 1.minute)) + + assert_deprecated do + assert_equal(-1, (1 <=> 1.minute)) + end + assert_equal(0, (0.seconds <=> 0.seconds)) assert_equal(0, (0.seconds <=> 0.minutes)) assert_equal(0, (1.second <=> 1.second)) assert_equal(1, (1.second <=> 0.second)) assert_equal(1, (1.minute <=> 1.second)) - assert_equal(1, (61 <=> 1.minute)) + + assert_deprecated do + assert_equal(1, (61 <=> 1.minute)) + end end def test_twelve_months_equals_one_year -- cgit v1.2.3