Merge pull request #27608 from kamipo/remove_deprecated_passing_string_to_define_callback

Remove deprecated passing string to define callback

7 files changed, 99 insertions, 116 deletions

diff --git a/activemodel/test/cases/callbacks_test.rb b/activemodel/test/cases/callbacks_test.rb
index 63b6c56f8c..a6f0948376 100644
--- a/activemodel/test/cases/callbacks_test.rb
+++ b/activemodel/test/cases/callbacks_test.rb
@@ -27,7 +27,7 @@ class CallbacksTest < ActiveModel::TestCase
       false
     end
 
-    ActiveSupport::Deprecation.silence { after_create "@callbacks << :final_callback" }
+    after_create { |model| model.callbacks << :final_callback }
 
     def initialize(options = {})
       @callbacks = []
diff --git a/activemodel/test/cases/validations/conditional_validation_test.rb b/activemodel/test/cases/validations/conditional_validation_test.rb
index 4881008017..048d27446e 100644
--- a/activemodel/test/cases/validations/conditional_validation_test.rb
+++ b/activemodel/test/cases/validations/conditional_validation_test.rb
@@ -43,7 +43,9 @@ class ConditionalValidationTest < ActiveModel::TestCase
 
   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,7 +54,9 @@ class ConditionalValidationTest < ActiveModel::TestCase
 
   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]
@@ -60,7 +64,9 @@ class ConditionalValidationTest < ActiveModel::TestCase
 
   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]
@@ -68,7 +74,9 @@ class ConditionalValidationTest < ActiveModel::TestCase
 
   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 @@ class ConditionalValidationTest < ActiveModel::TestCase
   # 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"
diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb
index 20c11dd852..5ce86738cd 100644
--- a/activemodel/test/cases/validations/with_validation_test.rb
+++ b/activemodel/test/cases/validations/with_validation_test.rb
@@ -69,26 +69,34 @@ class ValidatesWithTest < ActiveModel::TestCase
   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 @@ class ValidatesWithTest < ActiveModel::TestCase
     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
diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb
index 53ff037de1..1de5309bd3 100644
--- a/activerecord/test/cases/callbacks_test.rb
+++ b/activerecord/test/cases/callbacks_test.rb
@@ -6,10 +6,6 @@ class CallbackDeveloper < ActiveRecord::Base
   self.table_name = "developers"
 
   class << self
-    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
@@ -33,7 +29,6 @@ class CallbackDeveloper < ActiveRecord::Base
   ActiveRecord::Callbacks::CALLBACKS.each do |callback_method|
     next if callback_method.to_s.start_with?("around_")
     define_callback_method(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] }
@@ -178,7 +173,6 @@ class CallbacksTest < ActiveRecord::TestCase
     david = CallbackDeveloper.new
     assert_equal [
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
@@ -189,12 +183,10 @@ class CallbacksTest < ActiveRecord::TestCase
     david = CallbackDeveloper.find(1)
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
@@ -206,17 +198,14 @@ class CallbacksTest < ActiveRecord::TestCase
     david.valid?
     assert_equal [
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_validation,           :method ],
-      [ :before_validation,           :string ],
       [ :before_validation,           :proc   ],
       [ :before_validation,           :object ],
       [ :before_validation,           :block  ],
       [ :after_validation,            :method ],
-      [ :after_validation,            :string ],
       [ :after_validation,            :proc   ],
       [ :after_validation,            :object ],
       [ :after_validation,            :block  ],
@@ -228,22 +217,18 @@ class CallbacksTest < ActiveRecord::TestCase
     david.valid?
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_validation,           :method ],
-      [ :before_validation,           :string ],
       [ :before_validation,           :proc   ],
       [ :before_validation,           :object ],
       [ :before_validation,           :block  ],
       [ :after_validation,            :method ],
-      [ :after_validation,            :string ],
       [ :after_validation,            :proc   ],
       [ :after_validation,            :object ],
       [ :after_validation,            :block  ],
@@ -254,44 +239,36 @@ class CallbacksTest < ActiveRecord::TestCase
     david = CallbackDeveloper.create("name" => "David", "salary" => 1000000)
     assert_equal [
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_validation,           :method ],
-      [ :before_validation,           :string ],
       [ :before_validation,           :proc   ],
       [ :before_validation,           :object ],
       [ :before_validation,           :block  ],
       [ :after_validation,            :method ],
-      [ :after_validation,            :string ],
       [ :after_validation,            :proc   ],
       [ :after_validation,            :object ],
       [ :after_validation,            :block  ],
       [ :before_save,                 :method ],
-      [ :before_save,                 :string ],
       [ :before_save,                 :proc   ],
       [ :before_save,                 :object ],
       [ :before_save,                 :block  ],
       [ :before_create,               :method ],
-      [ :before_create,               :string ],
       [ :before_create,               :proc   ],
       [ :before_create,               :object ],
       [ :before_create,               :block  ],
       [ :after_create,                :method ],
