Remove deprecated behavior that halts callbacks when the return is false

activemodel/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Remove deprecated behavior that halts callbacks when the return is false.
+
+    *Rafael Mendonça França*
+
 *   Remove unused `ActiveModel::TestCase` class.
 
     *Yuji Yaginuma*

activemodel/lib/active_model/callbacks.rb

@@ -103,7 +103,6 @@ def self.extended(base) #:nodoc:
     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]

activemodel/lib/active_model/validations/callbacks.rb

@@ -23,7 +23,6 @@ module Callbacks
       included do
         include ActiveSupport::Callbacks
         define_callbacks :validation,
-                         terminator: deprecated_false_terminator,
                          skip_after_callbacks_if_terminated: true,
                          scope: [:kind, :name]
       end

activemodel/test/cases/callbacks_test.rb

@@ -63,12 +63,10 @@ def create
     assert_equal model.callbacks.last, :final_callback
   end
 
-  test "the callback chain is halted when a before callback returns false (deprecated)" do
+  test "the callback chain is not halted when a before callback returns false)" do
     model = ModelCallbacks.new(before_create_returns: false)
-    assert_deprecated do
-      model.create
-      assert_equal model.callbacks.last, :before_create
-    end
+    model.create
+    assert_equal model.callbacks.last, :final_callback
   end
 
   test "the callback chain is halted when a callback throws :abort" do
@@ -127,6 +125,7 @@ class Violin2 < Violin
   test "after_create callbacks with both callbacks declared in one line" do
     assert_equal ["callback1", "callback2"], Violin1.new.create.history
   end
+
   test "after_create callbacks with both callbacks declared in different lines" do
     assert_equal ["callback1", "callback2"], Violin2.new.create.history
   end

activemodel/test/cases/validations/callbacks_test.rb

@@ -29,7 +29,7 @@ class DogWithTwoValidators < Dog
   before_validation { history << "before_validation_marker2" }
 end
 
-class DogDeprecatedBeforeValidatorReturningFalse < Dog
+class DogBeforeValidatorReturningFalse < Dog
   before_validation { false }
   before_validation { history << "before_validation_marker2" }
 end
@@ -121,13 +121,11 @@ def test_further_callbacks_should_not_be_called_if_before_validation_throws_abor
     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
+  def test_further_callbacks_should_be_called_if_before_validation_returns_false
+    d = DogBeforeValidatorReturningFalse.new
+    output = d.valid?
+    assert_equal ["before_validation_marker2"], d.history
+    assert_equal true, output
   end
 
   def test_further_callbacks_should_be_called_if_after_validation_returns_false

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Remove deprecated behavior that halts callbacks when the return is false.
+
+    *Rafael Mendonça França*
+
 *   Deprecate `ColumnDumper#migration_keys`.
 
     *Ryuta Kamizono*

activerecord/lib/active_record/transactions.rb

@@ -11,7 +11,6 @@ module Transactions
                        :before_commit_without_transaction_enrollment,
                        :commit_without_transaction_enrollment,
                        :rollback_without_transaction_enrollment,
-                       terminator: deprecated_false_terminator,
                        scope: [:kind, :name]
     end
 

activerecord/test/cases/callbacks_test.rb

@@ -39,11 +39,6 @@ def history
   end
 end
 
-class CallbackDeveloperWithFalseValidation < CallbackDeveloper
-  before_validation proc { |model| model.history << [:before_validation, :returning_false]; false }
-  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] }
@@ -132,23 +127,6 @@ def history
   end
 end
 
-class CallbackCancellationDeveloper < 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    { defined?(@cancel_before_save) ? !@cancel_before_save : false }
-  before_create  { !@cancel_before_create  }
-  before_update  { !@cancel_before_update  }
-  before_destroy { !@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 CallbackHaltedDeveloper < ActiveRecord::Base
   self.table_name = "developers"
 
@@ -404,70 +382,6 @@ def test_delete
     ], david.history
   end
 
