Change behaviour of association's after_add callback

Argh, one failing test remains. Will add new patch in a bit.

Fixes the broken has_many :through test cases with association callbacks.

This did however bring up another issue: (ActiveRecord tests)

ted = post.people_with_callbacks.build(:first_name => "Ted")
assert_equal [
  [:added, :after, "Lary"],
  [:added, :before, "Ted"]
], log.last(2)
    
ted.save
assert_equal [
  [:added, :before, "Ted"],
  [:added, :after, "Ted"]
], log.last(2)
# WARNING: This is flawed!?
assert post.people_with_callbacks.include?( ted )

All but the last assertion pass, which is confusing. From the the second assertion we would assume a join model instance was saved because the callback was called, but looking closer it is in fact not the case.

I'm not sure whether or not his patch addresses this concern, but I'd rather bring it up here rather than start a new ticket:

It is highly unexpected that the following triggers an association callback:

class Blog
  has_many :posts, :after_add  => :do_something

  def do_something(post)
    post.profit!
  end
end

blog.posts.create(:title => "I'm an in-valid? post")

This example is a bit contrived, but when creating an invalid child object through an association which carries an add callback, the callback is fired regardless of whether or not the child is valid (and therefor persisted).

The current implementation of these call backs requires some variation of the following to work as expected:

class Blog
  def do_something(post)
    post.profit! unless post.new_record?
  end
end

Was this intended? And if so, what's the point of being notified when an invalid object was attempted to be included in an associated collection? Obviously it's not being persisted (which is to the point of this ticket), so ... ???

Invalid associated objects are still added to the association in memory. So I suppose a case can be made the after_add should still be fired.

I would agree that there could be cases for wanting to execute a callback after any element is added to the association, regardless of whether or not it is actually valid or persisted. Although, honestly, the only fair use I can think of for that functionality is basically logging bad attempts.

It would be terrible form to try to catch an object after it was submitted to the association, check for validity, then if invalid .. modify the object and try again. I guess you could potentially do something crazy like .. if the object isn't really supposed to be in the original association, you could - by means of this callback - attempt to throw it into a different association. But again, these are things you should never do in practice. ;)

I don't want to start an argument about it - because really, it's fairly insignificant - so I'll probably end the "ranting" with this post just saying: My feeling is that if it's called after_add or after_remove it should be after the element is actually added. If the object is invalid .. it certainly isn't actually added to the association. So, maybe it's just a disconnect in the current naming of the callback. The current after_add functionality speaks more to what I would expect from a before_add callback, meaning I get the ability to manipulate the object, regardless of state; granted the current functionality doesn't then attempt to insert it afterward.

So, maybe the shortest distance is to just fill out the current documentation on the subject (see "Association callbacks") in a more meaningful way.

Take a shot at filling out the documentation a bit and we'll see what
the final result looks like. If the documentation reads too much like
a justification for a bad decision, we can look at reversing that
decision :)

As soon as I'm done with what I'm working on, I'll patch the documentation and post it here.

BTW: I apologize to Kenneth for hijacking this ticket. :-\ Certainly not my intention. I'm discontinuing posting to this thread and have created ticket #2664 Update the documentation for association callbacks.

I've unintentionally spawned another thread on the core list dealing with this issue: http://groups.google.com/group/rubyonrails-core/browse_thread/threa...

+1 to either modifying the documentation to be more explicit, or changing the way association callbacks are performed.

I don't think the behaviour of before_add/after_add/before_remove/after_remove should be changed, even though many times I was tempted to do the same.

Instead I propose 4 extra callbacks that do what this ticket requests: before_create/after_create/before_destroy/after_destroy.

The add/remove callbacks are related to additions/removals from the collection in memory, the create/destroy callbacks are related to additions/removals from the collection in the datastore.

I filed a ticket earlier for this: see #3277 +before_create+, +after_create+, +before_destroy+ and +after_destroy+ callbac...

Reproduces with Rails 3.0.3.

+1 for modifying after_add to not get called if the added_record is not persisted, or for adding an alternative callbacks for 'after_persisted' or 'after_create' which would guarantee a valid, persisted record.