Throw :abort halts default CallbackChains

This commit changes arguments and default value of CallbackChain's :terminator
option.

After this commit, Chains of callbacks defined **without** an explicit
`:terminator` option will be halted as soon as a `before_` callback throws
`:abort`.

Chains of callbacks defined **with** a `:terminator` option will maintain their
existing behavior of halting as soon as a `before_` callback matches the
terminator's expectation. For instance, ActiveModel's callbacks will still
halt the chain when a `before_` callback returns `false`.

actionpack/lib/abstract_controller/callbacks.rb

@@ -9,7 +9,7 @@ module Callbacks
 
     included do
       define_callbacks :process_action,
-                       terminator: ->(controller,_) { controller.response_body },
+                       terminator: ->(controller, result_lambda) { result_lambda.call if result_lambda.is_a?(Proc); controller.response_body },
                        skip_after_callbacks_if_terminated: true
     end
 

activemodel/lib/active_model/callbacks.rb

@@ -103,7 +103,7 @@ def self.extended(base) #:nodoc:
     def define_model_callbacks(*callbacks)
       options = callbacks.extract_options!
       options = {
-        terminator: ->(_,result) { result == false },
+        terminator: ->(_,result_lambda) { result_lambda.call == false },
         skip_after_callbacks_if_terminated: true,
         scope: [:kind, :name],
         only: [:before, :around, :after]

activemodel/lib/active_model/validations/callbacks.rb

@@ -23,7 +23,7 @@ module Callbacks
       included do
         include ActiveSupport::Callbacks
         define_callbacks :validation,
-                         terminator: ->(_,result) { result == false },
+                         terminator: ->(_,result_lambda) { result_lambda.call == false },
                          skip_after_callbacks_if_terminated: true,
                          scope: [:kind, :name]
       end

activerecord/lib/active_record/transactions.rb

@@ -17,7 +17,7 @@ module Transactions
 
     included do
       define_callbacks :commit, :rollback,
-                       terminator: ->(_, result) { result == false },
+                       terminator: ->(_, result_lambda) { result_lambda.call == false },
                        scope: [:kind, :name]
 
       mattr_accessor :raise_in_transactional_callbacks, instance_writer: false

activesupport/CHANGELOG.md

+*   Changes arguments and default value of CallbackChain's :terminator option
+
+    Chains of callbacks defined without an explicit `:terminator` option will
+    now be halted as soon as a `before_` callback throws `:abort`.
+
+    Chains of callbacks defined with a `:terminator` option will maintain their
+    existing behavior of halting as soon as a `before_` callback matches the
+    terminator's expectation. For instance, ActiveModel's callbacks will still
+    halt the chain when a `before_` callback returns `false`.
+
+    *claudiob*
+
 *   Deprecate `MissingSourceFile` in favor of `LoadError`.
 
     `MissingSourceFile` was just an alias to `LoadError` and was not being

activesupport/lib/active_support/callbacks.rb

@@ -142,8 +142,8 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio
             halted = env.halted
 
             if !halted && user_conditions.all? { |c| c.call(target, value) }
-              result = user_callback.call target, value
-              env.halted = halted_lambda.call(target, result)
+              result_lambda = -> { user_callback.call target, value }
+              env.halted = halted_lambda.call(target, result_lambda)
               if env.halted
                 target.send :halted_callback_hook, filter
               end
@@ -161,8 +161,9 @@ def self.halting(callback_sequence, user_callback, halted_lambda, filter)
             halted = env.halted
 
             unless halted
-              result = user_callback.call target, value
-              env.halted = halted_lambda.call(target, result)
+              result_lambda = -> { user_callback.call target, value }
+              env.halted = halted_lambda.call(target, result_lambda)
+
               if env.halted
                 target.send :halted_callback_hook, filter
               end
@@ -517,7 +518,8 @@ class CallbackChain #:nodoc:#
       def initialize(name, config)
         @name = name
         @config = {
-          :scope => [ :kind ]
+          scope: [:kind],
+          terminator: default_terminator
         }.merge!(config)
         @chain = []
         @callbacks = nil
@@ -588,6 +590,17 @@ def remove_duplicates(callback)
         @callbacks = nil
         @chain.delete_if { |c| callback.duplicates?(c) }
       end
+
+      def default_terminator
+        Proc.new do |target, result_lambda|
+          terminate = true
+          catch(:abort) do
+            result_lambda.call if result_lambda.is_a?(Proc)
+            terminate = false
+          end
+          terminate
+        end
+      end
     end
 
     module ClassMethods

activesupport/test/callbacks_test.rb

@@ -511,8 +511,6 @@ def self.set_save_callbacks
       set_callback :save, :before, :third
       set_callback :save, :after, :first
       set_callback :save, :around, :around_it
-      set_callback :save, :after, :second
-      set_callback :save, :around, :around_it
       set_callback :save, :after, :third
     end
 
@@ -552,16 +550,27 @@ def halted_callback_hook(filter)
   end
 
   class CallbackTerminator < AbstractCallbackTerminator
-    define_callbacks :save, terminator: ->(_,result) { result == :halt }
+    define_callbacks :save, terminator: ->(_, result_lambda) { result_lambda.call == :halt }
     set_save_callbacks
   end
 
   class CallbackTerminatorSkippingAfterCallbacks < AbstractCallbackTerminator
-    define_callbacks :save, terminator: ->(_,result) { result == :halt },
+    define_callbacks :save, terminator: ->(_, result_lambda) { result_lambda.call == :halt },
                             skip_after_callbacks_if_terminated: true
     set_save_callbacks
   end
 
+  class CallbackDefaultTerminator < AbstractCallbackTerminator
+    define_callbacks :save
+
+    def second
+      @history << "second"
+      throw(:abort)
+    end
+
+    set_save_callbacks
+  end
+
   class CallbackObject
     def before(caller)
       caller.record << "before"
@@ -701,7 +710,7 @@ class CallbackTerminatorTest < ActiveSupport::TestCase
     def test_termination_skips_following_before_and_around_callbacks
       terminator = CallbackTerminator.new
       terminator.save
-      assert_equal ["first", "second", "third", "second", "first"], terminator.history
+      assert_equal ["first", "second", "third", "first"], terminator.history
     end
 
     def test_termination_invokes_hook
@@ -725,6 +734,26 @@ def test_termination_skips_after_callbacks
     end
   end
 
+  class CallbackDefaultTerminatorTest < ActiveSupport::TestCase
+    def test_default_termination
+      terminator = CallbackDefaultTerminator.new
+      terminator.save
+      assert_equal ["first", "second", "third", "first"], terminator.history
+    end
+
+    def test_default_termination_invokes_hook
+      terminator = CallbackDefaultTerminator.new
+      terminator.save
+      assert_equal :second, terminator.halted
+    end
+
+    def test_block_never_called_if_abort_is_thrown
+      obj = CallbackDefaultTerminator.new
+      obj.save
+      assert !obj.saved
+    end
+  end
+
   class HyphenatedKeyTest < ActiveSupport::TestCase
     def test_save
       obj = HyphenatedCallbacks.new