#5927 ActiveRecord should favor ActiveModel API - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5927 ✓committed

ActiveRecord should favor ActiveModel API

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

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

Comments and changes to this ticket

You flagged this item as spam.
David Chelimsky November 7th, 2010 @ 02:24 PM
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.
José Valim November 7th, 2010 @ 07:12 PM
- 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):

https://rails.lighthouseapp.com/projects/8994/tickets/4296

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?
You flagged this item as spam.
David Chelimsky November 9th, 2010 @ 01:31 AM
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?
You flagged this item as spam.
David Chelimsky November 9th, 2010 @ 01:47 AM
Updated patch: https://github.com/dchelimsky/rails/commit/787407c40db54d6ea87308e2...
You flagged this item as spam.
David Chelimsky November 9th, 2010 @ 01:47 AM
FYI - the updated patch is rebased off a more recent master. Everything is passing now for me.
José Valim November 9th, 2010 @ 10:13 AM
Yes, persisted? should return false if the object is destroyed. After a quick glance, the patch looks fine to me!
Repository November 9th, 2010 @ 06:54 PM
- 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 state:committed]

Signed-off-by: Santiago Pastorino santiago@wyeworks.com
https://github.com/rails/rails/commit/1f06652a57e727700c3a673dc1f86...
Repository November 9th, 2010 @ 07:28 PM
(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 state:committed]

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