The order of macro-style methods could be problematic for before_destroy callbacks

[daviddavis]

Due to rails/rails#3458, we may want to consider putting callbacks before associations (or maybe just before_destroy?). Take the following example.

def Person
  has_many :roles, :dependent => :destroy

  before_destroy :check_deletable

  def :check_deletable
    fail "Cannot delete super admin." if super_admin?
  end
end

This won't work as expected. Instead it will destroy a user's roles whenever destroy is called regardless if the user actually gets deleted. The fix is to list the callback first:

def Person
  before_destroy :check_deletable

  has_many :roles, :dependent => :destroy

  def :check_deletable
    fail "Cannot delete super admin." if super_admin?
  end
end

It might be a best practice to just list callbacks before associations--or just before_destroy?

EDIT: Thanks to @smudge (see below), here's a better workaround:

def Person
  has_many :roles, dependent: :destroy

  before_destroy :check_deletable, prepend: true

  def :check_deletable
    fail "Cannot delete super admin." if super_admin?
  end
end

[nickserv]

👍

[xaviershay]

Just got bitten by this.

The behaviour is documented in "Ordering callbacks": http://api.rubyonrails.org/classes/ActiveRecord/Callbacks.html

... but I still think it is broken behaviour.

[daviddavis]

@xaviershay you mean broken in Rails? I think I'd agree with that.

[xaviershay]

correct, it's a non-obvious dependency between apparently unrelated declarative statements.

[bbatsov]

@xaviershay @daviddavis I also agree that's broken in Rails. Maybe this should be discussed with someone on the Rails team?

[smudge]

Just got hit by this too. HUGE gotchya. (dependent: :destroy trashes the db before before_destroy can do anything about it.)

For now I'm using before_destroy, prepend: true as a workaround, but I still feel that I've been lied to. (before_destroy should always run before any kind of destroy takes place.)

[andre-schilipack]

@smudge Nice workaround! before_destroy, prepend: true. I think it's the best way for me, right now.

I hope rails change its behavior soon.

[daviddavis]

I opened a temporary fix in #110