Permalink
Browse files

Revert "Revert "More 1.9 way""

Fixed failing tests
This reverts commit 8350ce9.
  • Loading branch information...
1 parent 6e6764b commit fc956425d405e75bcf310f3e6f08cf201cc8131a @spastorino spastorino committed Jan 6, 2012
Showing with 4 additions and 4 deletions.
  1. +2 −0 actionpack/test/template/form_helper_test.rb
  2. +2 −4 activemodel/lib/active_model/conversion.rb
@@ -867,6 +867,7 @@ def test_form_for_with_remote_in_html
def test_form_for_with_remote_without_html
@post.persisted = false
+ def @post.id; nil; end
form_for(@post, :remote => true) do |f|
concat f.text_field(:title)
concat f.text_area(:body)
@@ -1016,6 +1017,7 @@ def test_submit_with_object_as_new_record_and_locale_strings
old_locale, I18n.locale = I18n.locale, :submit
@post.persisted = false
+ def @post.id; nil; end
@pixeltrix

pixeltrix Jan 6, 2012

Owner

Doesn't this imply that form_for expects an object that responds to id ? Shouldn't it work with any ActiveModel compliant object?

@spastorino

spastorino Jan 6, 2012

Owner

This line doesn't imply that, I'm just reverting this https://github.com/rails/rails/blob/master/actionpack/test/template/form_helper_test.rb#L68 stub, but yeah I could have removed the method if that's what you mean

@josevalim

josevalim Jan 6, 2012

Member

Maybe a better implementation of this would be "stubbing" to_key instead of id.

@spastorino

spastorino Jan 6, 2012

Owner

change whatever you want bros :)

@pixeltrix

pixeltrix Jan 6, 2012

Owner

I still think there's a problem with the new implementation of to_key. We're changing the return value in the case where a model isn't persisted from always nil to whatever the current attribute value is - some code maybe relying on the old behaviour.

@josevalim

josevalim Jan 6, 2012

Member

The documentation for to_key is very clear: "returns a key regardless if the object is persisted or not". If someone was relying on the old behavior, they were wrong :). In any case, this is a Rails 4 change, so it gives us plenty of mileage to detect issues.

@spastorino

spastorino Jan 6, 2012

Owner

Avoided stubs to id here 3853b73

form_for(@post) do |f|
concat f.submit
end
@@ -39,11 +39,9 @@ def to_model
# Returns an Enumerable of all key attributes if any is set, regardless
# if the object is persisted or not.
- #
- # Note the default implementation uses persisted? just because all objects
- # in Ruby 1.8.x responds to <tt>:id</tt>.
def to_key
- persisted? ? [id] : nil
+ key = respond_to?(:id) && id
+ key ? [key] : nil
end
# Returns a string representing the object's key suitable for use in URLs,

0 comments on commit fc95642

Please sign in to comment.