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)`.
This commit is contained in:
parent
91b8129320
commit
bb78af73ab
@ -1,3 +1,11 @@
|
||||
* Deprecate returning `false` as a way to halt callback chains.
|
||||
|
||||
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)`.
|
||||
|
||||
*claudiob*
|
||||
|
||||
* Clear query cache on rollback.
|
||||
|
||||
*Florian Weingarten*
|
||||
|
@ -17,7 +17,7 @@ def handle_dependency
|
||||
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
|
||||
|
@ -13,7 +13,7 @@ def handle_dependency
|
||||
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
|
||||
|
@ -200,7 +200,7 @@ def add_autosave_association_callbacks(reflection)
|
||||
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
|
||||
|
||||
|
@ -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.
|
||||
#
|
||||
|
@ -113,9 +113,9 @@ def persisted?
|
||||
# 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 @@ def save(*args)
|
||||
# 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 @@ def delete
|
||||
# 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 @@ def destroy
|
||||
# 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
|
||||
|
@ -17,7 +17,6 @@ module Transactions
|
||||
|
||||
included do
|
||||
define_callbacks :commit, :rollback,
|
||||
terminator: ->(_, result_lambda) { result_lambda.call == false },
|
||||
scope: [:kind, :name]
|
||||
|
||||
mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
|
||||
|
@ -5,6 +5,7 @@ module ActiveRecord
|
||||
class AttributeTest < ActiveRecord::TestCase
|
||||
setup do
|
||||
@type = Minitest::Mock.new
|
||||
@type.expect(:==, false, [false])
|
||||
end
|
||||
|
||||
teardown do
|
||||
|
@ -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 @@ def cancel
|
||||
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 @@ def test_delete
|
||||
], 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 @@ def test_before_save_returning_false
|
||||
|
||||
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 @@ def assert_save_callbacks_not_called(someone)
|
||||
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 @@ def test_callback_returning_false
|
||||
], 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
|
||||
|
@ -194,6 +194,16 @@ def test_update_should_rollback_on_failure!
|
||||
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 @@ def test_transactions_state_from_commit
|
||||
meta = class << topic; self; end
|
||||
meta.send("define_method", "before_#{filter}_for_transaction") do
|
||||
Book.create
|
||||
false
|
||||
throw(:abort)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -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
|
||||
|
@ -46,6 +46,6 @@ class FunkyBulb < Bulb
|
||||
|
||||
class FailedBulb < Bulb
|
||||
before_destroy do
|
||||
false
|
||||
throw(:abort)
|
||||
end
|
||||
end
|
||||
|
@ -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
|
||||
|
||||
|
@ -56,7 +56,7 @@ def reject_empty_ships_on_create(attributes)
|
||||
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
|
||||
|
@ -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
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user