Fire model callbacks before notifying observers

+1. This patch is simple and the test is clean and easy to read. The proposed behavior also seems to be correct: model callbacks should fire before external Observer callbacks.

+1. Looks good to me.

I'd also like to add that the tests look good, and firing the callbacks on the model before the observers seems to be the right thing to do.

I can't think of any reason why it should be the way it is. Seems logical that the callbacks defined closest to the model should be called first.

+1 - patch and tests look good.

+1

I think it makes much more sense for model callbacks to fire first, then observers. Tests all look good and pass.

That a model's callbacks fire before any of its observers are notified makes the most sense to me, at least as a default. Indeed, the surprise that they didn't work that way is the reason that I wrote the patch. Assuming that there's no good way to change your implementation to match this behavior, you could explore ways to configure the callback and observer notification order, perhaps on a per-model basis.

Thanks Craig, I managed to work around it with superclasses. When you define callbacks in superclasses (and do self.abstract_class = true on that superclass of course), these callbacks get called first. Handily, I already had the superclass in place so it was an easy change and now I don't need to maintain a list of observed classes in my observer.

However, what upset me about this change was that you mention that documenting the modification would have been nice, but you never actually did it. I searched for half a day (literally) trying to find an explanation as to why the behaviour had suddenly changed and finally stumbled upon this ticket. Needless to say that if the execution order was documented I would have found the problem much sooner as I scoured the API for any hint of it :) Perhaps could you add a line to the docs to this effect now?

Cheers,

Brendon

I was searching the book, The Rails Way, for an answer to another question when I stumbled upon the "Timing" section on p. 284. It states that, "Observers are notified before the in-object callbacks are triggered. Otherwise, it wouldn't be possible to act on the whole object in something like a before_destroy observer without having the object's own callbacks executed first."

Does this new information make anyone else want to remove my patch?

Yes please. It makes sense to reverse the change (and document it in an obvious place) especially since a prominent book explicitly talks of it and states a very good reason to have it the original order.

I don't think the order should be reverted. Trying to appeal to the book as the authority is not a solid argument, and unless the book expands in more detail on why it is such a good thing, I don't see the sentence that Craig found that compelling.

In what concrete example would you expect or need an external callback to be called first?

After a model object is destroyed it still retains all of its properties in memory, so any external before_destroy callback would still be able to operate just fine. The only time it wouldn't would be if it needed to operate on the object itself (by updating and saving the object), but that seems pointless since it's in the process of being deleted.

An object's internal callbacks should take precedence over external callbacks.

I gave a perfectly fine example above where I wanted to create dependent objects after the creation of the observed object, and then work on these as a group via internal callbacks specific to certain objects.

i am in trouble now with post notifying observers ...

Assume you want to prevent object destruction based on observing before_destroy.

There are two problems now:

1) the return value of notified callback is thrown away. This seems to be ok for me, as i think an observer should not have an easy to use way of changing model behaviour.

but now

2) i do not have a chance to define something like model.please_do_not_destoy_as_external_conditions_are_not_met = true and react on it within the model before_destroy callback itself ...

What could be a solution to this problem now?

This change broke my app, too. (I use before_save() to set a belongs_to association to a new record, but now that record doesn't get saved.)

Perhaps the best of both worlds is to call notify() and run_callback() in different order based on whether the prefix is 'before' or 'after'. That way, observers have access to the latest data (after some event) and the ability to modify data (before some event).

+1 @tom_302

I completely agree that before and after callbacks should be handled differently.

In the case of before callbacks, observers can (1) process external information that the model shouldn't have to worry about, (2) change the state of the model appropriately, so (3) the model can make an informed final decision on whether to follow through with the action.

In the case of "after" observers like after_create, the model can do it's own post-processing first, providing a complete model for the observer to use when, say, sending an email.

+1 @legolin Well-reasoned option, should be considered.