Deprecate `false` as the way to halt AR callbacks

Before this commit, returning `false` in an ActiveRecord `before_` callback
such as `before_create` 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)`.

8 files changed, 163 insertions, 26 deletions

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..7fa8eeaa77 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