From 91b8129320522f664801f122daea4a7628db90a7 Mon Sep 17 00:00:00 2001 From: claudiob Date: Mon, 8 Dec 2014 06:53:24 -0800 Subject: [PATCH] Deprecate `false` as the way to halt AM callbacks Before this commit, returning `false` in an ActiveModel `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)`. --- activemodel/CHANGELOG.md | 2 +- activemodel/lib/active_model/callbacks.rb | 3 +-- activemodel/test/cases/callbacks_test.rb | 16 +++++++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md index 562d49d069..b52ec8a67f 100644 --- a/activemodel/CHANGELOG.md +++ b/activemodel/CHANGELOG.md @@ -1,6 +1,6 @@ * Deprecate returning `false` as a way to halt callback chains. - Returning `false` in a `before_` validation callback will display a + Returning `false` in a `before_` callback will display a deprecation warning explaining that the preferred method to halt a callback chain is to explicitly `throw(:abort)`. diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index 061d96a80b..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_lambda) { result_lambda.call == false }, skip_after_callbacks_if_terminated: true, scope: [:kind, :name], only: [:before, :around, :after] 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