sending :id => nil to form helpers does not remove the "id" html attribute

Here's a patch with tests.

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.

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.

+1

Based on the inconsistency of :class and :id behavior

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

+1

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

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.

+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?

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

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.

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.

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