Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 22, 2012

Conversation

iHiD
Copy link
Contributor

@iHiD 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
@josevalim
Copy link
Contributor

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

@iHiD
Copy link
Contributor Author

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
Copy link
Contributor

simonc commented Jan 25, 2012

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

@iHiD
Copy link
Contributor Author

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...?

@iHiD
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants