Deprecate `false` as the way to halt AM validation callbacks

Before this commit, returning `false` in an ActiveModel validation
callback such as `before_validation` would halt the callback chain.

After this commit, the behavior is deprecated: will still work until
the next release of Rails but will also display a deprecation warning.

The preferred way to halt a callback chain is to explicitly `throw(:abort)`.

3 files changed, 28 insertions, 6 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index b86e988841..562d49d069 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1 +1,10 @@
+*   Deprecate returning `false` as a way to halt callback chains.
+
+    Returning `false` in a `before_` validation callback will display a
+    deprecation warning explaining that the preferred method to halt a callback
+    chain is to explicitly `throw(:abort)`.
+
+    *claudiob*
+
+
 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/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb
index 449f1a0299..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_lambda) { result_lambda.call == false },
                          skip_after_callbacks_if_terminated: true,
                          scope: [:kind, :name]
       end
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?