form.input :field, :as => :check_boxes, :collection => [...] is broken #246

Closed
timcharper opened this Issue Apr 20, 2010 · 34 comments

Projects

None yet

6 participants

@timcharper

Say you provide something like this in your form:

<%= form.input :roles, :as => :check_boxes, :collection => ["admin"] %>

When you submit the form, the params will always contain a blank value

params[:user][:roles] # => ["", "admin"]

This is because of the underlying check_box unchecked magic that rails provides. It always defines two fields with the same name, the first field of being a hidden element with the unchecked value, and then the second field being the checkbox with the checked value. This allows you to specify different values of the checkbox dependent on the checked state, as opposed to value not present, and value present with some value.

This works fine in every case except those cases where the name ends with []. Instead of replacing the hidden value, it appends the hidden value. IE:

<!-- Works ok -->
<input type="hidden" name="lights" value="off" />
<input type='checkbox" name="lights" value="on" />

<!-- Doesn't work okay -->
<input type="hidden" name=" favorite_colors[]" value="" />
<input type='checkbox" name=" favorite_colors[]" value="blue" />
<input type="hidden" name=" favorite_colors[]" value="" />
<input type='checkbox" name=" favorite_colors[]" value="red" />

Submitted params of the latter example if you check every box: { "favorite_colors" => ["", "blue", "", "red"] }

@justinfrench

Is this in 0.9.8, edge, the rails3 branch, or other?

@timcharper

formatastic 0.9.8, rails 2.3.5

@justinfrench

Interesting. So I'm guessing this bug has been there all along. Obviously the work-around for you right now is to strip blank items from that array.

I'll have to look into the fix, but Rails' check_box produces two inputs (one hidden), so my guess is we need to rely on check_box_tag instead, which doesn't do this.

@justinfrench

... and I've looked at this for an hour or two. Pretty tricky. I'll take a look with fresh eyes later, but if someone else wants to have a go, great.

@sobrinho
Collaborator

This HTML works in Firefox but not in others browsers.

Which we need to do is set a index for array. [1] instead of [] for example. I will look this weekend.

Patchs are welcome.

@justinfrench

This is the only issue holding me back from 0.9.9 :)

@timcharper

@sobrinho

Which HTML works in Firefox but not other browsers? the [] convention is parsed by rails, and should not make a difference on the browser side.

@timcharper

I would favor a solution that doesn't produce two inputs, such as using check_box_tag

@justinfrench

@timcharper this is what I tried, but it turned out to be a huge rabbit hole.

@timcharper

crap, that sucks. Rails fail here.

I suppose modifying the output via .sub(/]+>/) is out of the question?

@justinfrench

HAH

@timcharper

it seems like rails should have an option to generate this extraneous checkbox or not, so I would pursue a path that uses a workaround until rails makes a change like that.

@justinfrench

If I knew Rails was going to change, I'd look at a workaround, but not under the assumption that maybe one day they'll change. There already is a work-around on the server-side (filter the blank values from the params), so I'd rather leave that in place until we have a decent solution.

There's only been one report of this bug, so I'm not inclined to throw a heap of hours at it for a work-around just yet. Someone else may find a nice way forward, still open to ideas!

@sobrinho
Collaborator

@justinfrench

Submit a form with this fields on Firefox and Safari and inspect your rails application:

<input type="text" name="fields[][foo]" />
<input type="text" name="fields[][bar]" />
<input type="text" name="fields[][foo]" />
<input type="text" name="fields[][bar]" />

Firefox sends:

params[:fields] #=> { 0 => { :foo => '', :bar => '' }, 1 => { :foo => '', :bar => '' } }

Safari sends:

params[:fields] #=> { 0 => { :foo => '' }, 1 => { :bar => '' }, 2 => { :foo => '' }, 3 => { :bar => '' } }
@justinfrench

Ok, re-opening!

@justinfrench
Owner

@sobrinho, what's your suggestion to move forward on this? I'm happy to work on a patch, but I don't want to throw time into the wrong solution?

@sobrinho
Collaborator

The solution is a key for the field name but I don't know how we can generate this key. On my applications I temporaly do something like this:

key = Time.now.to_f
text_field_tag "something[somethings_attributes][#{key}][foo]"
text_field_tag "something[somethings_attributes][#{key}][bar]"

I do this for javascript function to "Add new resource". But this solution isn't good.

@justinfrench
Owner

What's the impact if we just start counting from zero (mimicking Safari or Firefox's implementation, whichever we think is more correct)?

@sobrinho
Collaborator

This counting will increment on each input or only in nested fields?

I agree this should be a counter starting from zero.

@yabawock
Collaborator

Btw - this behaviour is described within the rails documentation:

http://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#M001612

Gotcha

