Raise ArgumentError if an unrecognised callback is skipped

At present, if you skip a callback that hasn't been defined,
activesupport callbacks silently does nothing. However, it's easy to
mistype the name of a callback and mistakenly think that it's being
skipped, when it is not.

This problem even exists in the current test suite.
CallbacksTest::SkipCallbacksTest#test_skip_person attempts to skip
callbacks that were never set up.

This PR changes `skip_callback` to raise an `ArgumentError` if the
specified callback cannot be found.

4 files changed, 46 insertions, 15 deletions

diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb
index 59ffb0a19e..13795f0dd8 100644
--- a/actionpack/lib/abstract_controller/callbacks.rb
+++ b/actionpack/lib/abstract_controller/callbacks.rb
@@ -62,9 +62,9 @@ module AbstractController
       #   using #skip_action_callback
       def skip_action_callback(*names)
         ActiveSupport::Deprecation.warn('`skip_action_callback` is deprecated and will be removed in the next major version of Rails. Please use skip_before_action, skip_after_action or skip_around_action instead.')
-        skip_before_action(*names)
-        skip_after_action(*names)
-        skip_around_action(*names)
+        skip_before_action(*names, raise: false)
+        skip_after_action(*names, raise: false)
+        skip_around_action(*names, raise: false)
       end
 
       def skip_filter(*names)
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 7eaad6340f..3ad2392365 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   `ActiveSupport::Callbacks#skip_callback` now raises an `ArgumentError` if
+    an unrecognized callback is removed.
+
+    *Iain Beeston*
+
 *   Added `ActiveSupport::ArrayInquirer` and `Array#inquiry`.
 
     Wrapping an array in an `ArrayInquirer` gives a friendlier way to check its
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index 9cf09ab266..19d2b1099d 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -691,19 +691,27 @@ module ActiveSupport
       #   class Writer < Person
       #      skip_callback :validate, :before, :check_membership, if: -> { self.age > 18 }
       #   end
+      #
+      # An <tt>ArgumentError</tt> will be raised if the callback has not
+      # 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)
+        options[:raise] = true unless options.key?(:raise)
 
         __update_callbacks(name) do |target, chain|
           filters.each do |filter|
-            filter = chain.find {|c| c.matches?(type, filter) }
+            callback = chain.find {|c| c.matches?(type, filter) }
+
+            if !callback && options[:raise]
+              raise ArgumentError, "#{type.to_s.capitalize} #{name} callback #{filter.inspect} has not been defined"
+            end
 
-            if filter && options.any?
-              new_filter = filter.merge_conditional_options(chain, if_option: options[:if], unless_option: options[:unless])
-              chain.insert(chain.index(filter), new_filter)
+            if callback && (options.key?(:if) || options.key?(:unless))
+              new_callback = callback.merge_conditional_options(chain, if_option: options[:if], unless_option: options[:unless])
+              chain.insert(chain.index(callback), new_callback)
             end
 
-            chain.delete(filter)
+            chain.delete(callback)
           end
           target.set_callbacks name, chain
         end
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb
index f6abef8cee..cda9732cae 100644
--- a/activesupport/test/callbacks_test.rb
+++ b/activesupport/test/callbacks_test.rb
@@ -73,8 +73,8 @@ module CallbacksTest
 
   class PersonSkipper < Person
     skip_callback :save, :before, :before_save_method, :if => :yes
-    skip_callback :save, :after, :before_save_method, :unless => :yes
-    skip_callback :save, :after, :before_save_method, :if => :no
+    skip_callback :save, :after,  :after_save_method, :unless => :yes
+    skip_callback :save, :after,  :after_save_method, :if => :no
     skip_callback :save, :before, :before_save_method, :unless => :no
     skip_callback :save, :before, CallbackClass , :if => :yes
     def yes; true; end
@@ -1021,7 +1021,7 @@ module CallbacksTest
         define_callbacks :foo
         n.times { set_callback :foo, :before, callback }
         def run; run_callbacks :foo; end
-        def self.skip(thing); skip_callback :foo, :before, thing; end
+        def self.skip(*things); skip_callback :foo, :before, *things; end
       }
     end
 
@@ -1070,11 +1070,11 @@ module CallbacksTest
       }
     end
 
-    def test_skip_lambda # removes nothing
+    def test_skip_lambda # raises error
       calls = []
       callback = ->(o) { calls << o }
       klass = build_class(callback)
-      10.times { klass.skip callback }
+      assert_raises(ArgumentError) { klass.skip callback }
       klass.new.run
       assert_equal 10, calls.length
     end
@@ -1088,11 +1088,29 @@ module CallbacksTest
       assert_equal 0, calls.length
     end
 
-    def test_skip_eval # removes nothing
+    def test_skip_string # raises error
       calls = []
       klass = build_class("bar")
       klass.class_eval { define_method(:bar) { calls << klass } }
-      klass.skip "bar"
+      assert_raises(ArgumentError) { klass.skip "bar" }
+      klass.new.run
+      assert_equal 1, calls.length
+    end
+
+    def test_skip_undefined_callback # raises error
+      calls = []
+      klass = build_class(:bar)
+      klass.class_eval { define_method(:bar) { calls << klass } }
+      assert_raises(ArgumentError) { klass.skip :qux }
+      klass.new.run
+      assert_equal 1, calls.length
+    end
+
+    def test_skip_without_raise # removes nothing
+      calls = []
+      klass = build_class(:bar)
+      klass.class_eval { define_method(:bar) { calls << klass } }
+      klass.skip :qux, raise: false
       klass.new.run
       assert_equal 1, calls.length
     end