TypeError (expected Array (got String) for param for check_boxes #322

Closed
yabawock opened this Issue Aug 3, 2010 · 26 comments

Projects

None yet

5 participants

@yabawock
Collaborator

The workaround to pass an empty value to check_box collection as a default can trigger a TypeError in some conditions.

TypeError (expected Array (got String) for param `phonenumber_ids')

@yabawock
Collaborator

Fix pushed in 7a5120e

Please review and comment

@paneq

I just find out that either the documentation or implementation of this feature is wrong. It states that:

The :hidden_fields option provides a way to re-enable these hidden inputs by setting it to true.

However thanks to this line of code:

list_item_content << template.content_tag(:li, self.hidden_field(input_name, {:value => ''}), li_options) unless hidden_fields

it works the opposite way. Setting it to 'true' the hidden field is skipped.

@yabawock
Collaborator

We should think about renaming that field or making the documentation more specific in regard to the new situation. If you look at #246 you will see that originally this solution didn't use any hidden fields for the collection to work around the params hash being passed to rails being severly borken and inconsistent across browsers.

Then the issue came up that it was impossible to clear the collection completly. So an initial default value (as a hidden field) has been re-introduced (first as string value, now as an array)

So the :hidden_fields option refers to a hidden field for each choice, not for the whole collection

@paneq

Could you explain this statment more clearly please: "So the :hidden_fields option refers to a hidden field for each choice, not for the whole collection".

I just updated to current git version using bundler and now this feature works fine for me.

@paneq

Thank you for such a quick fix :-)

@yabawock
Collaborator

Using :hidden_fields => true you get HTML code like this:
http://pastie.org/1072704

Each choice in your collection has a empty default array value of it's own

The new default way :hidden_fields => false results in HTML code like this:
http://pastie.org/1072707

Where there is only one hidden field with an empty default value for the whole collection.

@paneq

The new default way works really nice with associations so its great for me :-). Thank you for the explanation. The only other case I can imagine is that someone might not want the hidden fields at all (don't know why) but that is not my case.

@noahhendrix

After applying this patch I'm getting strange behavior. It's entirely possible the problem lies in my application design, but just want to put it out there because this worked fine before:

when formtastic compares checkbox values with the model to determine checked status it tries to run the value_method on the raw string passed in from params

Here is an the error message if it makes more sense:
undefined method `id' for "":String

In my case it is a search form, so my model is the search. If I pass the generic :search, :as => @search it works, but doesn't refill the values, as expected.

Any thoughts?

@yabawock
Collaborator

Can you give us some more information? Form code, association definition, and maybe some more information from the stack trace?

Also your syntax :search, :as => @search looks wrong

@noahhendrix

Yeah so,

Using http://github.com/ernie/meta_search to power the search on the model

http://gist.github.com/506374

screenshot of error: error

@yabawock
Collaborator

After looking at the code and the stack trace, this is unrelated or only very slightly related to the check_box collection modification. If no :value_method is given Formtastic assumes that the collection elements respond to :id - which your array of strings doesn't do.

Easiest fix for the moment would be to add an :value_method => :to_s to your form code for the collection

@giddie

@yabawock => That patch fixes my issues too. However, I wonder if you'd be OK with moving that new hidden field so it's directly inside the OL, instead of being in an LI wrapper? My CSS expects there to be something visible in each LI, and an empty LI is surely not semantic anyway, right?

@noahhendrix

I thought the same thing before, so I added :value_method => :to_s, but of course then my model doesn't respond to that method properly, it just returns the object's memory stamp.

On related note, I tried again with the value_method and it doesn't seem to be mapping the :value_method properly

error

@noahhendrix

@giddie => that will break versions of IE, semantics dictate that only list items should be in OL/UL.

@giddie

Then maybe outside the OL? Do you agree that it seems odd to have a single hidden element in an LI?

@yabawock
Collaborator

@noahhendrix:
I reworked the label and value method detection in commit 8fb219c

The spec I wrote which was failing before ist now green. Please test and give some feedback if it works for you.

@giddie:
In commit 7b5ed06 I moved the hidden field to be within the fieldset for the checkbox collection but before the ordered list. This should be valid html code and work around the extra list item we had before.

@noahhendrix

@yabawock, that works flawlessly. Thanks!

@justinfrench
Owner

Closed?

@noahhendrix

For me it is, thanks again!

@justinfrench
Owner

Does any of this need to be ported to master before a new RC or 1.0 final?

@noahhendrix

It can wait, at least for me.

@yabawock
Collaborator

@justin

I don't think any of this needs to be backported. Most of it is related to the changed handling of checkbox collections. If something should be packported it is in 8fb219c - the smarter detection of the value method would provide a benefit for 1.0 without being a very intrusive change.

If nothing else pops up i'll close this in a few hours.

@giddie

@noahhendrix => The hidden field solution is perfect for me now; thanks :)

@yabawock
Collaborator

Ok, since everybody seems happy now i'm closing this.

Thanks for you ideas and feedback.

@justinfrench
Owner

Awesome work!

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