... the client either sends only the hidden field (representing the check box is unchecked), or both fields. Since the HTML specification says key/value pairs have to be sent in the same order they appear in the form and Rails parameters extraction always gets the first occurrence of any given key, that works in ordinary forms.
Unfortunately that workaround does not work when the check box goes within an array-like parameter...

@yabawock
Collaborator

I have prepared a proposed fix - it's in brach gh-246, commit 105bb7e

Solving this problems kinda opens a can of worms. Currently I see two problems arising. One is backward as well as rails form helper compatiblity, the second is making formtastic work out of the box.

I based my fix on the assumption that anyone who is affected by this bug would also be affected with original formhelper output. Because of this - in my opinion - it would be best to keep formtastic operating like the default rails form helper, at least out of the box.

Based on this decision my fix introduces a new boolean option for "input" fields, namely :no_hidden_input. Setting this option to true would make formtastic output plain checkbox inputs without the associated hidden fields.

Naturally, based on this mode of operation, formtastic doesn't work "out of the box" for an application developer being affected by this use case.

So guys, what's you opinion on this whole mess. I'm open for suggestions how to proceed from here, code improvements or just opinions showing the way to proceed.

@yabawock
Collaborator

A bugfix has been integrated into the rails3 branch and will be in Formtastic 1.1. Formtastic 1.0 will not include this fix as it might break existing applications. By default the check_boxes helper will not render the hidden inputs anymore.

By setting the option :hidden_fields => true on a check_boxes tag the "old" behaviour will be restored.

The merge is in commit dd5d4f8

@giddie

The problem with this is that formtastic no longer works out of the box for simple associations. I have a has_and_belongs_to_many association between User and UserRole. The checkboxes will now be rendered without the hidden fields; however, this means that when submitting the form with no checkboxes checked, the role_ids array isn't submitted at all, and user.roles is not updated :( I think this is a far more common use-case than the one the change is supposed to fix.

I think it would be safer to use Rails' default behaviour if at all possible. Maybe a config option in an initializer would be better, so that users can switch to no-hidden-checkboxes-by-default if they want? The ideal solution would be to push for a proper fix upstream, surely?

@justinfrench
Owner

There is nothing "upstream" Rails can do, as far as I'm aware. In the complex "array-like" cases (see quote from Rails API above), the params hash generated by browsers is kinda useless.

I agree that it's a shame to "break" (remember, the option is still there) the common use case, but it's also not cool to have the same code behave differently in "simple" and "complex" examples, and definitely not cool to ship code that we know is kinda useless.

Open to suggestions, definitely keen to have more people get involved in this, but this is the best we could do so far.

@yabawock
Collaborator

I've just played around with Firebug and modifying the params hash passed to rails. I think I found a way to pass an empty value to rails that signals "no checkboxes" without breaking anything else. At least in my simple demo app here on github it works for removing phonenumbers from the contact.

I'll look into it tonight, but if it works out the way I hope it the fix is less work than writing the test to accompany it.

It should be possible for the check_boxes function to generate an empty value as a hidden field that gets overriding by the array generated from selected checkboxes. The empty string will be used by rails to "empty" the association.

@yabawock
Collaborator

I've pushed the enhancement in commit 96a2e54

Looking forward to hearing if this solution works for your use case as well.

@justinfrench
Owner

Nice idea!

@yabawock
Collaborator

Fix confirmed as working in #321

@giddie

I'm afraid I'm getting a 500 when one or more checkboxes is selected; the log says "TypeError (expected Array (got String) for param `role_ids'):", but provides no trace.

I assume it's something to do with the fact that user[role_ids] is a String?

This is with Rails 3.0.0.rc & Firefox 3.6.8. The same happens with Chrome.

@yabawock
Collaborator

Could you show me the params hash that your controller is receiving (should be in the development log) and the relevant part of your _form where you generate the check_boxes?

@giddie

The full log for the PUSH request is here: http://gist.github.com/506116 Unfortunately, it seems Rails fails to parse the options at all. And here's my form code: http://gist.github.com/506117

@yabawock
Collaborator

Thanks, i've opened a separate issue to track that, but I was able to reproduce it locally in one application.

@julian7 julian7 pushed a commit that referenced this issue Jun 25, 2011
@yabawock yabawock Provide an empty default value for the form variable that's going to
hold the array of values the user selected. This value get's replaced
by the array as soon as a single checkbox is ticked, otherwise this default
value is being used, very similar to the way the upstream rails helper handles
this situation for single checkboxes.

More background information on this is available in GH-246
96a2e54
@davidray

I'm still having this problem on 2.0.2 with Rails 3.0.10.

@justinfrench

@davidray can you please open a new issue with specific information about your case? this is a pretty noisy and old issue

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment