Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Peter Marklund September 4th, 2009 @ 07:02 AM

  Added tests for when the selection or one of the options is nil.

  • indifferent-select-options.diff 3.4 KB
• 

  José Valim September 5th, 2009 @ 07:07 PM

  Converting everything to string is likely to break things. Wouldn't be better to explicitly check for symbols?

• You flagged this item as spam.
  

  Peter Marklund September 6th, 2009 @ 08:37 AM

  It's probably unusual that you have anything other than strings or symbols as options, but if you do, it seams reasonable that the objects that you use have a sensible to_s implementation, after all, to_s will be invoked when the HTML is rendered, no? Can you give an example of a use case that would be broken?

• You flagged this item as spam.
  

  Peter Marklund September 7th, 2009 @ 09:49 AM

  Actually a more common use case for running into this issue is that the option values in the select are ActiveRecord integer ids. What's wrong with this code?

  <% form_tag do %>
    <%= select_tag(:category_id,
      options_for_select(Category.all.map { |c| [c.name, c.id] }, params[:category_id])) %>
    <%= submit_tag %>
  <% end %>
  

  What's wrong is that params[:category_id] will always fail to select an option since param values are strings, not integers. The workaround is to invoke to_i on the parameter or the option values or both. I think this is the scenario where this gotcha has come up the most often for me.

• 

  Matt Jones September 7th, 2009 @ 08:46 PM

  Two observations:

  • this code:

    if selected.respond_to?(:include?) && !selected.is_a?(String)
    

    is less specific than this:
    

    if selected.is_a?(Enumerable)
    

  The original code follows the duck-typing philosophy, with a small gotcha where String also responds_to :include?. While it's not always likely that something else could be passed in, it seems like a mistake to explicitly check for Enumerable.

  • the code in the patch gets slow for large multiple-selection lists, especially if many options are selected. It's calling :to_s on selected for each element, which could really slow things down. For instance, if you had a multivalue select with 100 elements, 50 of which were selected, this code would call to_s 5000 times vs. a correct approach (converting types before calling options_for_select) which would need only O(200) conversions. While speed isn't always the goal, a ~25x performance loss is pretty extreme.

• You flagged this item as spam.
  

  Peter Marklund September 8th, 2009 @ 12:38 AM

  Matt,
  there reason I checked for Enumerable was because I needed to use the map method. It seems reasonable to require that if you want to select multiple values you do so with an Enumerable. I even doubt that anyone is using anything than an Array in practice. Of course, to follow the duck typing pattern you would check for the presence of both those methods, but checking for Enumerable seemed cleaner and more clear to me.

  Regarding the speed, your example is a bit of an edge case. On my machine invoking to_s 5000 times on an integer seems to take about 4ms ad that doesn't seem too significant.

• 

  Michael Koziarski September 8th, 2009 @ 04:55 AM

  • State changed from “new” to “wontfix”

  You should be using collection_select for the case you've described there, it appears to work just fine. I agree with jose that there's a risk of breaking things if we just to_s things blindly.

  Given that there's a better and functional replacement for your case, and there's a risk of breakage, marking as wontfix.

• You flagged this item as spam.
  

  Peter Marklund September 8th, 2009 @ 11:07 PM

  I don't really see a practical risk of backwards incompatibility in this change. Can someone please show me a use case that breaks?

  Yes, you are right Michael, using collection_select seems to not have this issue, curiously, I haven't tracked down why. The issue with collection_select though is that it mandates the use of an object which I didn't have in my example and it would force me to use a dummy object:

  <% form_tag do %>
  <%= collection_select(:dummy, :category_id, Category.all, :name, :id, {:selected => params[:dummy][:category_id]}) %>
    <%= submit_tag %>
  <% end %>
  

  It seems a plain select_tag is better for this use case even if you then have to live with the to_s gotcha.