Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

explicit callback cancellation instead of implicit based on return value

#6621

I was recently bit by a bug where User records were silently not being saved. Here's a mini example of the sort of error I had:

class User < ActiveRecord::Base

  before_create :foo

  def foo
    if self.age > 30
      self.completed_survey = self.prequalified
    else
      self.completed_survey = true
    end
  end

end

# this one saves
user = User.create(:age => 15, :prequalified => false)
# this one also saves
user = User.create(:age => 35, :prequalified => true)
# this one does not save!
user = User.create(:age => 35, :prequalified => false)

Can you spot the error? The title of the bug gives it away. If the user is over 30 and prequalified is false, foo will accidentally return false and returning false from a before_create callback cancels the save. Also, using User.create instead of User.create! meant no exception was raised, so it went unnoticed for awhile.

This ticket is a proposal to change the interface so that save cancellation is explicit rather than implicit. Something like self.abort_save! could raise a cancel exception. The old interface could be slowly deprecated by warning when false is returned.

If this code was written, would it be accepted?

Reported by Greg Hazel · March 25th, 2011 @ 01:38 AM

State: open
Milestone: none
Assigned to: nobody
Importance: Low

Activity

  1. Andrew White
    Andrew White
    • State changed from new to wontfix
    • Importance changed from to Low

    Sorry, changing this behaviour would break loads of applications.

    March 25th, 2011 @ 07:12 AM

  2. Greg Hazel
    Greg Hazel
    • State changed from wontfix to open

    No, it wouldn't. As I described there is an obvious deprecation path which could be used to prevent application breakage. If you still don't think a long deprecation path is sufficient, how about a global option which defaults to the current behavior?

    Leaving a bad interface will break future applications (like mine, just recently).

    [state:open]

    March 25th, 2011 @ 07:38 AM

  3. Greg Hazel