Merge pull request #22598 from yui-knk/deprecate_string_callback

Deprecate passing string to define callback.

actionpack/lib/abstract_controller/callbacks.rb

@@ -48,7 +48,8 @@ module AbstractController
 
       def _normalize_callback_option(options, from, to) # :nodoc:
         if from = options[from]
-          from = Array(from).map {|o| "action_name == '#{o}'"}.join(" || ")
+          _from = Array(from).map(&:to_s).to_set
+          from = proc {|c| _from.include? c.action_name }
           options[to] = Array(options[to]).unshift(from)
         end
       end

activemodel/test/cases/callbacks_test.rb

@@ -28,7 +28,7 @@ class CallbacksTest < ActiveModel::TestCase
       false
     end
 
-    after_create "@callbacks << :final_callback"
+    ActiveSupport::Deprecation.silence { after_create "@callbacks << :final_callback" }
 
     def initialize(options = {})
       @callbacks = []

activemodel/test/cases/validations/with_validation_test.rb

@@ -101,6 +101,7 @@ class ValidatesWithTest < ActiveModel::TestCase
     validator.expect(:new, validator, [{foo: :bar, if: "1 == 1", class: Topic}])
     validator.expect(:validate, nil, [topic])
     validator.expect(:is_a?, false, [Symbol])
+    validator.expect(:is_a?, false, [String])
 
     Topic.validates_with(validator, if: "1 == 1", foo: :bar)
     assert topic.valid?

activerecord/lib/active_record/callbacks.rb

@@ -175,21 +175,6 @@ module ActiveRecord
   #       end
   #   end
   #
-  # The callback macros usually accept a symbol for the method they're supposed to run, but you can also
-  # pass a "method string", which will then be evaluated within the binding of the callback. Example:
-  #
-  #   class Topic < ActiveRecord::Base
-  #     before_destroy 'self.class.delete_all "parent_id = #{id}"'
-  #   end
-  #
-  # Notice that single quotes (') are used so the <tt>#{id}</tt> part isn't evaluated until the callback
-  # is triggered. Also note that these inline callbacks can be stacked just like the regular ones:
-  #
-  #   class Topic < ActiveRecord::Base
-  #     before_destroy 'self.class.delete_all "parent_id = #{id}"',
-  #                    'puts "Evaluated after parents are destroyed"'
-  #   end
-  #
   # == <tt>before_validation*</tt> returning statements
   #
   # If the +before_validation+ callback throws +:abort+, the process will be

activerecord/test/cases/callbacks_test.rb

@@ -33,7 +33,7 @@ class CallbackDeveloper < ActiveRecord::Base
   ActiveRecord::Callbacks::CALLBACKS.each do |callback_method|
     next if callback_method.to_s =~ /^around_/
     define_callback_method(callback_method)
-    send(callback_method, callback_string(callback_method))
+    ActiveSupport::Deprecation.silence { send(callback_method, callback_string(callback_method)) }
     send(callback_method, callback_proc(callback_method))
     send(callback_method, callback_object(callback_method))
     send(callback_method) { |model| model.history << [callback_method, :block] }

activesupport/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Deprecate passing string to define callback.
+
+    *Yuichiro Kaneko*
+
 *   `ActiveSupport::Cache::Store#namespaced_key`, 
     `ActiveSupport::Cache::MemCachedStore#escape_key`, and 
     `ActiveSupport::Cache::FileStore#key_file_path` 

activesupport/lib/active_support/callbacks.rb

@@ -295,6 +295,13 @@ 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.
+          MSG
+        end
+
         new chain.name, filter, kind, options, chain.config
       end
 
@@ -575,7 +582,7 @@ module ActiveSupport
       #   set_callback :save, :before_meth
       #
       # 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; or as an
+      # 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+.
       #

activesupport/test/callbacks_test.rb

@@ -59,7 +59,7 @@ 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))
-      send(callback_method, callback_string(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)
@@ -228,7 +228,7 @@ module CallbacksTest
     set_callback :save, :before, :nope,           :if =>     :no
     set_callback :save, :before, :nope,           :unless => :yes
     set_callback :save, :after,  :tweedle
-    set_callback :save, :before, "tweedle_dee"
+    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 }
@@ -1046,7 +1046,7 @@ module CallbacksTest
 
     def test_add_eval
       calls = []
-      klass = build_class("bar")
+      klass = ActiveSupport::Deprecation.silence { build_class("bar") }
       klass.class_eval { define_method(:bar) { calls << klass } }
       klass.new.run
       assert_equal 1, calls.length
@@ -1086,7 +1086,7 @@ module CallbacksTest
 
     def test_skip_string # raises error
       calls = []
-      klass = build_class("bar")
+      klass =  ActiveSupport::Deprecation.silence { build_class("bar") }
       klass.class_eval { define_method(:bar) { calls << klass } }
       assert_raises(ArgumentError) { klass.skip "bar" }
       klass.new.run
@@ -1111,4 +1111,14 @@ module CallbacksTest
       assert_equal 1, calls.length
     end
   end
+
+  class DeprecatedWarningTest < ActiveSupport::TestCase
+    def test_deprecate_string_callback
+      klass = Class.new(Record)
+
+      assert_deprecated do
+        klass.send :before_save, "tweedle_dee"
+      end
+    end
+  end
 end