Deprecate passing string to define callback.

8 files changed, 31 insertions, 23 deletions

diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb
index d5317e4717..d63ce9c1c3 100644
--- a/actionpack/lib/abstract_controller/callbacks.rb
+++ b/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
diff --git a/activemodel/test/cases/callbacks_test.rb b/activemodel/test/cases/callbacks_test.rb
index 85455c112c..e4ecc0adb4 100644
--- a/activemodel/test/cases/callbacks_test.rb
+++ b/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 = []
diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb
index 03c7943308..c73580138d 100644
--- a/activemodel/test/cases/validations/with_validation_test.rb
+++ b/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?
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index 4058affec3..854f9776a3 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/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
diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb
index 73ac30e547..4f70ae3a1d 100644
--- a/activerecord/test/cases/callbacks_test.rb
+++ b/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] }
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index d946fc1bbd..da81e83a51 100644
--- a/activesupport/CHANGELOG.md
+++ b/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` 
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index d43fde03a9..bf560ec1fa 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/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+.
       #
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb
index 3b00ff87a0..a624473f46 100644
--- a/activesupport/test/callbacks_test.rb
+++ b/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