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.

1 files changed, 13 insertions, 5 deletions

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