diff options
| -rw-r--r-- | activemodel/test/cases/validations/conditional_validation_test.rb | 66 | ||||
| -rw-r--r-- | activemodel/test/cases/validations/with_validation_test.rb | 40 | ||||
| -rw-r--r-- | activerecord/lib/active_record/callbacks.rb | 4 | ||||
| -rw-r--r-- | activerecord/lib/active_record/transactions.rb | 2 | ||||
| -rw-r--r-- | activesupport/CHANGELOG.md | 4 | ||||
| -rw-r--r-- | activesupport/lib/active_support/callbacks.rb | 55 | ||||
| -rw-r--r-- | activesupport/test/callbacks_test.rb | 22 |
7 files changed, 38 insertions, 155 deletions
diff --git a/activemodel/test/cases/validations/conditional_validation_test.rb b/activemodel/test/cases/validations/conditional_validation_test.rb index 68dade556c..aa027c4128 100644 --- a/activemodel/test/cases/validations/conditional_validation_test.rb +++ b/activemodel/test/cases/validations/conditional_validation_test.rb @@ -43,48 +43,6 @@ class ConditionalValidationTest < ActiveModel::TestCase assert_equal ["hoo 5"], t.errors["title"] end - def test_if_validation_using_string_true - # When the evaluated string returns true - ActiveSupport::Deprecation.silence do - Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "a = 1; a == 1") - end - t = Topic.new("title" => "uhohuhoh", "content" => "whatever") - assert t.invalid? - assert t.errors[:title].any? - assert_equal ["hoo 5"], t.errors["title"] - end - - def test_unless_validation_using_string_true - # When the evaluated string returns true - ActiveSupport::Deprecation.silence do - Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "a = 1; a == 1") - end - t = Topic.new("title" => "uhohuhoh", "content" => "whatever") - assert t.valid? - assert_empty t.errors[:title] - end - - def test_if_validation_using_string_false - # When the evaluated string returns false - ActiveSupport::Deprecation.silence do - Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "false") - end - t = Topic.new("title" => "uhohuhoh", "content" => "whatever") - assert t.valid? - assert_empty t.errors[:title] - end - - def test_unless_validation_using_string_false - # When the evaluated string returns false - ActiveSupport::Deprecation.silence do - Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "false") - end - t = Topic.new("title" => "uhohuhoh", "content" => "whatever") - assert t.invalid? - assert t.errors[:title].any? - assert_equal ["hoo 5"], t.errors["title"] - end - def test_if_validation_using_block_true # When the block returns true Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", @@ -122,28 +80,4 @@ class ConditionalValidationTest < ActiveModel::TestCase assert t.errors[:title].any? assert_equal ["hoo 5"], t.errors["title"] end - - # previous implementation of validates_presence_of eval'd the - # string with the wrong binding, this regression test is to - # ensure that it works correctly - def test_validation_with_if_as_string - Topic.validates_presence_of(:title) - ActiveSupport::Deprecation.silence do - Topic.validates_presence_of(:author_name, if: "title.to_s.match('important')") - end - - t = Topic.new - assert t.invalid?, "A topic without a title should not be valid" - assert_empty t.errors[:author_name], "A topic without an 'important' title should not require an author" - - t.title = "Just a title" - assert t.valid?, "A topic with a basic title should be valid" - - t.title = "A very important title" - assert t.invalid?, "A topic with an important title, but without an author, should not be valid" - assert t.errors[:author_name].any?, "A topic with an 'important' title should require an author" - - t.author_name = "Hubert J. Farnsworth" - assert t.valid?, "A topic with an important title and author should be valid" - end end diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb index fbe20dc000..13ef5e6a31 100644 --- a/activemodel/test/cases/validations/with_validation_test.rb +++ b/activemodel/test/cases/validations/with_validation_test.rb @@ -70,51 +70,15 @@ class ValidatesWithTest < ActiveModel::TestCase assert_includes topic.errors[:base], OTHER_ERROR_MESSAGE end - test "with if statements that return false" do - ActiveSupport::Deprecation.silence do - Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 2") - end - topic = Topic.new - assert topic.valid? - end - - test "with if statements that return true" do - ActiveSupport::Deprecation.silence do - Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 1") - end - topic = Topic.new - assert topic.invalid? - assert_includes topic.errors[:base], ERROR_MESSAGE - end - - test "with unless statements that return true" do - ActiveSupport::Deprecation.silence do - Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 1") - end - topic = Topic.new - assert topic.valid? - end - - test "with unless statements that returns false" do - ActiveSupport::Deprecation.silence do - Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 2") - end - topic = Topic.new - assert topic.invalid? - assert_includes topic.errors[:base], ERROR_MESSAGE - end - test "passes all configuration options to the validator class" do topic = Topic.new validator = Minitest::Mock.new - validator.expect(:new, validator, [{ foo: :bar, if: "1 == 1", class: Topic }]) + validator.expect(:new, validator, [{ foo: :bar, if: :condition_is_true, class: Topic }]) validator.expect(:validate, nil, [topic]) validator.expect(:is_a?, false, [Symbol]) validator.expect(:is_a?, false, [String]) - ActiveSupport::Deprecation.silence do - Topic.validates_with(validator, if: "1 == 1", foo: :bar) - end + Topic.validates_with(validator, if: :condition_is_true, foo: :bar) assert topic.valid? validator.verify end diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb index ac1070e65b..9ab2780760 100644 --- a/activerecord/lib/active_record/callbacks.rb +++ b/activerecord/lib/active_record/callbacks.rb @@ -98,9 +98,9 @@ module ActiveRecord # == Types of callbacks # # There are four types of callbacks accepted by the callback macros: Method references (symbol), callback objects, - # inline methods (using a proc), and inline eval methods (using a string). Method references and callback objects + # inline methods (using a proc). Method references and callback objects # are the recommended approaches, inline methods using a proc are sometimes appropriate (such as for - # creating mix-ins), and inline eval methods are deprecated. + # creating mix-ins). # # The method reference callbacks work by specifying a protected or private method available in the object, like this: # diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 6761f2da25..97cba5d1c7 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -285,7 +285,7 @@ module ActiveRecord fire_on = Array(options[:on]) assert_valid_transaction_action(fire_on) options[:if] = Array(options[:if]) - options[:if].unshift("transaction_include_any_action?(#{fire_on})") + options[:if].unshift(-> { transaction_include_any_action?(fire_on) }) end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 7696fdcd7a..387eb4cbb3 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,7 @@ +* Remove deprecated `:if` and `:unless` string filter for callbacks. + + *Rafael Mendonça França* + * `Hash#slice` now falls back to Ruby 2.5+'s built-in definition if defined. *Akira Matsuda* diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index eb6a4c3ce7..0ed4681b7d 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -298,8 +298,8 @@ module ActiveSupport @kind = kind @filter = filter @key = compute_identifier filter - @if = Array(options[:if]) - @unless = Array(options[:unless]) + @if = check_conditionals(Array(options[:if])) + @unless = check_conditionals(Array(options[:unless])) end def filter; @key; end @@ -323,7 +323,7 @@ module ActiveSupport def duplicates?(other) case @filter - when Symbol, String + when Symbol matches?(other.kind, other.filter) else false @@ -350,9 +350,21 @@ module ActiveSupport end private + def check_conditionals(conditionals) + if conditionals.any? { |c| c.is_a?(String) } + raise ArgumentError, <<-MSG.squish + Passing string to be evaluated in :if and :unless conditional + options is not supported. Pass a symbol for an instance method, + or a lambda, proc or block, instead. + MSG + end + + conditionals + end + def compute_identifier(filter) case filter - when String, ::Proc + when ::Proc filter.object_id else filter @@ -427,7 +439,6 @@ module ActiveSupport # Filters support: # # Symbols:: A method to call. - # Strings:: Some content to evaluate. # Procs:: A proc to call with the object. # Objects:: An object with a <tt>before_foo</tt> method on it to call. # @@ -437,8 +448,6 @@ module ActiveSupport case filter when Symbol new(nil, filter, [], nil) - when String - new(nil, :instance_exec, [:value], compile_lambda(filter)) when Conditionals::Value new(filter, :call, [:target, :value], nil) when ::Proc @@ -455,10 +464,6 @@ module ActiveSupport new(filter, method_to_call, [:target], nil) end end - - def self.compile_lambda(filter) - eval("lambda { |value| #{filter} }") - end end # Execute before and after filters in a sequence instead of @@ -651,26 +656,17 @@ module ActiveSupport # # ===== Options # - # * <tt>:if</tt> - A symbol, a string (deprecated) or an array of symbols, - # each naming an instance method or a proc; the callback will be called - # only when they all return a true value. - # * <tt>:unless</tt> - A symbol, a string (deprecated) or an array of symbols, - # each naming an instance method or a proc; the callback will be called - # only when they all return a false value. + # * <tt>:if</tt> - A symbol or an array of symbols, each naming an instance + # method or a proc; the callback will be called only when they all return + # a true value. + # * <tt>:unless</tt> - A symbol or an array of symbols, each naming an + # instance method or a proc; the callback will be called only when they + # all return a false value. # * <tt>:prepend</tt> - If +true+, the callback will be prepended to the # existing chain rather than appended. def set_callback(name, *filter_list, &block) type, filters, options = normalize_callback_params(filter_list, block) - if options[:if].is_a?(String) || options[:unless].is_a?(String) - ActiveSupport::Deprecation.warn(<<-MSG.squish) - Passing string to be evaluated in :if and :unless conditional - options is deprecated and will be removed in Rails 5.2 without - replacement. Pass a symbol for an instance method, or a lambda, - proc or block, instead. - MSG - end - self_chain = get_callbacks name mapped = filters.map do |filter| Callback.build(self_chain, filter, type, options) @@ -695,13 +691,6 @@ module ActiveSupport def skip_callback(name, *filter_list, &block) type, filters, options = normalize_callback_params(filter_list, block) - if options[:if].is_a?(String) || options[:unless].is_a?(String) - ActiveSupport::Deprecation.warn(<<-MSG.squish) - Passing string to :if and :unless conditional options is deprecated - and will be removed in Rails 5.2 without replacement. - MSG - end - options[:raise] = true unless options.key?(:raise) __update_callbacks(name) do |target, chain| diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index 3902e41a60..5f894db46f 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -193,13 +193,6 @@ module CallbacksTest before_save Proc.new { |r| r.history << "b00m" }, if: :no before_save Proc.new { |r| r.history << [:before_save, :symbol] }, unless: :no before_save Proc.new { |r| r.history << "b00m" }, unless: :yes - # string - ActiveSupport::Deprecation.silence do - before_save Proc.new { |r| r.history << [:before_save, :string] }, if: "yes" - before_save Proc.new { |r| r.history << "b00m" }, if: "no" - before_save Proc.new { |r| r.history << [:before_save, :string] }, unless: "no" - before_save Proc.new { |r| r.history << "b00m" }, unless: "yes" - end # Combined if and unless before_save Proc.new { |r| r.history << [:before_save, :combined_symbol] }, if: :yes, unless: :no before_save Proc.new { |r| r.history << "b00m" }, if: :yes, unless: :yes @@ -592,8 +585,6 @@ module CallbacksTest [:before_save, :proc], [:before_save, :symbol], [:before_save, :symbol], - [:before_save, :string], - [:before_save, :string], [:before_save, :combined_symbol], ], person.history end @@ -1182,14 +1173,15 @@ module CallbacksTest end end - class DeprecatedWarningTest < ActiveSupport::TestCase - def test_deprecate_string_conditional_options + class NotSupportedStringConditionalTest < ActiveSupport::TestCase + def test_string_conditional_options klass = Class.new(Record) - assert_deprecated { klass.before_save :tweedle, if: "true" } - assert_deprecated { klass.after_save :tweedle, unless: "false" } - assert_deprecated { klass.skip_callback :save, :before, :tweedle, if: "true" } - assert_deprecated { klass.skip_callback :save, :after, :tweedle, unless: "false" } + assert_raises(ArgumentError) { klass.before_save :tweedle, if: ["true"] } + assert_raises(ArgumentError) { klass.before_save :tweedle, if: "true" } + assert_raises(ArgumentError) { klass.after_save :tweedle, unless: "false" } + assert_raises(ArgumentError) { klass.skip_callback :save, :before, :tweedle, if: "true" } + assert_raises(ArgumentError) { klass.skip_callback :save, :after, :tweedle, unless: "false" } end end |