-      [ :after_create,                :string ],
       [ :after_create,                :proc   ],
       [ :after_create,                :object ],
       [ :after_create,                :block  ],
       [ :after_save,                  :method ],
-      [ :after_save,                  :string ],
       [ :after_save,                  :proc   ],
       [ :after_save,                  :object ],
       [ :after_save,                  :block  ],
       [ :after_commit,                :block  ],
       [ :after_commit,                :object ],
       [ :after_commit,                :proc   ],
-      [ :after_commit,                :string ],
       [ :after_commit,                :method ]
     ], david.history
   end
@@ -323,49 +300,40 @@ class CallbacksTest < ActiveRecord::TestCase
     david.save
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_validation,           :method ],
-      [ :before_validation,           :string ],
       [ :before_validation,           :proc   ],
       [ :before_validation,           :object ],
       [ :before_validation,           :block  ],
       [ :after_validation,            :method ],
-      [ :after_validation,            :string ],
       [ :after_validation,            :proc   ],
       [ :after_validation,            :object ],
       [ :after_validation,            :block  ],
       [ :before_save,                 :method ],
-      [ :before_save,                 :string ],
       [ :before_save,                 :proc   ],
       [ :before_save,                 :object ],
       [ :before_save,                 :block  ],
       [ :before_update,               :method ],
-      [ :before_update,               :string ],
       [ :before_update,               :proc   ],
       [ :before_update,               :object ],
       [ :before_update,               :block  ],
       [ :after_update,                :method ],
-      [ :after_update,                :string ],
       [ :after_update,                :proc   ],
       [ :after_update,                :object ],
       [ :after_update,                :block  ],
       [ :after_save,                  :method ],
-      [ :after_save,                  :string ],
       [ :after_save,                  :proc   ],
       [ :after_save,                  :object ],
       [ :after_save,                  :block  ],
       [ :after_commit,                :block  ],
       [ :after_commit,                :object ],
       [ :after_commit,                :proc   ],
-      [ :after_commit,                :string ],
       [ :after_commit,                :method ]
     ], david.history
   end
@@ -399,29 +367,24 @@ class CallbacksTest < ActiveRecord::TestCase
     david.destroy
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_destroy,              :method ],
-      [ :before_destroy,              :string ],
       [ :before_destroy,              :proc   ],
       [ :before_destroy,              :object ],
       [ :before_destroy,              :block  ],
       [ :after_destroy,               :method ],
-      [ :after_destroy,               :string ],
       [ :after_destroy,               :proc   ],
       [ :after_destroy,               :object ],
       [ :after_destroy,               :block  ],
       [ :after_commit,                :block  ],
       [ :after_commit,                :object ],
       [ :after_commit,                :proc   ],
-      [ :after_commit,                :string ],
       [ :after_commit,                :method ]
     ], david.history
   end
@@ -431,12 +394,10 @@ class CallbacksTest < ActiveRecord::TestCase
     CallbackDeveloper.delete(david.id)
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
@@ -569,17 +530,14 @@ class CallbacksTest < ActiveRecord::TestCase
     assert_deprecated { david.save }
     assert_equal [
       [ :after_find,            :method ],
-      [ :after_find,            :string ],
       [ :after_find,            :proc   ],
       [ :after_find,            :object ],
       [ :after_find,            :block  ],
       [ :after_initialize,            :method ],
-      [ :after_initialize,            :string ],
       [ :after_initialize,            :proc   ],
       [ :after_initialize,            :object ],
       [ :after_initialize,            :block  ],
       [ :before_validation,           :method ],
-      [ :before_validation,           :string ],
       [ :before_validation,           :proc   ],
       [ :before_validation,           :object ],
       [ :before_validation,           :block  ],
@@ -587,7 +545,6 @@ class CallbacksTest < ActiveRecord::TestCase
       [ :after_rollback, :block  ],
       [ :after_rollback, :object ],
       [ :after_rollback, :proc   ],
-      [ :after_rollback, :string ],
       [ :after_rollback, :method ],
     ], david.history
   end
@@ -597,17 +554,14 @@ class CallbacksTest < ActiveRecord::TestCase
     david.save
     assert_equal [
       [ :after_find,        :method ],
-      [ :after_find,        :string ],
       [ :after_find,        :proc   ],
       [ :after_find,        :object ],
       [ :after_find,        :block  ],
       [ :after_initialize,  :method ],
-      [ :after_initialize,  :string ],
       [ :after_initialize,  :proc   ],
       [ :after_initialize,  :object ],
       [ :after_initialize,  :block  ],
       [ :before_validation, :method ],
-      [ :before_validation, :string ],
       [ :before_validation, :proc   ],
       [ :before_validation, :object ],
       [ :before_validation, :block  ],
@@ -615,7 +569,6 @@ class CallbacksTest < ActiveRecord::TestCase
       [ :after_rollback,    :block  ],
       [ :after_rollback,    :object ],
       [ :after_rollback,    :proc   ],
-      [ :after_rollback,    :string ],
       [ :after_rollback,    :method ],
     ], david.history
   end
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