Skip to content

+ ActiveRecord::Base#destroy! #6629

Merged
merged 1 commit into from Jun 6, 2012

3 participants

@marcandre

save! and variants are super useful when we have some generic exception handler setup, for example with rescue_from, and we expect the operation to succeed.

destroy actions would benefit the same way of a destroy! version in the same way (especially since they so often succeed).

Doesn't it sound great too? destroy!

@josevalim josevalim and 1 other commented on an outdated diff Jun 5, 2012
activerecord/lib/active_record/persistence.rb
@@ -130,6 +135,17 @@ def destroy
freeze
end
+ # 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::RecordNotSaved. See
+ # ActiveRecord::Callbacks for further details.
+ def destroy!
+ destroy or raise ActiveRecord::RecordNotDestroyed
@josevalim
Ruby on Rails member
josevalim added a note Jun 5, 2012

Can we use ||? It is in Rails Code Guidelines to not use and and or. Thanks!

@marcandre
marcandre added a note Jun 5, 2012

Sure, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on the diff Jun 5, 2012
activerecord/test/cases/callbacks_test.rb
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! }
@carlosantoniodasilva
Ruby on Rails member

I think these assertions should go to another test in persistence_test instead, they make more sense there where things like save! are already being tested, wdyt?

@marcandre
marcandre added a note Jun 5, 2012

Currently, the test for destroy in persistence_test doesn't test failures. This is only done in callback_test. I added the exact same corresponding tests for destroy!.

Moreover, there is no accessible model that cancels destruction in the callbacks; these are inlined in callback_test.

If you want, I can:
1) Move some model out of callback_test
2) Move some destroy testing out of callback_test into persistence_test
3) Move the corresponding destroy! testing.

These behavior seem pretty intertwined to me (as only callbacks can make destroy return false, unless I'm mistaken), so I think that as long as the test is in either file we're ok.

Thanks

@carlosantoniodasilva
Ruby on Rails member

I don't think it's necessary to do all this moving, I just thought that'd be reasonable to have destroy! closer to save! tests. About callbacks, one could return false from the destroy method itself without touching the callback chain. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marcandre

BTW, the commit also supplements the rdoc for destroy, mentioning callbacks and return value of false.

@carlosantoniodasilva
Ruby on Rails member

Great, can we have a changelog entry as well please?

@marcandre

Changelog entry added.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jun 6, 2012
activerecord/lib/active_record/persistence.rb
@@ -130,6 +135,17 @@ def destroy
freeze
end
+ # 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::RecordNotSaved. See
@carlosantoniodasilva
Ruby on Rails member

Ops, just noticed the constant name is wrong here, should be ActiveRecord::RecordNotDestroyed, right?

@marcandre
marcandre added a note Jun 6, 2012

Ah, the pitfalls of copy-paste :-)
Fixed, thanks.

@carlosantoniodasilva
Ruby on Rails member

Hehe :), thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva merged commit e638c61 into rails:master Jun 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.