Browse files

Revert "Make InstanceTagMethods#value_before_type_cast raise if the m…

…odel don't respond to attr_before_type_cast or attr method"

And    "Makes form_helper use overriden model accessors"

This reverts commit 3ba8e31 and fb0bd8c.
  • Loading branch information...
1 parent b4954e8 commit 3021297e89dc3720d1bce50dabf1177f8935172e @spastorino spastorino committed Oct 10, 2010
Showing with 3 additions and 38 deletions.
  1. +3 −8 actionpack/lib/action_view/helpers/form_helper.rb
  2. +0 −30 actionpack/test/template/form_helper_test.rb
View
11 actionpack/lib/action_view/helpers/form_helper.rb
@@ -1014,14 +1014,9 @@ def value(object, method_name)
def value_before_type_cast(object, method_name)
unless object.nil?
- if object.respond_to?(method_name)
- object.send(method_name)
- # FIXME: this is AR dependent
- elsif object.respond_to?(method_name + "_before_type_cast")
- object.send(method_name + "_before_type_cast")
- else
- raise NoMethodError, "Model #{object.class} does not respond to #{method_name}"
- end
+ object.respond_to?(method_name + "_before_type_cast") ?
+ object.send(method_name + "_before_type_cast") :
+ object.send(method_name)
end
end
View
30 actionpack/test/template/form_helper_test.rb
@@ -4,18 +4,6 @@
class FormHelperTest < ActionView::TestCase
tests ActionView::Helpers::FormHelper
- class Developer
- def name_before_type_cast
- "David"
- end
-
- def name
- "Santiago"
- end
-
- attr_writer :language
- end
-
def form_for(*)
@output_buffer = super
end
@@ -252,24 +240,6 @@ def test_text_field_with_custom_type
text_field("user", "email", :type => "email")
end
- def test_text_field_from_a_user_defined_method
- @developer = Developer.new
- assert_dom_equal(
- '<input id="developer_name" name="developer[name]" size="30" type="text" value="Santiago" />', text_field("developer", "name")
- )
- end
-
- def test_text_field_on_a_model_with_undefined_attr_reader
- @developer = Developer.new
- @developer.language = 'ruby'
- begin
- text_field("developer", "language")
- rescue NoMethodError => error
- message = error.message
- end
- assert_equal "Model #{Developer} does not respond to language", message
- end
-
def test_check_box
assert_dom_equal(
'<input name="post[secret]" type="hidden" value="0" /><input checked="checked" id="post_secret" name="post[secret]" type="checkbox" value="1" />',

5 comments on commit 3021297

@chrisroos

Was this change intentional? Specifically, the change in behaviour that was previously described in the test_text_field_from_a_user_defined_method test?

I've got an ActiveRecord model that has an attribute stored in the database. I override the method to provide a default value when there isn't one in the database. Something like this:

class Transaction
  def date
    read_attribute(:date) || original_date # where original_date will always exist
  end
end

Prior to this commit I would see either the default or overridden date value in my form. Post this commit I either see the overridden date or nil as #value_before_type_cast now prefers the raw database value.

I can get around this by specifying the actual value with :value but that feels like duplication.

So, assuming the effect of this commit was intentional I'm wondering whether there's a better way to achieve what I'm after?

@spastorino
Ruby on Rails member

The commit was wrong, the previous way never reach the object.send(method_name + "_before_type_cast") line.
Why? because, the if is always true since AR defines the readers for you.

@chrisroos

Does it definitely need to call _before_type_cast? I rewrote the value_before_type_cast method as below and none of the tests fail.

def value_before_type_cast(object, method_name)
  object.send(method_name) unless object.nil?
end

Do you know if this is indicative of missing tests or of unnecessary behaviour?

@spastorino
Ruby on Rails member

missing tests, I guess, please go ahead and add them :)

@chrisroos

Hey. Because I don't know what tests are missing I've gone down the route of adding a failing test to expose the behaviour I want, and a fix for it. I've issued a pull request with my changes at #1147. It's more to generate a discussion around what the behaviour should be.

Please sign in to comment.