-  def test_deprecated_before_save_returning_false
-    david = ImmutableDeveloper.find(1)
-    assert_deprecated do
-      assert david.valid?
-      assert !david.save
-      exc = assert_raise(ActiveRecord::RecordNotSaved) { david.save! }
-      assert_equal david, exc.record
-      assert_equal "Failed to save the record", exc.message
-    end
-
-    david = ImmutableDeveloper.find(1)
-    david.salary = 10_000_000
-    assert !david.valid?
-    assert !david.save
-    assert_raise(ActiveRecord::RecordInvalid) { david.save! }
-
-    someone = CallbackCancellationDeveloper.find(1)
-    someone.cancel_before_save = true
-    assert_deprecated do
-      assert someone.valid?
-      assert !someone.save
-    end
-    assert_save_callbacks_not_called(someone)
-  end
-
-  def test_deprecated_before_create_returning_false
-    someone = CallbackCancellationDeveloper.new
-    someone.cancel_before_create = true
-    assert_deprecated do
-      assert someone.valid?
-      assert !someone.save
-    end
-    assert_save_callbacks_not_called(someone)
-  end
-
-  def test_deprecated_before_update_returning_false
-    someone = CallbackCancellationDeveloper.find(1)
-    someone.cancel_before_update = true
-    assert_deprecated do
-      assert someone.valid?
-      assert !someone.save
-    end
-    assert_save_callbacks_not_called(someone)
-  end
-
-  def test_deprecated_before_destroy_returning_false
-    david = ImmutableDeveloper.find(1)
-    assert_deprecated do
-      assert !david.destroy
-      exc = assert_raise(ActiveRecord::RecordNotDestroyed) { david.destroy! }
-      assert_equal david, exc.record
-      assert_equal "Failed to destroy the record", exc.message
-    end
-    assert_not_nil ImmutableDeveloper.find_by_id(1)
-
-    someone = CallbackCancellationDeveloper.find(1)
-    someone.cancel_before_destroy = true
-    assert_deprecated do
-      assert !someone.destroy
-      assert_raise(ActiveRecord::RecordNotDestroyed) { someone.destroy! }
-    end
-    assert !someone.after_destroy_called
-  end
-
   def assert_save_callbacks_not_called(someone)
     assert !someone.after_save_called
     assert !someone.after_create_called
@@ -525,30 +439,6 @@ def test_before_destroy_throwing_abort
     assert !someone.after_destroy_called
   end
 
-  def test_callback_returning_false
-    david = CallbackDeveloperWithFalseValidation.find(1)
-    assert_deprecated { david.save }
-    assert_equal [
-      [ :after_find,            :method ],
-      [ :after_find,            :proc   ],
-      [ :after_find,            :object ],
-      [ :after_find,            :block  ],
-      [ :after_initialize,            :method ],
-      [ :after_initialize,            :proc   ],
-      [ :after_initialize,            :object ],
-      [ :after_initialize,            :block  ],
-      [ :before_validation,           :method ],
-      [ :before_validation,           :proc   ],
-      [ :before_validation,           :object ],
-      [ :before_validation,           :block  ],
-      [ :before_validation, :returning_false  ],
-      [ :after_rollback, :block  ],
-      [ :after_rollback, :object ],
-      [ :after_rollback, :proc   ],
-      [ :after_rollback, :method ],
-    ], david.history
-  end
-
   def test_callback_throwing_abort
     david = CallbackDeveloperWithHaltedValidation.find(1)
     david.save

activerecord/test/cases/transactions_test.rb

@@ -206,16 +206,6 @@ def test_update_should_rollback_on_failure!
     assert_equal posts_count, author.posts.reload.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

activesupport/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Remove deprecated behavior that halts callbacks when the return is false.
+
+    *Rafael Mendonça França*
+
 *   Deprecate passing string to `:if` and `:unless` conditional options
     on `set_callback` and `skip_callback`.
 

activesupport/lib/active_support/callbacks.rb

@@ -856,30 +856,6 @@ def get_callbacks(name) # :nodoc:
           def set_callbacks(name, callbacks) # :nodoc:
             self.__callbacks = __callbacks.merge(name.to_sym => callbacks)
           end
-
-          def deprecated_false_terminator # :nodoc:
-            Proc.new do |target, result_lambda|
-              terminate = true
-              catch(:abort) do
-                result = result_lambda.call if result_lambda.is_a?(Proc)
-                if Callbacks.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 Rails 5.1.
-              To explicitly halt the callback chain, please use `throw :abort` instead.
-            MSG
-          end
       end
   end
 end

activesupport/test/callbacks_test.rb

@@ -869,34 +869,8 @@ def test_block_never_called_if_abort_is_thrown
     end
   end
 
-  class CallbackFalseTerminatorWithoutConfigTest < ActiveSupport::TestCase
-    def test_returning_false_does_not_halt_callback_if_config_variable_is_not_set
-      obj = CallbackFalseTerminator.new
-      obj.save
-      assert_nil obj.halted
-      assert obj.saved
-    end
-  end
-
-  class CallbackFalseTerminatorWithConfigTrueTest < ActiveSupport::TestCase
-    def setup
-      ActiveSupport::Callbacks.halt_and_display_warning_on_return_false = true
-    end
-
-    def test_returning_false_does_not_halt_callback_if_config_variable_is_true
-      obj = CallbackFalseTerminator.new
-      obj.save
-      assert_nil obj.halted
-      assert obj.saved
-    end
-  end
-
-  class CallbackFalseTerminatorWithConfigFalseTest < ActiveSupport::TestCase
-    def setup
-      ActiveSupport::Callbacks.halt_and_display_warning_on_return_false = false
-    end
-
-    def test_returning_false_does_not_halt_callback_if_config_variable_is_false
+  class CallbackFalseTerminatorTest < ActiveSupport::TestCase
+    def test_returning_false_does_not_halt_callback
       obj = CallbackFalseTerminator.new
       obj.save
       assert_nil obj.halted