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

33 files changed, 507 insertions, 81 deletions

diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb
index 0d2f18279b..32de82780f 100644
--- a/actionpack/lib/abstract_controller/callbacks.rb
+++ b/actionpack/lib/abstract_controller/callbacks.rb
@@ -9,7 +9,7 @@ module AbstractController
 
     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
 
diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index b86e988841..d643a08235 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1 +1,12 @@
+*   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 ActiveModel or ActiveModel::Validations
+    `before_` callback had the side effect of halting the callback chain.
+    This is not recommended anymore and, depending on the value of the
+    `config.active_support.halt_callback_chains_on_return_false` option, will
+    either not work at all or display a deprecation warning.
+
+
 Please check [4-2-stable](https://github.com/rails/rails/blob/4-2-stable/activemodel/CHANGELOG.md) for previous changes.
diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb
index b3d70dc515..6214802074 100644
--- a/activemodel/lib/active_model/callbacks.rb
+++ b/activemodel/lib/active_model/callbacks.rb
@@ -6,7 +6,7 @@ module ActiveModel
   # Provides an interface for any class to have Active Record like callbacks.
   #
   # Like the Active Record methods, the callback chain is aborted as soon as
-  # one of the methods in the chain returns +false+.
+  # one of the methods throws +:abort+.
   #
   # First, extend ActiveModel::Callbacks from the class you are creating:
   #
@@ -103,7 +103,6 @@ module ActiveModel
     def define_model_callbacks(*callbacks)
       options = callbacks.extract_options!
       options = {
-        terminator: ->(_,result) { result == false },
         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 25ccabd66b..4b58ef66e3 100644
--- a/activemodel/lib/active_model/validations/callbacks.rb
+++ b/activemodel/lib/active_model/validations/callbacks.rb
@@ -15,15 +15,14 @@ module ActiveModel
     #     after_validation  :do_stuff_after_validation
     #   end
     #
-    # Like other <tt>before_*</tt> callbacks if +before_validation+ returns
-    # +false+ then <tt>valid?</tt> will not be called.
+    # Like other <tt>before_*</tt> callbacks if +before_validation+ throws
+    # +:abort+ then <tt>valid?</tt> will not be called.
     module Callbacks
       extend ActiveSupport::Concern
 
       included do
         include ActiveSupport::Callbacks
         define_callbacks :validation,
-                         terminator: ->(_,result) { result == false },
                          skip_after_callbacks_if_terminated: true,
                          scope: [:kind, :name]
       end
diff --git a/activemodel/test/cases/callbacks_test.rb b/activemodel/test/cases/callbacks_test.rb
index 2ac681b8d8..85455c112c 100644
--- a/activemodel/test/cases/callbacks_test.rb
+++ b/activemodel/test/cases/callbacks_test.rb
@@ -33,11 +33,13 @@ class CallbacksTest < ActiveModel::TestCase
     def initialize(options = {})
       @callbacks = []
       @valid = options[:valid]
-      @before_create_returns = options[:before_create_returns]
+      @before_create_returns = options.fetch(:before_create_returns, true)
+      @before_create_throws = options[:before_create_throws]
     end
 
     def before_create
       @callbacks << :before_create
+      throw(@before_create_throws) if @before_create_throws
       @before_create_returns
     end
 
@@ -62,10 +64,18 @@ class CallbacksTest < ActiveModel::TestCase
     assert_equal model.callbacks.last, :final_callback
   end
 
-  test "the callback chain is halted when a before callback returns false" do
+  test "the callback chain is halted when a before callback returns false (deprecated)" do
     model = ModelCallbacks.new(before_create_returns: false)
+    assert_deprecated do
+      model.create
+      assert_equal model.callbacks.last, :before_create
+    end
+  end
+
+  test "the callback chain is halted when a callback throws :abort" do
+    model = ModelCallbacks.new(before_create_throws: :abort)
     model.create
-    assert_equal model.callbacks.last, :before_create
+    assert_equal model.callbacks, [:before_create]
   end
 
   test "after callbacks are not executed if the block returns false" do
diff --git a/activemodel/test/cases/validations/callbacks_test.rb b/activemodel/test/cases/validations/callbacks_test.rb
index 5d6d48b824..4b0dd58efb 100644
--- a/activemodel/test/cases/validations/callbacks_test.rb
+++ b/activemodel/test/cases/validations/callbacks_test.rb
@@ -30,11 +30,16 @@ class DogWithTwoValidators < Dog
   before_validation { self.history << 'before_validation_marker2' }
 end
 
-class DogBeforeValidatorReturningFalse < Dog
+class DogDeprecatedBeforeValidatorReturningFalse < Dog
   before_validation { false }
   before_validation { self.history << 'before_validation_marker2' }
 end
 
+class DogBeforeValidatorThrowingAbort < Dog
+  before_validation { throw :abort }
+  before_validation { self.history << 'before_validation_marker2' }
+end
+
 class DogAfterValidatorReturningFalse < Dog
   after_validation { false }
   after_validation { self.history << 'after_validation_marker' }
@@ -86,13 +91,22 @@ class CallbacksWithMethodNamesShouldBeCalled < ActiveModel::TestCase
     assert_equal ['before_validation_marker1', 'before_validation_marker2'], d.history
   end
 
-  def test_further_callbacks_should_not_be_called_if_before_validation_returns_false
-    d = DogBeforeValidatorReturningFalse.new
+  def test_further_callbacks_should_not_be_called_if_before_validation_throws_abort
+    d = DogBeforeValidatorThrowingAbort.new
     output = d.valid?
     assert_equal [], d.history
     assert_equal false, output
   end
 
+  def test_deprecated_further_callbacks_should_not_be_called_if_before_validation_returns_false
+    d = DogDeprecatedBeforeValidatorReturningFalse.new
+    assert_deprecated do
+      output = d.valid?
+      assert_equal [], d.history
+      assert_equal false, output
+    end
+  end
+
   def test_further_callbacks_should_be_called_if_after_validation_returns_false
     d = DogAfterValidatorReturningFalse.new
     d.valid?
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 8980a1d874..83224c522c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,15 @@
+*   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 ActiveRecord `before_` callback had the
+    side effect of halting the callback chain.
+    This is not recommended anymore and, depending on the value of the
+    `config.active_support.halt_callback_chains_on_return_false` option, will
+    either not work at all or display a deprecation warning.
+
+    *claudiob*
+
 *   Clear query cache on rollback.
 
     *Florian Weingarten*
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index d7f655d00c..2a782c06d0 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -17,7 +17,7 @@ module ActiveRecord
           unless empty?
             record = klass.human_attribute_name(reflection.name).downcase
             owner.errors.add(:base, :"restrict_dependent_destroy.many", record: record)
-            false
+            throw(:abort)
           end
 
         else
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 74b8c53758..41a75b820e 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -13,7 +13,7 @@ module ActiveRecord
           if load_target
             record = klass.human_attribute_name(reflection.name).downcase
             owner.errors.add(:base, :"restrict_dependent_destroy.one", record: record)
-            false
+            throw(:abort)
           end
 
         else
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index c39b045a5e..fa6c5e9e8c 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -200,13 +200,19 @@ module ActiveRecord
             after_create save_method
             after_update save_method
           else
-            define_non_cyclic_method(save_method) { save_belongs_to_association(reflection) }
+            define_non_cyclic_method(save_method) { throw(:abort) if save_belongs_to_association(reflection) == false }
             before_save save_method
           end
 
           if reflection.validate? && !method_defined?(validation_method)
             method = (collection ? :validate_collection_association : :validate_single_association)
-            define_non_cyclic_method(validation_method) { send(method, reflection) }
+            define_non_cyclic_method(validation_method) do
+              send(method, reflection)
+              # 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
             validate validation_method
           end
         end
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index 497ce8c15c..f44e5af5de 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/activerecord/lib/active_record/callbacks.rb
@@ -192,14 +192,14 @@ module ActiveRecord
   #
   # == <tt>before_validation*</tt> returning statements
   #
-  # If the returning value of a +before_validation+ callback can be evaluated to +false+, the process will be
+  # If the +before_validation+ callback throws +:abort+, the process will be
   # aborted and <tt>Base#save</tt> will return +false+. If Base#save! is called it will raise a
   # ActiveRecord::RecordInvalid exception. Nothing will be appended to the errors object.
   #
   # == Canceling callbacks
   #
-  # If a <tt>before_*</tt> callback returns +false+, all the later callbacks and the associated action are
-  # cancelled. If an <tt>after_*</tt> callback returns +false+, all the later callbacks are cancelled.
+  # If a <tt>before_*</tt> callback throws +:abort+, all the later callbacks and
+  # the associated action are cancelled.
   # Callbacks are generally run in the order they are defined, with the exception of callbacks defined as
   # methods on the model, which are called last.
   #
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index f53c5f17ef..cf6673db2e 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -113,9 +113,9 @@ module ActiveRecord
     # the current time. However, if you supply <tt>touch: false</tt>, these
     # timestamps will not be updated.
     #
-    # There's a series of callbacks associated with +save+. If any of the
-    # <tt>before_*</tt> callbacks return +false+ the action is cancelled and
-    # +save+ returns +false+. See ActiveRecord::Callbacks for further
+    # There's a series of callbacks associated with #save. If any of the
+    # <tt>before_*</tt> callbacks throws +:abort+ the action is cancelled and
+    # #save returns +false+. See ActiveRecord::Callbacks for further
     # details.
     #
     # Attributes marked as readonly are silently ignored if the record is
@@ -139,9 +139,9 @@ module ActiveRecord
     # the current time. However, if you supply <tt>touch: false</tt>, these
     # timestamps will not be updated.
     #
-    # There's a series of callbacks associated with <tt>save!</tt>. If any of
-    # the <tt>before_*</tt> callbacks return +false+ the action is cancelled
-    # and <tt>save!</tt> raises ActiveRecord::RecordNotSaved. See
+    # There's a series of callbacks associated with #save!. If any of
+    # the <tt>before_*</tt> callbacks throws +:abort+ the action is cancelled
+    # and #save! raises ActiveRecord::RecordNotSaved. See
     # ActiveRecord::Callbacks for further details.
     #
     # Attributes marked as readonly are silently ignored if the record is
@@ -171,10 +171,10 @@ module ActiveRecord
     # Deletes the record in the database and freezes this instance to reflect
     # that no changes should be made (since they can't be persisted).
     #
-    # There's a series of callbacks associated with <tt>destroy</tt>. If
-    # the <tt>before_destroy</tt> callback return +false+ the action is cancelled
-    # and <tt>destroy</tt> returns +false+. See
-    # ActiveRecord::Callbacks for further details.
+    # There's a series of callbacks associated with #destroy. If the
+    # <tt>before_destroy</tt> callback throws +:abort+ the action is cancelled
+    # and #destroy returns +false+.
+    # See ActiveRecord::Callbacks for further details.
     def destroy
       raise ReadOnlyRecord, "#{self.class} is marked as readonly" if readonly?
       destroy_associations
@@ -186,10 +186,10 @@ module ActiveRecord
     # Deletes the record in the database and freezes this instance to reflect
     # that no changes should be made (since they can't be persisted).
     #
-    # There's a series of callbacks associated with <tt>destroy!</tt>. If
-    # the <tt>before_destroy</tt> callback return +false+ the action is cancelled
-    # and <tt>destroy!</tt> raises ActiveRecord::RecordNotDestroyed. See
-    # ActiveRecord::Callbacks for further details.
+    # There's a series of callbacks associated with #destroy!. If the
+    # <tt>before_destroy</tt> callback throws +:abort+ the action is cancelled
+    # and #destroy! raises ActiveRecord::RecordNotDestroyed.
+    # See ActiveRecord::Callbacks for further details.
     def destroy!
       destroy || raise(ActiveRecord::RecordNotDestroyed, self)
     end
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index de701edca0..31ca90fb58 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -17,7 +17,6 @@ module ActiveRecord
 
     included do
       define_callbacks :commit, :rollback,
-                       terminator: ->(_, result) { result == false },
                        scope: [:kind, :name]
 
       mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
diff --git a/activerecord/test/cases/attribute_test.rb b/activerecord/test/cases/attribute_test.rb
index 7b325abf1d..39a976fcc8 100644
--- a/activerecord/test/cases/attribute_test.rb
+++ b/activerecord/test/cases/attribute_test.rb
@@ -5,6 +5,7 @@ module ActiveRecord
   class AttributeTest < ActiveRecord::TestCase
     setup do
       @type = Minitest::Mock.new
+      @type.expect(:==, false, [false])
     end
 
     teardown do
diff --git a/activerecord/test/cases/callbacks_test.rb b/activerecord/test/cases/callbacks_test.rb
index d4cc081f32..670d94dc06 100644
--- a/activerecord/test/cases/callbacks_test.rb
+++ b/activerecord/test/cases/callbacks_test.rb
@@ -49,6 +49,11 @@ class CallbackDeveloperWithFalseValidation < CallbackDeveloper
   before_validation proc { |model| model.history << [:before_validation, :should_never_get_here] }
 end
 
+class CallbackDeveloperWithHaltedValidation < CallbackDeveloper
+  before_validation proc { |model| model.history << [:before_validation, :throwing_abort]; throw(:abort) }
+  before_validation proc { |model| model.history << [:before_validation, :should_never_get_here] }
+end
+
 class ParentDeveloper < ActiveRecord::Base
   self.table_name = 'developers'
   attr_accessor :after_save_called
@@ -73,6 +78,20 @@ class ImmutableDeveloper < ActiveRecord::Base
     end
 end
 
+class DeveloperWithCanceledCallbacks < ActiveRecord::Base
+  self.table_name = 'developers'
+
+  validates_inclusion_of :salary, in: 50000..200000
+
+  before_save :cancel
+  before_destroy :cancel
+
+  private
+    def cancel
+      throw(:abort)
+    end
+end
+
 class OnCallbacksDeveloper < ActiveRecord::Base
   self.table_name = 'developers'
 
@@ -136,6 +155,23 @@ class CallbackCancellationDeveloper < ActiveRecord::Base
   after_destroy { @after_destroy_called = true }
 end
 
+class CallbackHaltedDeveloper < ActiveRecord::Base
+  self.table_name = 'developers'
+
+  attr_reader   :after_save_called, :after_create_called, :after_update_called, :after_destroy_called
+  attr_accessor :cancel_before_save, :cancel_before_create, :cancel_before_update, :cancel_before_destroy
+
+  before_save    { throw(:abort) if defined?(@cancel_before_save) }
+  before_create  { throw(:abort) if @cancel_before_create  }
+  before_update  { throw(:abort) if @cancel_before_update  }
+  before_destroy { throw(:abort) if @cancel_before_destroy }
+
+  after_save    { @after_save_called    = true }
+  after_update  { @after_update_called  = true }
+  after_create  { @after_create_called  = true }
+  after_destroy { @after_destroy_called = true }
+end
+
 class CallbacksTest < ActiveRecord::TestCase
   fixtures :developers
 
@@ -393,12 +429,14 @@ class CallbacksTest < ActiveRecord::TestCase
     ], david.history
   end
 
-  def test_before_save_returning_false
+  def test_deprecated_before_save_returning_false
     david = ImmutableDeveloper.find(1)
-    assert david.valid?
-    assert !david.save
-    exc = assert_raise(ActiveRecord::RecordNotSaved) { david.save! }
-    assert_equal exc.record, david
+    assert_deprecated do
+      assert david.valid?
+      assert !david.save
+      exc = assert_raise(ActiveRecord::RecordNotSaved) { david.save! }
+      assert_equal exc.record, david
+    end
 
     david = ImmutableDeveloper.find(1)
     david.salary = 10_000_000
@@ -408,38 +446,48 @@ class CallbacksTest < ActiveRecord::TestCase
 
     someone = CallbackCancellationDeveloper.find(1)
     someone.cancel_before_save = true
-    assert someone.valid?
-    assert !someone.save
+    assert_deprecated do
+      assert someone.valid?
+      assert !someone.save
+    end
     assert_save_callbacks_not_called(someone)
   end
 
-  def test_before_create_returning_false
+  def test_deprecated_before_create_returning_false
     someone = CallbackCancellationDeveloper.new
     someone.cancel_before_create = true
-    assert someone.valid?
-    assert !someone.save
+    assert_deprecated do
+      assert someone.valid?
+      assert !someone.save
+    end
     assert_save_callbacks_not_called(someone)
   end
 
-  def test_before_update_returning_false
+  def test_deprecated_before_update_returning_false
     someone = CallbackCancellationDeveloper.find(1)
     someone.cancel_before_update = true
-    assert someone.valid?
-    assert !someone.save
+    assert_deprecated do
+      assert someone.valid?
+      assert !someone.save
+    end
     assert_save_callbacks_not_called(someone)
   end
 
-  def test_before_destroy_returning_false
+  def test_deprecated_before_destroy_returning_false
     david = ImmutableDeveloper.find(1)
-    assert !david.destroy
-    exc = assert_raise(ActiveRecord::RecordNotDestroyed) { david.destroy! }
-    assert_equal exc.record, david
+    assert_deprecated do
+      assert !david.destroy
+      exc = assert_raise(ActiveRecord::RecordNotDestroyed) { david.destroy! }
+      assert_equal exc.record, david
+    end
     assert_not_nil ImmutableDeveloper.find_by_id(1)
 
     someone = CallbackCancellationDeveloper.find(1)
     someone.cancel_before_destroy = true
-    assert !someone.destroy
-    assert_raise(ActiveRecord::RecordNotDestroyed) { someone.destroy! }
+    assert_deprecated do
+      assert !someone.destroy
+      assert_raise(ActiveRecord::RecordNotDestroyed) { someone.destroy! }
+    end
     assert !someone.after_destroy_called
   end
 
@@ -450,9 +498,59 @@ class CallbacksTest < ActiveRecord::TestCase
   end
   private :assert_save_callbacks_not_called
 
+  def test_before_create_throwing_abort
+    someone = CallbackHaltedDeveloper.new
+    someone.cancel_before_create = true
+    assert someone.valid?
+    assert !someone.save
+    assert_save_callbacks_not_called(someone)
+  end
+
+  def test_before_save_throwing_abort
+    david = DeveloperWithCanceledCallbacks.find(1)
+    assert david.valid?
+    assert !david.save
+    exc = assert_raise(ActiveRecord::RecordNotSaved) { david.save! }
+    assert_equal exc.record, david
+
+    david = DeveloperWithCanceledCallbacks.find(1)
+    david.salary = 10_000_000
+    assert !david.valid?
+    assert !david.save
+    assert_raise(ActiveRecord::RecordInvalid) { david.save! }
+
+    someone = CallbackHaltedDeveloper.find(1)
+    someone.cancel_before_save = true
+    assert someone.valid?
+    assert !someone.save
+    assert_save_callbacks_not_called(someone)
+  end
+
+  def test_before_update_throwing_abort
+    someone = CallbackHaltedDeveloper.find(1)
+    someone.cancel_before_update = true
+    assert someone.valid?
+    assert !someone.save
+    assert_save_callbacks_not_called(someone)
+  end
+
+  def test_before_destroy_throwing_abort
+    david = DeveloperWithCanceledCallbacks.find(1)
+    assert !david.destroy
+    exc = assert_raise(ActiveRecord::RecordNotDestroyed) { david.destroy! }
+    assert_equal exc.record, david
+    assert_not_nil ImmutableDeveloper.find_by_id(1)
+
+    someone = CallbackHaltedDeveloper.find(1)
+    someone.cancel_before_destroy = true
+    assert !someone.destroy
+    assert_raise(ActiveRecord::RecordNotDestroyed) { someone.destroy! }
+    assert !someone.after_destroy_called
+  end
+
   def test_callback_returning_false
     david = CallbackDeveloperWithFalseValidation.find(1)
-    david.save
+    assert_deprecated { david.save }
     assert_equal [
       [ :after_find,            :method ],
       [ :after_find,            :string ],
@@ -478,6 +576,34 @@ class CallbacksTest < ActiveRecord::TestCase
     ], david.history
   end
 
+  def test_callback_throwing_abort
+    david = CallbackDeveloperWithHaltedValidation.find(1)
+    david.save
+    assert_equal [
+      [ :after_find,        :method ],
+      [ :after_find,        :string ],
+      [ :after_find,        :proc   ],
+      [ :after_find,        :object ],
+      [ :after_find,        :block  ],
+      [ :after_initialize,  :method ],
+      [ :after_initialize,  :string ],
+      [ :after_initialize,  :proc   ],
+      [ :after_initialize,  :object ],
+      [ :after_initialize,  :block  ],
+      [ :before_validation, :method ],
+      [ :before_validation, :string ],
+      [ :before_validation, :proc   ],
+      [ :before_validation, :object ],
+      [ :before_validation, :block  ],
+      [ :before_validation, :throwing_abort ],
+      [ :after_rollback,    :block  ],
+      [ :after_rollback,    :object ],
+      [ :after_rollback,    :proc   ],
+      [ :after_rollback,    :string ],
+      [ :after_rollback,    :method ],
+    ], david.history
+  end
+
   def test_inheritance_of_callbacks
     parent = ParentDeveloper.new
     assert !parent.after_save_called
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index 0bbb4bb79e..e0aecb5996 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -194,6 +194,16 @@ class TransactionTest < ActiveRecord::TestCase
     assert_equal posts_count, author.posts(true).size
   end
 
+  def test_cancellation_from_returning_false_in_before_filter
+    def @first.before_save_for_transaction
+      false
+    end
+
+    assert_deprecated do
+      @first.save
+    end
+  end
+
   def test_cancellation_from_before_destroy_rollbacks_in_destroy
     add_cancelling_before_destroy_with_db_side_effect_to_topic @first
     nbooks_before_destroy = Book.count
@@ -650,7 +660,7 @@ class TransactionTest < ActiveRecord::TestCase
       meta = class << topic; self; end
       meta.send("define_method", "before_#{filter}_for_transaction") do
         Book.create
-        false
+        throw(:abort)
       end
     end
   end
diff --git a/activerecord/test/models/bird.rb b/activerecord/test/models/bird.rb
index dff099c1fb..2a51d903b8 100644
--- a/activerecord/test/models/bird.rb
+++ b/activerecord/test/models/bird.rb
@@ -7,6 +7,6 @@ class Bird < ActiveRecord::Base
   attr_accessor :cancel_save_from_callback
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method
-    false
+    throw(:abort)
   end
 end
diff --git a/activerecord/test/models/bulb.rb b/activerecord/test/models/bulb.rb
index 831a0d5387..a6e83fe353 100644
--- a/activerecord/test/models/bulb.rb
+++ b/activerecord/test/models/bulb.rb
@@ -46,6 +46,6 @@ end
 
 class FailedBulb < Bulb
   before_destroy do
-    false
+    throw(:abort)
   end
 end
diff --git a/activerecord/test/models/parrot.rb b/activerecord/test/models/parrot.rb
index 8c83de573f..b26035d944 100644
--- a/activerecord/test/models/parrot.rb
+++ b/activerecord/test/models/parrot.rb
@@ -11,7 +11,7 @@ class Parrot < ActiveRecord::Base
   attr_accessor :cancel_save_from_callback
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method
-    false
+    throw(:abort)
   end
 end
 
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index 641a33f9be..366c70f902 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -56,7 +56,7 @@ class Pirate < ActiveRecord::Base
   attr_accessor :cancel_save_from_callback, :parrots_limit
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method
-    false
+    throw(:abort)
   end
 
   private
diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb
index 5f618a50d2..c2f6d492d8 100644
--- a/activerecord/test/models/ship.rb
+++ b/activerecord/test/models/ship.rb
@@ -14,7 +14,7 @@ class Ship < ActiveRecord::Base
   attr_accessor :cancel_save_from_callback
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method
-    false
+    throw(:abort)
   end
 end
 
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
diff --git a/guides/source/configuring.md b/guides/source/configuring.md
index 0a375d7cb8..5baab8a4b5 100644
--- a/guides/source/configuring.md
+++ b/guides/source/configuring.md
@@ -507,6 +507,8 @@ There are a few configuration options available in Active Support:
 
 * `config.active_support.time_precision` sets the precision of JSON encoded time values. Defaults to `3`.
 
+* `config.active_support.halt_callback_chains_on_return_false` specifies whether ActiveRecord, ActiveModel and ActiveModel::Validations callback chains can be halted by returning `false` in a 'before' callback. Defaults to `true`.
+
 * `ActiveSupport::Logger.silencer` is set to `false` to disable the ability to silence logging in a block. The default is `true`.
 
 * `ActiveSupport::Cache::Store.logger` specifies the logger to use within cache store operations.
diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md
index 0b9f59bb46..c0c94475e0 100644
--- a/guides/source/upgrading_ruby_on_rails.md
+++ b/guides/source/upgrading_ruby_on_rails.md
@@ -53,6 +53,28 @@ Don't forget to review the difference, to see if there were any unexpected chang
 Upgrading from Rails 4.2 to Rails 5.0
 -------------------------------------
 
+### Halting callback chains by returning `false`
+
+In Rails 4.2, when a 'before' callback returns `false` in ActiveRecord,
+ActiveModel and ActiveModel::Validations, then the entire callback chain
+is halted. In other words, successive 'before' callbacks are not executed,
+and neither is the action wrapped in callbacks.
+
+In Rails 5.0, returning `false` in a callback will not have this side effect
+of halting the callback chain. Instead, callback chains must be explicitly
+halted by calling `throw(:abort)`.
+
+When you upgrade from Rails 4.2 to Rails 5.0, returning `false` in a callback
+will still halt the callback chain, but you will receive a deprecation warning
+about this upcoming change.
+
+When you are ready, you can opt into the new behavior and remove the deprecation
+warning by adding the following configuration to your `config/application.rb`:
+
+    config.active_support.halt_callback_chains_on_return_false = false
+
+See [#17227](https://github.com/rails/rails/pull/17227) for more details.
+
 Upgrading from Rails 4.1 to Rails 4.2
 -------------------------------------
 
diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md
index 588cea8886..5367a4dd0e 100644
--- a/railties/CHANGELOG.md
+++ b/railties/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   Add `config/initializers/callback_terminator.rb`
+
+    Newly generated Rails apps have a new initializer called
+    `callback_terminator.rb` which sets the value of the configuration option
+    `config.active_support.halt_callback_chains_on_return_false` to `false`.
+
+    As a result, new Rails apps do not halt callback chains when a callback
+    returns `false`; only when they are explicitly halted with `throw(:abort)`.
+
+    The terminator is *not* added when running `rake rails:update`, so returning
+    `false` will still work on old apps ported to Rails 5, displaying a
+    deprecation warning to prompt users to update their code to the new syntax.
+
+    *claudiob*
+
 *   Generated fixtures won't use the id when generated with references attributes.
 
     *Pablo Olmos de Aguilera Corradini*
diff --git a/railties/lib/rails/generators/rails/app/app_generator.rb b/railties/lib/rails/generators/rails/app/app_generator.rb
index 1ff1f970b5..0550bf113e 100644
--- a/railties/lib/rails/generators/rails/app/app_generator.rb
+++ b/railties/lib/rails/generators/rails/app/app_generator.rb
@@ -88,9 +88,14 @@ module Rails
 
     def config_when_updating
       cookie_serializer_config_exist = File.exist?('config/initializers/cookies_serializer.rb')
+      callback_terminator_config_exist = File.exist?('config/initializers/callback_terminator.rb')
 
       config
 
+      unless callback_terminator_config_exist
+        remove_file 'config/initializers/callback_terminator.rb'
+      end
+
       unless cookie_serializer_config_exist
         gsub_file 'config/initializers/cookies_serializer.rb', /json/, 'marshal'
       end
diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/callback_terminator.rb b/railties/lib/rails/generators/rails/app/templates/config/initializers/callback_terminator.rb
new file mode 100644
index 0000000000..e63022da91
--- /dev/null
+++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/callback_terminator.rb
@@ -0,0 +1,4 @@
+# Be sure to restart your server when you modify this file.
+
+# Do not halt callback chains when a callback returns false.
+Rails.application.config.active_support.halt_callback_chains_on_return_false = false
diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb
index ac8f735cec..e30c9a7b2f 100644
--- a/railties/test/generators/app_generator_test.rb
+++ b/railties/test/generators/app_generator_test.rb
@@ -160,6 +160,38 @@ class AppGeneratorTest < Rails::Generators::TestCase
     assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :json/)
   end
 
+  def test_rails_update_does_not_create_callback_terminator_initializer
+    app_root = File.join(destination_root, 'myapp')
+    run_generator [app_root]
+
+    FileUtils.rm("#{app_root}/config/initializers/callback_terminator.rb")
+
+    Rails.application.config.root = app_root
+    Rails.application.class.stubs(:name).returns("Myapp")
+    Rails.application.stubs(:is_a?).returns(Rails::Application)
+
+    generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell
+    generator.send(:app_const)
+    quietly { generator.send(:update_config_files) }
+    assert_no_file "#{app_root}/config/initializers/callback_terminator.rb"
+  end
+
+  def test_rails_update_does_not_remove_callback_terminator_initializer_if_already_present
+    app_root = File.join(destination_root, 'myapp')
+    run_generator [app_root]
+
+    FileUtils.touch("#{app_root}/config/initializers/callback_terminator.rb")
+
+    Rails.application.config.root = app_root
+    Rails.application.class.stubs(:name).returns("Myapp")
+    Rails.application.stubs(:is_a?).returns(Rails::Application)
+
+    generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell
+    generator.send(:app_const)
+    quietly { generator.send(:update_config_files) }
+    assert_file "#{app_root}/config/initializers/callback_terminator.rb"
+  end
+
   def test_rails_update_set_the_cookie_serializer_to_marchal_if_it_is_not_already_configured
     app_root = File.join(destination_root, 'myapp')
     run_generator [app_root]
diff --git a/railties/test/railties/engine_test.rb b/railties/test/railties/engine_test.rb
index 91cdc60bd1..4ffcf64275 100644
--- a/railties/test/railties/engine_test.rb
+++ b/railties/test/railties/engine_test.rb
@@ -498,17 +498,12 @@ YAML
       boot_rails
 
       initializers = Rails.application.initializers.tsort
-      index        = initializers.index { |i| i.name == "dummy_initializer" }
-      selection    = initializers[(index-3)..(index)].map(&:name).map(&:to_s)
+      dummy_index  = initializers.index  { |i| i.name == "dummy_initializer" }
+      config_index = initializers.rindex { |i| i.name == :load_config_initializers }
+      stack_index  = initializers.index  { |i| i.name == :build_middleware_stack }
 
-      assert_equal %w(
-       load_config_initializers
-       load_config_initializers
-       engines_blank_point
-       dummy_initializer
-      ), selection
-
-      assert index < initializers.index { |i| i.name == :build_middleware_stack }
+      assert config_index < dummy_index
+      assert dummy_index < stack_index
     end
 
     class Upcaser