Merge pull request #21218 from repinel/fix-as-callback-terminator

WIP: Fix the AS::Callbacks terminator regression from 4.2.3

7 files changed, 45 insertions, 36 deletions

diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb
index 2cf39b68fb..0d6a3dc52d 100644
--- a/activemodel/lib/active_model/callbacks.rb
+++ b/activemodel/lib/active_model/callbacks.rb
@@ -103,6 +103,7 @@ module ActiveModel
     def define_model_callbacks(*callbacks)
       options = callbacks.extract_options!
       options = {
+        terminator: deprecated_false_terminator,
         skip_after_callbacks_if_terminated: true,
         scope: [:kind, :name],
         only: [:before, :around, :after]
diff --git a/activemodel/lib/active_model/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb
index 4b58ef66e3..52111e5442 100644
--- a/activemodel/lib/active_model/validations/callbacks.rb
+++ b/activemodel/lib/active_model/validations/callbacks.rb
@@ -23,6 +23,7 @@ module ActiveModel
       included do
         include ActiveSupport::Callbacks
         define_callbacks :validation,
+                         terminator: deprecated_false_terminator,
                          skip_after_callbacks_if_terminated: true,
                          scope: [:kind, :name]
       end
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 4a569fc242..1a2988ea77 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -11,6 +11,7 @@ module ActiveRecord
                        :before_commit_without_transaction_enrollment,
                        :commit_without_transaction_enrollment,
                        :rollback_without_transaction_enrollment,
+                       terminator: deprecated_false_terminator,
                        scope: [:kind, :name]
     end
 
diff --git a/activerecord/test/cases/attribute_test.rb b/activerecord/test/cases/attribute_test.rb
index 0ec368f51d..c69782d93f 100644
--- a/activerecord/test/cases/attribute_test.rb
+++ b/activerecord/test/cases/attribute_test.rb
@@ -4,7 +4,6 @@ module ActiveRecord
   class AttributeTest < ActiveRecord::TestCase
     setup do
       @type = Minitest::Mock.new
-      @type.expect(:==, false, [false])
     end
 
     teardown do
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 69413fef47..309d842da1 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -277,20 +277,17 @@
 
     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.
+    In the past, callbacks could only be halted by explicitly providing a
+    terminator and by having a callback match the conditions of the terminator.
 
 *   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`.
+    to `true` will let an app support the deprecated way of halting Active Record,
+    Active Model and Active Model validations 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)`.
+    returned by those 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`.
@@ -300,7 +297,7 @@
     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*
+    *claudiob*, *Roque Pinel*
 
 *   Changes arguments and default value of CallbackChain's `:terminator` option
 
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index 80c5fdba17..3db9ea2ac0 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -536,23 +536,12 @@ module ActiveSupport
         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
+            result_lambda.call if result_lambda.is_a?(Proc)
+            terminate = false
           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
@@ -686,7 +675,8 @@ module ActiveSupport
       #
       #   In this example, if any before validate callbacks returns +false+,
       #   any successive before and around callback is not executed.
-      #   Defaults to +false+, meaning no value halts the chain.
+      #
+      #   The default terminator halts the chain when a callback throws +:abort+.
       #
       # * <tt>:skip_after_callbacks_if_terminated</tt> - Determines if after
       #   callbacks should be terminated by the <tt>:terminator</tt> option. By
@@ -764,6 +754,30 @@ module ActiveSupport
       def set_callbacks(name, callbacks)
         send "_#{name}_callbacks=", callbacks
       end
+
+      def deprecated_false_terminator
+        Proc.new do |target, result_lambda|
+          terminate = true
+          catch(:abort) do
+            result = result_lambda.call if result_lambda.is_a?(Proc)
+            if CallbackChain.halt_and_display_warning_on_return_false && result == false
+              display_deprecation_warning_for_false_terminator
+            else
+              terminate = false
+            end
+          end
+          terminate
+        end
+      end
+
+      private
+
+      def display_deprecation_warning_for_false_terminator
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          Returning `false` in Active Record and Active Model callbacks will not implicitly halt a callback chain in the next release of Rails.
+          To explicitly halt the callback chain, please use `throw :abort` instead.
+        MSG
+      end
     end
   end
 end
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb
index cda9732cae..4f47e0519f 100644
--- a/activesupport/test/callbacks_test.rb
+++ b/activesupport/test/callbacks_test.rb
@@ -766,13 +766,11 @@ module CallbacksTest
   end
 
   class CallbackFalseTerminatorWithoutConfigTest < ActiveSupport::TestCase
-    def test_returning_false_halts_callback_if_config_variable_is_not_set
+    def test_returning_false_does_not_halt_callback_if_config_variable_is_not_set
       obj = CallbackFalseTerminator.new
-      assert_deprecated do
-        obj.save
-        assert_equal :second, obj.halted
-        assert !obj.saved
-      end
+      obj.save
+      assert_equal nil, obj.halted
+      assert obj.saved
     end
   end
 
@@ -781,13 +779,11 @@ module CallbacksTest
       ActiveSupport::Callbacks::CallbackChain.halt_and_display_warning_on_return_false = true
     end
 
-    def test_returning_false_halts_callback_if_config_variable_is_true
+    def test_returning_false_does_not_halt_callback_if_config_variable_is_true
       obj = CallbackFalseTerminator.new
-      assert_deprecated do
-        obj.save
-        assert_equal :second, obj.halted
-        assert !obj.saved
-      end
+      obj.save
+      assert_equal nil, obj.halted
+      assert obj.saved
     end
   end