aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md8
-rw-r--r--activerecord/lib/active_record/associations/has_many_association.rb2
-rw-r--r--activerecord/lib/active_record/associations/has_one_association.rb2
-rw-r--r--activerecord/lib/active_record/autosave_association.rb2
-rw-r--r--activerecord/lib/active_record/callbacks.rb6
-rw-r--r--activerecord/lib/active_record/persistence.rb28
-rw-r--r--activerecord/lib/active_record/transactions.rb1
-rw-r--r--activerecord/test/cases/attribute_test.rb1
-rw-r--r--activerecord/test/cases/callbacks_test.rb166
-rw-r--r--activerecord/test/cases/transactions_test.rb12
-rw-r--r--activerecord/test/models/bird.rb2
-rw-r--r--activerecord/test/models/bulb.rb2
-rw-r--r--activerecord/test/models/parrot.rb2
-rw-r--r--activerecord/test/models/pirate.rb2
-rw-r--r--activerecord/test/models/ship.rb2
15 files changed, 191 insertions, 47 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 8980a1d874..60313a0a02 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -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*
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index d7f655d00c..2a782c06d0 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -17,7 +17,7 @@ module ActiveRecord
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
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 74b8c53758..41a75b820e 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -13,7 +13,7 @@ module ActiveRecord
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
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 5045e3065e..fa6c5e9e8c 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -200,7 +200,7 @@ module ActiveRecord
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
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index 497ce8c15c..f44e5af5de 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/activerecord/lib/active_record/callbacks.rb
@@ -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.
#
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index f53c5f17ef..cf6673db2e 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -113,9 +113,9 @@ module ActiveRecord
# 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 @@ module ActiveRecord
# 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 @@ module ActiveRecord
# 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 @@ module ActiveRecord
# 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
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 95e9a8646e..31ca90fb58 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -17,7 +17,6 @@ module ActiveRecord
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
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
generated by cgit v1.2.3 (git 2.25.1) at 2024-07-03 22:49:07 +0000