Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

AssociationCollection's destroy method is not compatible with old version

#2306 

class Article < ActiveRecord::Base
  has_many :reviews
end

in rails 2.2.2, we can use fixnum parameter to destroy one collection object:


article = Article.find(params[:article_id])
article.reviews.destroy(params[:id])

the second line code generates sql which looks like this: SELECT * FROM reviews WHERE (article.id = 1) AND (reviews.id = 10) ORDER BY id desc DELETE FROM reviews WHERE id = 10

in rails 2.3, it will raise an exception: ActiveRecord::AssociationTypeMismatch: Review(#-615260898) expected, got Fixnum(#-604984008)

I thought this commit breaks the compatibility http://github.com/rails/rails/co...

Reported by quake wang · March 21st, 2009 @ 06:20 AM

State: resolved
Milestone: 2.x
Assigned to: nobody
Importance: none

Activity

  1. Anthony Eden
    Anthony Eden

    I'm seeing the same issue. Is there documentation somewhere about the deprecation of collection.destroy(id) because I see that it is not in the latest API docs (maybe it was never there in the first place?) Alternatively is there a fix?

    April 6th, 2009 @ 04:22 PM

  2. CancelProfileIsBroken
    CancelProfileIsBroken
    • Tag set to bugmash

    August 6th, 2009 @ 01:10 PM

  3. Josh Nichols
    Josh Nichols
    • Tag changed from bugmash to bugmash, verified

    Verified this occurring 2-3-stable. I'd +1 adding back support for destroy(id), and if appropriate, adding a deprecation warning.

    August 9th, 2009 @ 05:48 PM

  4. Josh Nichols
    Josh Nichols

    Attached a patch with a failing test case.

    August 9th, 2009 @ 06:14 PM

  5. Josh Nichols
    Josh Nichols

    Attached a patch with a working implementation.

    August 9th, 2009 @ 06:25 PM

  6. Dan Pickett
    Dan Pickett

    +1 verified

    latest patch applies cleanly to master and tests behavior adequately

    August 9th, 2009 @ 06:48 PM

  7. Elad Meidar
    Elad Meidar

    +1 Verified, +1 on the catch, i am usually using delete since i almost never REALLY destroy data in my applications.

    +1 patch applies to 2-3-stable and master, all tests pass.

    Tested also on a mini app, included some extra #delete manual tests to assure that method is working well, and it does.

    August 9th, 2009 @ 06:58 PM

  8. Dan Croak
    Dan Croak

    +1 verified Josh's patch applies to 2-3-stable and tests run green.

    I have attached a patch that alters his implementation to check ActiveRecord::Base instead of String and Integer. This better reveals the intent, in my opinion, of the conditional.

    It should apply cleanly after applying his 2306-destroy_association_by_id.patch

    August 9th, 2009 @ 06:59 PM

  9. Repository
    Repository

    August 9th, 2009 @ 09:47 PM

  10. Repository
    Repository

    August 9th, 2009 @ 09:47 PM

  11. José Valim
    José Valim
    • State changed from new to resolved
    • Tag changed from bugmash, verified to verified

    August 9th, 2009 @ 10:02 PM