Merge pull request #27608 from kamipo/remove_deprecated_passing_string_to_define_callback

Remove deprecated passing string to define callback

3 files changed, 68 insertions, 58 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 5af97e3d37..afc5c5d78b 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   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*
+
 *   Updated Unicode version to 9.0.0
 
     Now we can handle new emojis such like "👩‍👩‍👧‍👦" ("\u{1F469}\u{200D}\u{1F469}\u{200D}\u{1F467}\u{200D}\u{1F466}").
@@ -60,76 +69,76 @@
 
     *Yuji Yaginuma*
 
-*   Remove deprecated class `ActiveSupport::Concurrency::Latch`
+*   Remove deprecated class `ActiveSupport::Concurrency::Latch`.
 
     *Andrew White*
 
-*   Remove deprecated separator argument from `parameterize`
+*   Remove deprecated separator argument from `parameterize`.
 
     *Andrew White*
 
-*   Remove deprecated method `Numeric#to_formatted_s`
+*   Remove deprecated method `Numeric#to_formatted_s`.
 
     *Andrew White*
 
-*   Remove deprecated method `alias_method_chain`
+*   Remove deprecated method `alias_method_chain`.
 
     *Andrew White*
 
-*   Remove deprecated constant `MissingSourceFile`
+*   Remove deprecated constant `MissingSourceFile`.
 
     *Andrew White*
 
 *   Remove deprecated methods `Module.qualified_const_defined?`,
-    `Module.qualified_const_get` and `Module.qualified_const_set`
+    `Module.qualified_const_get` and `Module.qualified_const_set`.
 
     *Andrew White*
 
-*   Remove deprecated `:prefix` option from `number_to_human_size`
+*   Remove deprecated `:prefix` option from `number_to_human_size`.
 
     *Andrew White*
 
-*   Remove deprecated method `ActiveSupport::HashWithIndifferentAccess.new_from_hash_copying_default`
+*   Remove deprecated method `ActiveSupport::HashWithIndifferentAccess.new_from_hash_copying_default`.
 
     *Andrew White*
 
-*   Remove deprecated file `active_support/core_ext/time/marshal.rb`
+*   Remove deprecated file `active_support/core_ext/time/marshal.rb`.
 
     *Andrew White*
 
-*   Remove deprecated file `active_support/core_ext/struct.rb`
+*   Remove deprecated file `active_support/core_ext/struct.rb`.
 
     *Andrew White*
 
-*   Remove deprecated file `active_support/core_ext/module/method_transplanting.rb`
+*   Remove deprecated file `active_support/core_ext/module/method_transplanting.rb`.
 
     *Andrew White*
 
-*   Remove deprecated method `Module.local_constants`
+*   Remove deprecated method `Module.local_constants`.
 
     *Andrew White*
 
-*   Remove deprecated file `active_support/core_ext/kernel/debugger.rb`
+*   Remove deprecated file `active_support/core_ext/kernel/debugger.rb`.
 
     *Andrew White*
 
-*   Remove deprecated method `ActiveSupport::Cache::Store#namespaced_key`
+*   Remove deprecated method `ActiveSupport::Cache::Store#namespaced_key`.
 
     *Andrew White*
 
-*   Remove deprecated method `ActiveSupport::Cache::Strategy::LocalCache::LocalStore#set_cache_value`
+*   Remove deprecated method `ActiveSupport::Cache::Strategy::LocalCache::LocalStore#set_cache_value`.
 
     *Andrew White*
 
-*   Remove deprecated method `ActiveSupport::Cache::MemCacheStore#escape_key`
+*   Remove deprecated method `ActiveSupport::Cache::MemCacheStore#escape_key`.
 
     *Andrew White*
 
-*   Remove deprecated method `ActiveSupport::Cache::FileStore#key_file_path`
+*   Remove deprecated method `ActiveSupport::Cache::FileStore#key_file_path`.
 
     *Andrew White*
 
-*   Ensure duration parsing is consistent across DST changes
+*   Ensure duration parsing is consistent across DST changes.
 
     Previously `ActiveSupport::Duration.parse` used `Time.current` and
     `Time#advance` to calculate the number of seconds in the duration
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index e6c79f2a38..b3d3f3caec 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -286,9 +286,8 @@ module ActiveSupport
       class Callback #:nodoc:#
         def self.build(chain, filter, kind, options)
           if filter.is_a?(String)
-            ActiveSupport::Deprecation.warn(<<-MSG.squish)
-              Passing string to define callback is deprecated and will be removed
-              in Rails 5.1 without replacement.
+            raise ArgumentError, <<-MSG.squish
+              Passing string to define callback is not supported. Use proc instead.
             MSG
           end
 
@@ -643,9 +642,8 @@ module ActiveSupport
         #   set_callback :save, :before_method
         #
         # The callback can be specified as a symbol naming an instance method; as a
-        # proc, lambda, or block; as a string to be instance evaluated(deprecated); or as an
-        # object that responds to a certain method determined by the <tt>:scope</tt>
-        # argument to +define_callbacks+.
+        # proc, lambda, or block; or as an object that responds to a certain method
+        # determined by the <tt>:scope</tt> argument to +define_callbacks+.
         #
         # If a proc, lambda, or block is given, its body is evaluated in the context
         # of the current object. It can also optionally accept the current object as
