Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  Kenneth Kalmer February 28th, 2009 @ 03:21 PM

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

• You flagged this item as spam.
  

  Kenneth Kalmer February 28th, 2009 @ 05:01 PM

  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.

  • updated_after_add_behavior-20090228185246.patch 10.2 KB
• 

  Eloy Duran March 1st, 2009 @ 12:47 PM

  • Assigned user changed from “Eloy Duran” to “Michael Koziarski”
• 

  Pratik March 9th, 2009 @ 01:03 PM

  • Assigned user changed from “Michael Koziarski” to “Pratik”
  • Title changed from “[PATCH] Change behaviour of association's after_add callback” to “Change behaviour of association's after_add callback”

• You flagged this item as spam.
  

  Nathaniel Bibler May 14th, 2009 @ 07:32 PM

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

• 

  Michael Koziarski May 15th, 2009 @ 04:25 AM

  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.

• You flagged this item as spam.
  

  Nathaniel Bibler May 15th, 2009 @ 04:07 PM

  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.

• 

  Michael Koziarski May 16th, 2009 @ 12:37 AM

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

• 

  Pratik May 17th, 2009 @ 05:42 PM

  • Assigned user changed from “Pratik” to “Michael Koziarski”

• You flagged this item as spam.
  

  Nathaniel Bibler May 17th, 2009 @ 05:55 PM

  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.

• 

  CancelProfileIsBroken August 6th, 2009 @ 02:42 PM

  • Tag changed from “associations, callbacks, patch” to “associations, bugmash, callbacks, patch”

• You flagged this item as spam.
  

  Brennan Dunn August 18th, 2009 @ 01:50 AM

  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.

• You flagged this item as spam.
  

  Lawrence Pit September 28th, 2009 @ 01:43 AM

  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

• 

  Rizwan Reza February 12th, 2010 @ 12:46 PM

  • Tag changed from “associations, bugmash, callbacks, patch” to “associations, callbacks, patch”

  [bulk edit]

• 

  Jeremy Kemper May 4th, 2010 @ 06:48 PM

  • Milestone changed from “2.x” to “3.x”

  [bulk edit]

• 

  Santiago Pastorino February 2nd, 2011 @ 04:54 PM

  • State changed from “new” to “open”
  • Importance changed from “” to “”

  This issue has been automatically marked as stale because it has not been commented on for at least three months.

  The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

  Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

• 

  Santiago Pastorino February 2nd, 2011 @ 04:54 PM

  • State changed from “open” to “stale”

• You flagged this item as spam.
  

  Michael Kintzer February 2nd, 2011 @ 06:52 PM

  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.