This project is archived and is in readonly mode.

#641 ✓resolved
Clemens Kofler

FormHelper#fields_for has illogical (and unused) option

Reported by Clemens Kofler | July 17th, 2008 @ 12:12 PM | in 2.x

I've just found an option in FormHelper#fields_for that seems to be both, illogical and (therefore) unused.

In the case statement, there's the following snippet: 

# ...
when Array
  object = record_or_name_or_array.last
  object_name = ActionController::RecordIdentifier.singular_class_name(object)
  apply_form_for_options!(record_or_name_or_array, options)
# ...

I was suspicious because this isn't documented and the apply_form_options! call doesn't make sense because it generates form-specific stuff like the action attribute and html options for the form itself and has nothing to do with fields_for that only generates a scope around a given entity. I think this may have been copied from form_for at the time that fields_for was introduced as a new feature.

Using it inadvertedly produces routing errors. With nested resources (e.g. product has many prices and fields_for [:product, @price]) you get insufficient parameters and without nested resources (fields_for [:product, @price]) you get unknown named routes. It only works if you specify a :url parameter (fields_for [:product, @price], :url => 'whatever') and, of course, doesn't produce any sensible output (i.e. it acts like fields_for :product).

Since the functionality doesn't make sense, I tried removing it. It didn't break any tests so I guess this wasn't only undocumented but also unused.

Why does removing it make sense, apart from the fact that this is non-existent functionality?

Firstly, it may give us slightly improved performance when passing in an actual object as its first parameter because it doesn't have one redundant check on the type of the parameter.

Secondly, it may be worth removing it for now and later re-introducing it with sensible behavior with regards to a discussion on the Core list (http://groups.google.com/group/r.... One could then pass in an array of symbols/objects and have it build a nested hash (similar to form_for building a nested route).

Patch is attached.

Comments and changes to this ticket

  • RSL

    RSL July 17th, 2008 @ 01:26 PM

    I think you need this in fields_for because form_for actually uses fields_for internally.

  • Clemens Kofler

    Clemens Kofler July 17th, 2008 @ 01:34 PM

    RSL: Good catch but that's not exactly true.

    As I said, all tests pass which means it doesn't break any existing functionality. Moreover, form_for only uses fields_for internally to create the scope so you can say f.text_field :name and get product[name] instead of just name.

    If you look at the exact call in form_for, it looks like this: 

    fields_for(object_name, *(args << options), &proc)

    So form_for passes in the object name (e.g. product), followed by the args (and it pushes the options hash it extracted before back in the array) and the given block. The options that apply_form_for_options! uses are tag-related and fields_for does never produce an actual tag - so the call is, as far as I see it, redundant.

    Thanks for the comment, though! :)

  • RSL

    RSL July 17th, 2008 @ 01:45 PM

    Yeah. I read this in passing and just opened Rails source to verify and saw the same thing. I knew I remembered it using fields_for internally, just not the extent. :)

  • Ryan Bates

    Ryan Bates July 18th, 2008 @ 06:59 PM

    +1, I have some ideas on how passing an array to form_for can work (which probably deserves its own ticket) and getting this existing, odd behavior removed is a good first step.

  • josh

    josh July 19th, 2008 @ 04:36 PM

    • Assigned user set to “josh”
  • Repository

    Repository July 19th, 2008 @ 06:37 PM

    • State changed from “new” to “resolved”

    (from [938caf4e6b2448b45939d36824794ea0aa5e1804]) Removed unused option from FormHelper#fields_for [#641 state:resolved]

    Signed-off-by: Joshua Peek

    http://github.com/rails/rails/co...

  • Yong Bakos

    Yong Bakos October 6th, 2008 @ 07:23 AM

    I'd like to chime in on this, with a recommendation on future behavior.

    It sho would be nice if fields_for, when fed an array, would build field names with numbers.

    For example, imagine an Order that has_many LineItems, and each LineItem belongs_to a Product.

    
fields_for @order.line_items do |lif|
# field rendering <input name="line_items[1][quantity] />
end

    Maybe the bottom of this thread can help illustrate.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages