From 8ddc91bc546f7585736e654c428700d795f645e3 Mon Sep 17 00:00:00 2001 From: Mike Burns Date: Wed, 7 May 2014 09:24:10 +0200 Subject: [PATCH] Raise instead of returning false in AR callbacks We never check whether a `#destroy` fails in our controllers: def destroy article = Article.find(params[:id]) article.destroy redirect_to articles_url end Because of this history, it would cause confusion and potential bugs if we introduced a normal `before_destroy` into any of our models. Not only will we need to check the result of `#destroy` for any controller using that model, but we'd also need to check the return value for any controller using models that associate with that model. Since callbacks are used for validated database objects and are expected to succeed, we should raise on failure. This will roll back the transaction and notify the exception handler (e.g. Airbrake). This is true of all callbacks, not just `before_destroy`. --- best-practices/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/best-practices/README.md b/best-practices/README.md index 9429de34..c8c6a0a7 100644 --- a/best-practices/README.md +++ b/best-practices/README.md @@ -69,6 +69,8 @@ Rails * Don't change a migration after it has been merged into master if the desired change can be solved with another migration. * Don't reference a model class directly from a view. +* Don't return false from `ActiveModel` callbacks, but instead raise an + exception. * Don't use instance variables in partials. Pass local variables to partials from view templates. * Don't use SQL or SQL fragments (`where('inviter_id IS NOT NULL')`) outside of