Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Jeff Dean May 9th, 2010 @ 07:55 AM

  Here's a patch with tests.

  • nil-form-helper-id.diff 7.2 KB
• 

  Matt Jones May 9th, 2010 @ 04:58 PM

  Interesting idea, but the stated goal ("save bandwidth") smells an awful lot like premature optimization. Are you really generating enough deeply-nested fields to save more than a few bytes?

  This also seems likely to cause more complication for anybody who is trying to generate text_field calls programmatically:

    def some_text_field_helper(name, id=nil)
      text_field some_function_of_name(name), 'something', :id => id
    end
  

  The above is going to be trickier to code with this patch than without.

• 

  Dan Pickett May 9th, 2010 @ 05:48 PM

  • Tag changed from “form_helper, patch” to “bugmash, form_helper, patch”

• You flagged this item as spam.
  

  Jeff Dean May 9th, 2010 @ 07:32 PM

  For me the biggest issue is inconsistency. If I pass :class => nil to a text_field, no class attribute is emitted. If I pass :id => nil, the default id is emitted. On that basis alone (principle of least surprise), I think current behavior is a bug.

  I've encountered a number of other places where having no ids is beneficial - for example when I have the same form in multiple places on the same page (think the "new task" form within each task list in Basecamp) and I want the HTML to validate. In that case I'm forced to use an index, which I'm only doing for the purpose of getting valid HTML because Rails won't respect :id => nil.

  Matt - examples like the one you gave would be trivially more complex, but I think that's a totally acceptable price to pay for consistency across attributes.

• 

  Josh Kalderimis May 10th, 2010 @ 11:23 AM

  +1

  Based on the inconsistency of :class and :id behavior

  Is it only text_field, text_area and check_box which are affected?

• You flagged this item as spam.
  

  Wijnand Wiersma May 15th, 2010 @ 12:03 PM

  +1

  but I think this should affect all input types.
  Just a quick test reveals it doesn't work for radio buttons.

• You flagged this item as spam.
  

  Wijnand Wiersma May 15th, 2010 @ 12:03 PM

  • no changes were found...

• You flagged this item as spam.
  

  Wijnand Wiersma May 15th, 2010 @ 01:23 PM

  My previous comment (and sorry for the empty one) was incorrect.
  Using f.radio_button it does skip the id, I did a quick test using radio_button_tag so that explains my incorrect observation.

  The patch seems to be pretty complete to me and works fine.

• You flagged this item as spam.
  

  Casey Dreier May 15th, 2010 @ 08:16 PM

  +1 Patch applies just fine. Verified.

  I made a tiny addition of adding a test for verifying that the id is not generated for radio_boxes.

  Note that this patch as implemented does not currently allow you to specify :id => nil for select boxes. Is this something that should be added for consistency?

  • nil-form-helper-id_2.diff 7.6 KB

• You flagged this item as spam.
  

  Jeff Dean May 15th, 2010 @ 09:15 PM

  I think it should be consistent. I'm adding support for that now, and will re-post a patch.

• 

  Rizwan Reza May 15th, 2010 @ 09:15 PM

  • Tag changed from “bugmash, form_helper, patch” to “bugmash-review, form_helper, patch”

• You flagged this item as spam.
  

  Jeff Dean May 15th, 2010 @ 09:26 PM

  Thanks Casey. "select" actually did work as expected, and the syntax is:

  select("post", "secret", [], {}, "id" => nil)
  

  I've reattached the patch with an extra test case to support this, but I lost the "Sign-Off" by Casey because I didn't know how to keep it.

  • nil-form-helper-id-3.diff 7.6 KB
• 

  Repository May 15th, 2010 @ 09:46 PM

  • State changed from “new” to “resolved”

  (from [6617d0189377a2f820c8f948589bb2d4a91155af]) Sending :id => nil to form helpers now properly omits the "id" html element [#4559 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/6617d0189377a2f820c8f948589bb2...

• You flagged this item as spam.
  

  Jeff Dean May 15th, 2010 @ 10:13 PM

  Awesome - thanks José!

  I just realized that I used the arity-2 form of fetch, instead of the block format. Here is a patch that will make this a bit more performant. It's a straight refactoring, and the behavior doesn't change so I don't see the need to change the tests.

  • nil-form-helper-fetch-block.diff 1.5 KB

• You flagged this item as spam.
  

  Casey Dreier May 15th, 2010 @ 10:45 PM

  Sorry, Jeff, my apologies. I was using an incorrect syntax.

• 

  Rizwan Reza May 16th, 2010 @ 02:07 AM

  • Tag changed from “bugmash-review, form_helper, patch” to “form_helper, patch”