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.

ActiveRecord should favor ActiveModel API

#5927

ActiveModel introduced the persisted? method (inverse of new_record?), but ActiveRecord still uses new_record? internally and across associations. This means that extension libraries sometimes have to be aware of whether they're dealing with ActiveRecord or not.

Fixed in https://github.com/dchelimsky/rails/commit/e84cd8a2533c9581d76eca55...

NOTES:

  • patched against master as of c43d909ee28484c5e7d7d84b1228e10212d20737

  • there was one failing test before this commit, which is still failing:

    test/cases/associations/cascaded_eager_loading_test.rb:128
    test_eager_association_loading_with_has_many_sti_and_subclasses

  • there is one new failing test, but I question its validity. It specifies that to_key should return a non-nil value after destroy is called on a model. This seems like the opposite of what we would want, and this is the only new test failure (so no other code seems to rely on this behavior).

    test/cases/primary_keys_test.rb
    test_to_key_with_primary_key_after_destroy

Reported by David Chelimsky · November 7th, 2010 @ 02:20 PM

State: committed
Milestone: none
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: Low

Activity

  1. David Chelimsky
    David Chelimsky

    Note that new_record? is not removed by this change (that would not be so good for the myriad extensions rely on it). The change is that ActiveRecord uses persisted? internally and new_record? depends on the state of persisted?, rather than the other way 'round.

    November 7th, 2010 @ 02:24 PM

  2. José Valim
    José Valim
    • Importance changed from to Low

    David, I was the person who added the mentioned failing test and the reason is here (after some git blame):

    #4296 dom_id returns incorrect value for destroyed records

    Notice though that #to_key implementation does not care if the record was persisted or not:

    https://github.com/rails/rails/blob/master/activemodel/lib/active_m...

    That said, the test is valid but we could probably change the current to_key implementation in AR to the following:

    def to_key
  key = send(self.class.primary_key)
  [key] if key
end

    So what do you think?

    November 7th, 2010 @ 07:12 PM

  3. David Chelimsky
    David Chelimsky

    That seems fine to me, as long as persisted? returns false if it's been destroyed. Do you want me to make that change in my patch? Or merge what I've got and update it after?

    November 9th, 2010 @ 01:31 AM

  4. David Chelimsky
    David Chelimsky

    November 9th, 2010 @ 01:47 AM

  5. David Chelimsky
    David Chelimsky

    FYI - the updated patch is rebased off a more recent master. Everything is passing now for me.

    November 9th, 2010 @ 01:47 AM

  6. José Valim
    José Valim

    Yes, persisted? should return false if the object is destroyed. After a quick glance, the patch looks fine to me!

    November 9th, 2010 @ 10:13 AM

  7. Repository
    Repository
    • State changed from new to committed

    (from [1f06652a57e727700c3a673dc1f86e3b1e07ce1f]) use persisted? instead of new_record? wherever possible

    • persisted? is the API defined in ActiveModel
    • makes it easier for extension libraries to conform to ActiveModel APIs without concern for whether the extended object is specifically ActiveRecord

    [#5927 ActiveRecord should favor ActiveModel API state:committed]

    Signed-off-by: Santiago Pastorino santiago@wyeworks.com
    https://github.com/rails/rails/commit/1f06652a57e727700c3a673dc1f86...

    November 9th, 2010 @ 06:54 PM

  8. Repository
    Repository

    (from [f1c13b0dd7b22b5f6289ca1a09f1d7a8c7c8584b]) use persisted? instead of new_record? wherever possible

    • persisted? is the API defined in ActiveModel
    • makes it easier for extension libraries to conform to ActiveModel APIs without concern for whether the extended object is specifically ActiveRecord

    [#5927 ActiveRecord should favor ActiveModel API state:committed]

    Signed-off-by: Santiago Pastorino santiago@wyeworks.com
    https://github.com/rails/rails/commit/f1c13b0dd7b22b5f6289ca1a09f1d...

    November 9th, 2010 @ 07:28 PM