@@ -669,6 +667,14 @@ module ActiveSupport
         #   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)
@@ -692,6 +698,14 @@ module ActiveSupport
         # 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|
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb
index 28caa30bf1..ed26081d4e 100644
--- a/activesupport/test/callbacks_test.rb
+++ b/activesupport/test/callbacks_test.rb
@@ -23,10 +23,6 @@ module CallbacksTest
         method_name
       end
 
-      def callback_string(callback_method)
-        "history << [#{callback_method.to_sym.inspect}, :string]"
-      end
-
       def callback_proc(callback_method)
         Proc.new { |model| model.history << [callback_method, :proc] }
       end
@@ -61,7 +57,6 @@ module CallbacksTest
     [:before_save, :after_save].each do |callback_method|
       callback_method_sym = callback_method.to_sym
       send(callback_method, callback_symbol(callback_method_sym))
-      ActiveSupport::Deprecation.silence { send(callback_method, callback_string(callback_method_sym)) }
       send(callback_method, callback_proc(callback_method_sym))
       send(callback_method, callback_object(callback_method_sym.to_s.gsub(/_save/, "")))
       send(callback_method, CallbackClass)
@@ -197,10 +192,12 @@ module CallbacksTest
     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
@@ -272,7 +269,6 @@ module CallbacksTest
     set_callback :save, :before, :nope,           if: :no
     set_callback :save, :before, :nope,           unless: :yes
     set_callback :save, :after,  :tweedle
-    ActiveSupport::Deprecation.silence { set_callback :save, :before, "tweedle_dee" }
     set_callback :save, :before, proc { |m| m.history << "yup" }
     set_callback :save, :before, :nope,           if: proc { false }
     set_callback :save, :before, :nope,           unless: proc { true }
@@ -302,10 +298,6 @@ module CallbacksTest
       yield
     end
 
-    def tweedle_dee
-      @history << "tweedle dee"
-    end
-
     def tweedle_dum
       @history << "tweedle dum pre"
       yield
@@ -421,7 +413,6 @@ module CallbacksTest
       around = AroundPerson.new
       around.save
       assert_equal [
-        "tweedle dee",
         "yup", "yup",
         "tweedle dum pre",
         "w0tyes before",
@@ -540,7 +531,6 @@ module CallbacksTest
       assert_equal [], person.history
       person.save
       assert_equal [
-        [:before_save, :string],
         [:before_save, :proc],
         [:before_save, :object],
         [:before_save, :block],
@@ -548,7 +538,6 @@ module CallbacksTest
         [:after_save, :class],
         [:after_save, :object],
         [:after_save, :proc],
-        [:after_save, :string],
         [:after_save, :symbol]
       ], person.history
     end
@@ -567,7 +556,6 @@ module CallbacksTest
         [:after_save, :class],
         [:after_save, :object],
         [:after_save, :proc],
-        [:after_save, :string],
         [:after_save, :symbol]
       ], person.history
     end
@@ -580,7 +568,6 @@ module CallbacksTest
       person.save
       assert_equal [
         [:before_save, :symbol],
-        [:before_save, :string],
         [:before_save, :proc],
         [:before_save, :object],
         [:before_save, :class],
@@ -589,7 +576,6 @@ module CallbacksTest
         [:after_save, :class],
         [:after_save, :object],
         [:after_save, :proc],
-        [:after_save, :string],
         [:after_save, :symbol]
       ], person.history
     end
@@ -939,7 +925,6 @@ module CallbacksTest
       writer.save
       assert_equal [
         [:before_save, :symbol],
-        [:before_save, :string],
         [:before_save, :proc],
         [:before_save, :object],
         [:before_save, :class],
@@ -948,7 +933,6 @@ module CallbacksTest
         [:after_save, :class],
         [:after_save, :object],
         [:after_save, :proc],
-        [:after_save, :string],
         [:after_save, :symbol]
       ], writer.history
     end
@@ -1162,14 +1146,6 @@ module CallbacksTest
       assert_equal 1, calls.length
     end
 
-    def test_add_eval
-      calls = []
-      klass = ActiveSupport::Deprecation.silence { build_class("bar") }
-      klass.class_eval { define_method(:bar) { calls << klass } }
-      klass.new.run
-      assert_equal 1, calls.length
-    end
-
     def test_skip_class # removes one at a time
       calls = []
       callback = Class.new {
@@ -1204,7 +1180,7 @@ module CallbacksTest
 
     def test_skip_string # raises error
       calls = []
-      klass = ActiveSupport::Deprecation.silence { build_class("bar") }
+      klass = build_class(:bar)
       klass.class_eval { define_method(:bar) { calls << klass } }
       assert_raises(ArgumentError) { klass.skip "bar" }
       klass.new.run
@@ -1231,11 +1207,22 @@ module CallbacksTest
   end
 
   class DeprecatedWarningTest < ActiveSupport::TestCase
-    def test_deprecate_string_callback
+    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)
 
-      assert_deprecated do
-        klass.send :before_save, "tweedle_dee"
+      assert_raises(ArgumentError) do
+        klass.before_save "tweedle"
       end
     end
   end