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

3 files changed, 33 insertions, 48 deletions

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