Deprecate passing string to :if and :unless conditional options on set_callback and skip_callback

Author: kamipo

activemodel/test/cases/validations/conditional_validation_test.rb

@@ -43,7 +43,9 @@ def test_unless_validation_using_method_false
 
   def test_if_validation_using_string_true
     # When the evaluated string returns true
-    Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "a = 1; a == 1")
+    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?
@@ -52,23 +54,29 @@ def test_if_validation_using_string_true
 
   def test_unless_validation_using_string_true
     # When the evaluated string returns true
-    Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "a = 1; a == 1")
+    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
-    Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "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
-    Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "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?
@@ -118,7 +126,9 @@ def test_unless_validation_using_block_false
   # ensure that it works correctly
   def test_validation_with_if_as_string
     Topic.validates_presence_of(:title)
-    Topic.validates_presence_of(:author_name, if: "title.to_s.match('important')")
+    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"

activemodel/test/cases/validations/with_validation_test.rb

@@ -69,26 +69,34 @@ def check_validity!
   end
 
   test "with if statements that return false" do
-    Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 2")
+    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
-    Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 1")
+    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
-    Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 1")
+    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
-    Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 2")
+    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
@@ -102,7 +110,9 @@ def check_validity!
     validator.expect(:is_a?, false, [Symbol])
     validator.expect(:is_a?, false, [String])
 
-    Topic.validates_with(validator, if: "1 == 1", foo: :bar)
+    ActiveSupport::Deprecation.silence do
+      Topic.validates_with(validator, if: "1 == 1", foo: :bar)
+    end
     assert topic.valid?
     validator.verify
   end

activesupport/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Deprecate passing string to `:if` and `:unless` conditional options
+    on `set_callback` and `skip_callback`.
+
+    *Ryuta Kamizono*
+
 *   Raise `ArgumentError` when passing string to define callback.
 
     *Ryuta Kamizono*

activesupport/lib/active_support/callbacks.rb

@@ -667,6 +667,14 @@ def __update_callbacks(name) #:nodoc:
         #   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 :if and :unless conditional options is deprecated
+              and will be removed in Rails 5.2 without replacement.
+            MSG
+          end
+
           self_chain = get_callbacks name
           mapped = filters.map do |filter|
             Callback.build(self_chain, filter, type, options)
@@ -690,6 +698,14 @@ def set_callback(name, *filter_list, &block)
         # already been set (unless the <tt>:raise</tt> option is set to <tt>false</tt>).
         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

@@ -192,10 +192,12 @@ class ConditionalPerson < Record
     before_save Proc.new { |r| r.history << [:before_save, :symbol] }, unless: :no
     before_save Proc.new { |r| r.history << "b00m" }, unless: :yes
     # string
-    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"
+    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
@@ -1204,6 +1206,17 @@ def test_skip_without_raise # removes nothing
     end
   end
 
+  class DeprecatedWarningTest < ActiveSupport::TestCase
+    def test_deprecate_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" }
+    end
+  end
+
   class NotPermittedStringCallbackTest < ActiveSupport::TestCase
     def test_passing_string_callback_is_not_permitted
       klass = Class.new(Record)