Merge pull request #4802 from carlosantoniodasilva/collection-helpers-refactor

Refactor select helper and remove eval

3 files changed, 28 insertions, 14 deletions

diff --git a/actionpack/lib/action_view/helpers/form_options_helper.rb b/actionpack/lib/action_view/helpers/form_options_helper.rb
index c8811e3b10..c4cdfef4a2 100644
--- a/actionpack/lib/action_view/helpers/form_options_helper.rb
+++ b/actionpack/lib/action_view/helpers/form_options_helper.rb
@@ -334,7 +334,7 @@ module ActionView
         end.join("\n").html_safe
       end
 
-      # Returns a string of option tags that have been compiled by iterating over the +collection+ and assigning 
+      # Returns a string of option tags that have been compiled by iterating over the +collection+ and assigning
       # the result of a call to the +value_method+ as the option value and the +text_method+ as the option text.
       # Example:
       #   options_from_collection_for_select(@people, 'id', 'name')
@@ -418,9 +418,9 @@ module ActionView
       # wrap the output in an appropriate <tt><select></tt> tag.
       def option_groups_from_collection_for_select(collection, group_method, group_label_method, option_key_method, option_value_method, selected_key = nil)
         collection.map do |group|
-          group_label_string = eval("group.#{group_label_method}")
+          group_label_string = group.send(group_label_method)
           "<optgroup label=\"#{ERB::Util.html_escape(group_label_string)}\">" +
-            options_from_collection_for_select(eval("group.#{group_method}"), option_key_method, option_value_method, selected_key) +
+            options_from_collection_for_select(group.send(group_method), option_key_method, option_value_method, selected_key) +
             '</optgroup>'
         end.join.html_safe
       end
diff --git a/actionpack/lib/action_view/helpers/tags/collection_select.rb b/actionpack/lib/action_view/helpers/tags/collection_select.rb
index f84140d8d0..ec78e6e5f9 100644
--- a/actionpack/lib/action_view/helpers/tags/collection_select.rb
+++ b/actionpack/lib/action_view/helpers/tags/collection_select.rb
@@ -12,9 +12,14 @@ module ActionView
         end
 
         def render
-          selected_value = @options.has_key?(:selected) ? @options[:selected] : value(@object)
+          option_tags_options = {
+            :selected => @options.fetch(:selected) { value(@object) },
+            :disabled => @options[:disabled]
+          }
+
           select_content_tag(
-            options_from_collection_for_select(@collection, @value_method, @text_method, :selected => selected_value, :disabled => @options[:disabled]), @options, @html_options
+            options_from_collection_for_select(@collection, @value_method, @text_method, option_tags_options),
+            @options, @html_options
           )
         end
       end
diff --git a/actionpack/lib/action_view/helpers/tags/select.rb b/actionpack/lib/action_view/helpers/tags/select.rb
index 02b790db4e..53a108b7e6 100644
--- a/actionpack/lib/action_view/helpers/tags/select.rb
+++ b/actionpack/lib/action_view/helpers/tags/select.rb
@@ -11,21 +11,30 @@ module ActionView
         end
 
         def render
-          selected_value = @options.has_key?(:selected) ? @options[:selected] : value(@object)
+          option_tags_options = {
+            :selected => @options.fetch(:selected) { value(@object) },
+            :disabled => @options[:disabled]
+          }
 
-          # Grouped choices look like this:
-          #
-          #   [nil, []]
-          #   { nil => [] }
-          #
-          if !@choices.empty? && @choices.first.respond_to?(:last) && Array === @choices.first.last
-            option_tags = grouped_options_for_select(@choices, :selected => selected_value, :disabled => @options[:disabled])
+          option_tags = if grouped_choices?
+            grouped_options_for_select(@choices, option_tags_options)
           else
-            option_tags = options_for_select(@choices, :selected => selected_value, :disabled => @options[:disabled])
+            options_for_select(@choices, option_tags_options)
           end
 
           select_content_tag(option_tags, @options, @html_options)
         end
+
+        private
+
+        # Grouped choices look like this:
+        #
+        #   [nil, []]
+        #   { nil => [] }
+        #
+        def grouped_choices?
+          !@choices.empty? && @choices.first.respond_to?(:last) && Array === @choices.first.last
+        end
       end
     end
   end