Remove deprecated `:if` and `:unless` string filter for callbacks

activemodel/test/cases/validations/conditional_validation_test.rb

@@ -43,48 +43,6 @@ def test_unless_validation_using_method_false
     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 @@ def test_unless_validation_using_block_false
     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

activemodel/test/cases/validations/with_validation_test.rb

@@ -70,51 +70,15 @@ def check_validity!
     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

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:
   #

activerecord/lib/active_record/transactions.rb

@@ -285,7 +285,7 @@ def set_options_for_callbacks!(args, enforced_options = {})
             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
 

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*

activesupport/lib/active_support/callbacks.rb

@@ -298,8 +298,8 @@ def initialize(name, filter, kind, options, chain_config)
           @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 @@ def matches?(_kind, _filter)
 
         def duplicates?(other)
           case @filter
-          when Symbol, String
+          when Symbol
             matches?(other.kind, other.filter)
           else
             false
@@ -350,9 +350,21 @@ def current_scopes
         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 @@ def inverted_lambda
         # 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 @@ def self.build(filter, callback)
           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 @@ def self.build(filter, callback)
             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 @@ def __update_callbacks(name) #:nodoc:
         #
         # ===== 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 @@ def set_callback(name, *filter_list, &block)
         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|

activesupport/test/callbacks_test.rb

@@ -193,13 +193,6 @@ class ConditionalPerson < Record
     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 @@ def test_save_conditional_person
         [: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 @@ def test_skip_without_raise # removes nothing
     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