Fixed regression - unable to use a range as choices for form.select. #4574

Merged
merged 1 commit into from Jan 22, 2012

Conversation

Projects
None yet
3 participants
Contributor

iHiD commented Jan 21, 2012

As of 3.2, form.select(:foobar, 1..3) fails with:

NoMethodError: undefined method `empty?' for 1..3:Range
    actionpack/lib/action_view/helpers/tags/select.rb:21:in `render'

However, this still works fine with select_tag and options_for.

I've added tests for both cases and added a one-line fix to convert the range to an array for form.select.

@@ -4,6 +4,7 @@ module Tags
class Select < Base #:nodoc:
def initialize(object_name, method_name, template_object, choices, options, html_options)
@choices = choices
+ @choices = @choices.to_a if @choices.is_a?(Range)
@josevalim

josevalim Jan 21, 2012

Contributor

Let's simply do @choices = choices.to_a, wdyt?

@iHiD

iHiD Jan 21, 2012

Contributor

Currently @choices can be a string, array or hash, so it breaks if we force it into an array. I'm happy to refactor the render method, which imho isn't particularly readable atm, but I think the original one-liner is a good interim solution for those relying ranges currently. Thoughts?

@josevalim

josevalim Jan 22, 2012

Contributor

Hrm, so the current solution is fine for Rails 3.2, but on Rails master, I believe we can do @choices = choices.respond_to?(:to_a) ? choices.to_a : choices. In any case, I am merging this, feel free to submit another patch for master that uses respond_to, thanks.

josevalim added a commit that referenced this pull request Jan 22, 2012

Merge pull request #4574 from ihid/master
Fixed regression - unable to use a range as choices for form.select.

@josevalim josevalim merged commit c3aaaca into rails:master Jan 22, 2012

@josevalim josevalim merged commit 2ee8c6a into rails:master Jan 22, 2012

Contributor

josevalim commented Jan 22, 2012

Could you please send a pull request for 3-2-stable as well? I have tried to backport but it got merge conflicts. Thanks!

Contributor

iHiD commented Jan 22, 2012

Thanks for sorting master. The 3-2-stable version is at #4604
FYI - it is so different because the select method is still in form_options_helper.rb

simonc commented Jan 25, 2012

I could have been cool to have empty? on Range instead ? WDYT ?

Contributor

iHiD commented Jan 25, 2012

Yeah, I actually considered that initially, but thought as a short fix it was better to not add a method to a core class.

Saying that, if people like that idea I'll happily implement it instead...?

Contributor

iHiD commented Jan 26, 2012

Any thoughts on this, @josevalim?

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