Merge pull request #17227 from claudiob/explicitly-abort-callbacks

Introduce explicit way of halting callback chains by throwing :abort. Deprecate current implicit behavior of halting callback chains by returning `false` in apps ported to Rails 5.0. Completely remove that behavior in brand new Rails 5.0 apps.

Conflicts:
	railties/CHANGELOG.md

5 files changed, 175 insertions, 11 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 3606d7e572..f59ad476ae 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,44 @@
+*   Change the way in which callback chains can be halted.
+
+    The preferred method to halt a callback chain from now on is to explicitly
+    `throw(:abort)`.
+    In the past, returning `false` in an ActiveSupport callback had the side
+    effect of halting the callback chain. This is not recommended anymore and,
+    depending on the value of
+    `Callbacks::CallbackChain.halt_and_display_warning_on_return_false`, will
+    either not work at all or display a deprecation warning.
+
+
+*   Add Callbacks::CallbackChain.halt_and_display_warning_on_return_false
+
+    Setting `Callbacks::CallbackChain.halt_and_display_warning_on_return_false`
+    to true will let an app support the deprecated way of halting callback
+    chains by returning `false`.
+
+    Setting the value to false will tell the app to ignore any `false` value
+    returned by callbacks, and only halt the chain upon `throw(:abort)`.
+
+    The value can also be set with the Rails configuration option
+    `config.active_support.halt_callback_chains_on_return_false`.
+
+    When the configuration option is missing, its value is `true`, so older apps
+    ported to Rails 5.0 will not break (but display a deprecation warning).
+    For new Rails 5.0 apps, its value is set to `false` in an initializer, so
+    these apps will support the new behavior by default.
+
+    *claudiob*
+
+*   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.
+
+    *claudiob*
+
 *   Deprecate `MissingSourceFile` in favor of `LoadError`.
 
     `MissingSourceFile` was just an alias to `LoadError` and was not being
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index d2911a254c..0f1de8b076 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -4,6 +4,7 @@ require 'active_support/core_ext/array/extract_options'
 require 'active_support/core_ext/class/attribute'
 require 'active_support/core_ext/kernel/reporting'
 require 'active_support/core_ext/kernel/singleton_class'
+require 'active_support/core_ext/string/filters'
 require 'thread'
 
 module ActiveSupport
@@ -142,8 +143,8 @@ module ActiveSupport
             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 +162,9 @@ module ActiveSupport
             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
@@ -514,10 +516,17 @@ module ActiveSupport
 
       attr_reader :name, :config
 
+      # If true, any callback returning +false+ will halt the entire callback
+      # chain and display a deprecation message. If false, callback chains will
+      # only be halted by calling +throw :abort+. Defaults to +true+.
+      class_attribute :halt_and_display_warning_on_return_false
+      self.halt_and_display_warning_on_return_false = true
+
       def initialize(name, config)
         @name = name
         @config = {
-          :scope => [ :kind ]
+          scope: [:kind],
+          terminator: default_terminator
         }.merge!(config)
         @chain = []
         @callbacks = nil
@@ -588,6 +597,28 @@ module ActiveSupport
         @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 = result_lambda.call if result_lambda.is_a?(Proc)
+            if halt_and_display_warning_on_return_false && result == false
+              display_deprecation_warning_for_false_terminator
+            else
+              terminate = false
+            end
+          end
+          terminate
+        end
+      end
+
+      def display_deprecation_warning_for_false_terminator
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          Returning `false` in a callback will not implicitly halt a callback chain in the next release of Rails.
+          To explicitly halt a callback chain, please use `throw :abort` instead.
+        MSG
+      end
     end
 
     module ClassMethods
diff --git a/activesupport/lib/active_support/i18n_railtie.rb b/activesupport/lib/active_support/i18n_railtie.rb
index affcfb7398..9e742b1917 100644
--- a/activesupport/lib/active_support/i18n_railtie.rb
+++ b/activesupport/lib/active_support/i18n_railtie.rb
@@ -55,7 +55,13 @@ module I18n
 
       reloader = ActiveSupport::FileUpdateChecker.new(I18n.load_path.dup){ I18n.reload! }
       app.reloaders << reloader
-      ActionDispatch::Reloader.to_prepare { reloader.execute_if_updated }
+      ActionDispatch::Reloader.to_prepare do
+        reloader.execute_if_updated
+        # TODO: remove the following line as soon as the return value of
+        # callbacks is ignored, that is, returning `false` does not
+        # display a deprecation warning or halts the callback chain.
+        true
+      end
       reloader.execute
 
       @i18n_inited = true
diff --git a/activesupport/lib/active_support/railtie.rb b/activesupport/lib/active_support/railtie.rb
index 133aa6a054..6eba24b569 100644
--- a/activesupport/lib/active_support/railtie.rb
+++ b/activesupport/lib/active_support/railtie.rb
@@ -13,6 +13,13 @@ module ActiveSupport
       end
     end
 
+    initializer "active_support.halt_callback_chains_on_return_false", after: :load_config_initializers do |app|
+      if app.config.active_support.key? :halt_callback_chains_on_return_false
+        ActiveSupport::Callbacks::CallbackChain.halt_and_display_warning_on_return_false = \
+          app.config.active_support.halt_callback_chains_on_return_false
+      end
+    end
+
     # Sets the default value for Time.zone
     # If assigned value cannot be matched to a TimeZone, an exception will be raised.
     initializer "active_support.initialize_time_zone" do |app|
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb
index d19e5fd6e7..f6abef8cee 100644
--- a/activesupport/test/callbacks_test.rb
+++ b/activesupport/test/callbacks_test.rb
@@ -511,8 +511,6 @@ module CallbacksTest
       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,38 @@ module CallbacksTest
   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 CallbackFalseTerminator < AbstractCallbackTerminator
+    define_callbacks :save
+
+    def second
+      @history << "second"
+      false
+    end
+
+    set_save_callbacks
+  end
+
   class CallbackObject
     def before(caller)
       caller.record << "before"
@@ -701,7 +721,7 @@ module CallbacksTest
     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 +745,65 @@ module CallbacksTest
     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 CallbackFalseTerminatorWithoutConfigTest < ActiveSupport::TestCase
+    def test_returning_false_halts_callback_if_config_variable_is_not_set
+      obj = CallbackFalseTerminator.new
+      assert_deprecated do
+        obj.save
+        assert_equal :second, obj.halted
+        assert !obj.saved
+      end
+    end
+  end
+
+  class CallbackFalseTerminatorWithConfigTrueTest < ActiveSupport::TestCase
+    def setup
+      ActiveSupport::Callbacks::CallbackChain.halt_and_display_warning_on_return_false = true
+    end
+
+    def test_returning_false_halts_callback_if_config_variable_is_true
+      obj = CallbackFalseTerminator.new
+      assert_deprecated do
+        obj.save
+        assert_equal :second, obj.halted
+        assert !obj.saved
+      end
+    end
+  end
+
+  class CallbackFalseTerminatorWithConfigFalseTest < ActiveSupport::TestCase
+    def setup
+      ActiveSupport::Callbacks::CallbackChain.halt_and_display_warning_on_return_false = false
+    end
+
+    def test_returning_false_does_not_halt_callback_if_config_variable_is_false
+      obj = CallbackFalseTerminator.new
+      obj.save
+      assert_equal nil, obj.halted
+      assert obj.saved
+    end
+  end
+
   class HyphenatedKeyTest < ActiveSupport::TestCase
     def test_save
       obj = HyphenatedCallbacks.new