This project is archived and is in readonly mode.

#230 ✓ resolved
Craig Demyanovich

Fire model callbacks before notifying observers

Reported by Craig Demyanovich | May 21st, 2008 @ 03:57 PM | in 2.x

While extracting some behavior from a model callback to an observer, I discovered that observers are notified before model callbacks are fired. I expected the opposite. Since I know of no particular reason for the order, I'm submitting a patch to change it.

I didn't add any documentation, but I can envision adding a "Callbacks and Observers" section to the docs that indicates that model callbacks fire before observers are notified.

Comments and changes to this ticket

  • Zach Dennis

    Zach Dennis May 21st, 2008 @ 04:41 PM

    +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.

  • Jason Stewart
  • Jason Stewart

    Jason Stewart May 21st, 2008 @ 05:16 PM

    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.

  • Brandon Keepers

    Brandon Keepers May 23rd, 2008 @ 05:06 AM

    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.

  • Mark Van Holstyn

    Mark Van Holstyn May 23rd, 2008 @ 08:00 AM

    +1

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

  • Joshua Peek

    Joshua Peek May 25th, 2008 @ 12:29 AM

    • Milestone set to 2.1.1
    • State changed from “new” to “open”
    • Assigned user set to “Joshua Peek”
  • Repository

    Repository May 28th, 2008 @ 04:16 PM

    • State changed from “open” to “resolved”

    (from [08763e606282f14fe98f4cf87a9f93d6836373cc]) Callbacks fire before notifying observers [#230 state:resolved]

    Signed-off-by: Joshua Peek

    http://github.com/rails/rails/co...

  • Joshua Peek

    Joshua Peek May 28th, 2008 @ 04:26 PM

    • Milestone changed from 2.1.1 to 2.x
    • State changed from “resolved” to “open”

    After 2.1

  • Repository

    Repository June 3rd, 2008 @ 07:38 PM

    • State changed from “open” to “resolved”

    (from [aa1771668877f20ca044e8f45a9736fbb7c8402e]) Callbacks fire before notifying observers [#230 state:resolved]

    Signed-off-by: Joshua Peek

    http://github.com/rails/rails/co...

  • Brendon Muir

    Brendon Muir November 26th, 2008 @ 06:40 AM

    • Tag set to activerecord, patch, verified

    Actually, this just broke my application. Here's why:

    I have ComponentInstances, a tree based model that's the core of my CMS. Instances of ComponentInstances are polymorphic so you can add Pages, Links, Podcasts etc... to your site and ComponentInstance handles the ordering and tree structuring.

    To make this nice and automatic when you create a Page (for example) there is an observer on the Page model that creates and links the ComponentInstance. Actually there's only one observer that watches all my models that need associated ComponentInstances (but that's nothing surprising).

    The issue comes when I have after_create callbacks on my models that need to use the associated component_instance. For example, I have a callback on one of our models that creates some children instances below this model instance. To do this we need to go page.component_instance.children << blah. But here's the catch, because this callback in the model is called before the callback in the observer, the component_instance doesn't exist yet.

    Obviously this is a fairly complex case, but there's nothing wrong with it as far as I'm aware. Can we come to a solution that works for all?

    Cheers,

    Brendon

  • Craig Demyanovich

    Craig Demyanovich November 26th, 2008 @ 02:41 PM

    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.

  • Brendon

    Brendon November 27th, 2008 @ 08:39 AM

    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

  • Craig Demyanovich

    Craig Demyanovich February 4th, 2009 @ 06:56 PM

    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?

  • Brendon

    Brendon February 4th, 2009 @ 11:38 PM

    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.

  • Zach Dennis

    Zach Dennis February 5th, 2009 @ 12:31 AM

    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.

  • Brendon

    Brendon February 5th, 2009 @ 03:06 AM

    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.

  • Paul

    Paul April 21st, 2009 @ 10:32 AM

    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?

  • tom_302

    tom_302 July 22nd, 2009 @ 07:42 PM

    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).

  • legolin

    legolin August 31st, 2009 @ 02:57 PM

    +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.

  • Obie

    Obie January 29th, 2010 @ 02:19 AM

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

  • csnk

    csnk May 18th, 2011 @ 08:24 AM

    • Importance changed from “” to “”
  • csnk

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Pages