This project is archived and is in readonly mode.
DateHelper#date_or_time_select and DateHelper#default_time_from_options modify options hash
Reported by Clemens Kofler | July 17th, 2008 @ 12:30 AM
While working on a plugin that monkey patched some of Rails' DateHelper methods, I've stumbled across a small but potentially very confusing bug in date_or_time_select.
Here's the relevant code of the method:
order = (options[:order] ||= [:year, :month, :day])
# ...
[:day, :month, :year].each { |o| order.unshift(o) unless order.include?(o) }
# ...
[:hour, :minute, :second].each { |o| order.delete(o); order.push(o) }
The problem is that options[:order] is modified directly instead of using a cloned or duped version of it.
Let's assume the following snippet of code:
# in some configuration
date_helper_config = { :order => [ :year, :month, :day ] }
# in some view
<%= object.datetime_select :some_date, date_helper_config %>
<%= date_helper_config %>
In this case, date_helper_config all of a sudden has turned into [ :year, :month, :day, :hour, :minute, :second ].
I've tried duping or cloning the options before passing them in but since they only make shallow copies and keep the references in place, that doesn't change the problem. Only a workaround with deep cloning (as can be seen e.g. at http://zeljkofilipin.com/2006/02... would work in this case.
Since I think it's fairly usual to dup parameters when you intend to modify them I've created a patch that does exactly that - it just dups the options[:order] before using it. It also modifies the default_time_from_options method to prevent it from changing options[:default].
I've attached the failing test plus the patched date helper. All existing tests pass.
Comments and changes to this ticket
-
josh August 7th, 2008 @ 06:48 AM
- Assigned user set to josh
- Milestone cleared.
Dude! Why didn't you tell me about this one.
Lets roll it in with our big refactor.
-
josh August 7th, 2008 @ 06:48 AM
- State changed from new to open
- Tag changed from actionpack, bug, helper, patch, tested, tests to actionpack, bug, helper, patch, tests
-
Clemens Kofler August 7th, 2008 @ 10:24 AM
Sorry, totally forgot about this one. It's in now: http://github.com/josh/rails/com...
-
josh August 7th, 2008 @ 04:51 PM
- State changed from open to resolved
Resolving as we are committing shortly.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>