Skip to content

Fixes an issue when creating a date select with too many options. #239

Closed
wants to merge 1 commit into from

7 participants

@dlt
dlt commented Mar 27, 2011

This commit fixes issue #6627 (https://rails.lighthouseapp.com/projects/8994/tickets/6627-server-hanging-when-using-extreme-values-for-date_select-start_year). The issue occurs because the build_options method loops forever when the range between start_date and end_date is too big.

This commit adds a verification to #build_options and raises an error when the range is greater than 1000.

@sikachu sikachu commented on the diff Jun 15, 2011
actionpack/lib/action_view/helpers/date_helper.rb
@@ -810,6 +810,12 @@ module ActionView
step = options.delete(:step) || 1
options.reverse_merge!({:leading_zeros => true})
leading_zeros = options.delete(:leading_zeros)
+ limit = (stop - start).abs
+ max_options = 1000
+
+ if limit / step.abs > max_options
+ raise ArgumentError, "Cannot build too many options for select."
+ end
@sikachu
Ruby on Rails member
sikachu added a note Jun 15, 2011

I feel like the limit variable name should be more clear, something like number_of_options. Also, max_options can surely be renamed and make it as constant somewhere. I think that should be better.

Also, I would love if the exception message become more clearer too. Something like "There're too many number of years option to be built. Are you sure you aren't mistyped something?" would be more friendly.

Thank you so much for patching this edge case. Please update your patch and force push to your branch, so it will stay as single concise commit. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sikachu
Ruby on Rails member
sikachu commented Jun 15, 2011

Oh, an also if you can update the CHANGELOG for the new limitation that would be awesome.

@dost
dost commented Jul 23, 2011

I think hard limit 1000 is bad for any application with historic dates. Maybe including max limit as optional parameter would help in such situations.

@andmej
andmej commented Aug 27, 2011

@sikachu, what would be the appropriate place to add the max_options limit?

@dost making the limit optional wouldn't fix the issue at all.

@henrikhodne

@andmej I think he meant optional parameter as in you don't have to supply it, and if you don't it defaults to, say, 1000.

@libo
libo commented Sep 14, 2011

Since there hasn't been so much action on this issue I wrote my patch, hope it's fine.
libo@631eca1

@jeremy jeremy added a commit that closed this pull request Oct 9, 2011
Liborio Cannici Fixes an issue when creating a date select with too many options.
Inspired by dlt dlt@9e61563

Closes #239.

Conflicts:

	actionpack/CHANGELOG
114218e
@jeremy jeremy closed this in 114218e Oct 9, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.