Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

DateHelper#date_or_time_select and DateHelper#default_time_from_options modify options hash

#638

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.

Reported by Clemens Kofler · July 17th, 2008 @ 12:30 AM

State: resolved
Milestone: none
Assigned to: josh josh
Importance: Low

Activity

  1. josh
    josh
    • 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.

    August 7th, 2008 @ 06:48 AM

  2. josh
    josh
    • State changed from new to open
    • Tag changed from actionpack, bug, helper, patch, tested, tests to actionpack, bug, helper, patch, tests

    August 7th, 2008 @ 06:48 AM

  3. Clemens Kofler
    Clemens Kofler

    Sorry, totally forgot about this one. It's in now: http://github.com/josh/rails/com...

    August 7th, 2008 @ 10:24 AM

  4. josh
    josh
    • State changed from open to resolved

    Resolving as we are committing shortly.

    August 7th, 2008 @ 04:51 PM