Merge pull request #4672 from carlosantoniodasilva/av-url-helper

Improvements on some url helper methods and tests

2 files changed, 21 insertions, 31 deletions

diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb
index d27d49821b..b5fc882e31 100644
--- a/actionpack/lib/action_view/helpers/url_helper.rb
+++ b/actionpack/lib/action_view/helpers/url_helper.rb
@@ -323,30 +323,24 @@ module ActionView
       #   #
       def button_to(name, options = {}, html_options = {})
         html_options = html_options.stringify_keys
-        convert_boolean_attributes!(html_options, %w( disabled ))
+        convert_boolean_attributes!(html_options, %w(disabled))
 
-        method_tag = ''
-        if (method = html_options.delete('method')) && %w{put delete}.include?(method.to_s)
-          method_tag = method_tag(method)
-        end
+        url    = options.is_a?(String) ? options : url_for(options)
+        remote = html_options.delete('remote')
 
-        form_method = method.to_s == 'get' ? 'get' : 'post'
+        method     = html_options.delete('method').to_s
+        method_tag = %w{put delete}.include?(method) ? method_tag(method) : ""
+
+        form_method  = method == 'get' ? 'get' : 'post'
         form_options = html_options.delete('form') || {}
         form_options[:class] ||= html_options.delete('form_class') || 'button_to'
-
-        remote = html_options.delete('remote')
+        form_options.merge!(:method => form_method, :action => url)
+        form_options.merge!("data-remote" => "true") if remote
 
         request_token_tag = form_method == 'post' ? token_tag : ''
 
-        url = options.is_a?(String) ? options : self.url_for(options)
-        name ||= url
-
         html_options = convert_options_to_data_attributes(options, html_options)
-
-        html_options.merge!("type" => "submit", "value" => name)
-
-        form_options.merge!(:method => form_method, :action => url)
-        form_options.merge!("data-remote" => "true") if remote
+        html_options.merge!("type" => "submit", "value" => name || url)
 
         "#{tag(:form, form_options, true)}<div>#{method_tag}#{tag("input", html_options)}#{request_token_tag}</div></form>".html_safe
       end
@@ -596,11 +590,7 @@ module ActionView
         # We ignore any extra parameters in the request_uri if the
         # submitted url doesn't have any either. This lets the function
         # work with things like ?order=asc
-        if url_string.index("?")
-          request_uri = request.fullpath
-        else
-          request_uri = request.path
-        end
+        request_uri = url_string.index("?") ? request.fullpath : request.path
 
         if url_string =~ /^\w+:\/\//
           url_string == "#{request.protocol}#{request.host_with_port}#{request_uri}"
@@ -630,12 +620,12 @@ module ActionView
         end
 
         def link_to_remote_options?(options)
-          options.is_a?(Hash) && options.key?('remote') && options.delete('remote')
+          options.is_a?(Hash) && options.delete('remote')
         end
 
         def add_method_to_attributes!(html_options, method)
           if method && method.to_s.downcase != "get" && html_options["rel"] !~ /nofollow/
-            html_options["rel"] = "#{html_options["rel"]} nofollow".strip
+            html_options["rel"] = "#{html_options["rel"]} nofollow".lstrip
           end
           html_options["data-method"] = method
         end
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index cf4dafbac4..37ec0e323d 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -31,13 +31,13 @@ class UrlHelperTest < ActiveSupport::TestCase
 
   setup :_prepare_context
 
-  def hash_for(opts = [])
-    ActiveSupport::OrderedHash[*([:controller, "foo", :action, "bar"].concat(opts))]
+  def hash_for(options = {})
+    { :controller => "foo", :action => "bar" }.merge!(options)
   end
   alias url_hash hash_for
 
   def test_url_for_does_not_escape_urls
-    assert_equal "/?a=b&c=d", url_for(hash_for([:a, :b, :c, :d]))
+    assert_equal "/?a=b&c=d", url_for(hash_for(:a => :b, :c => :d))
   end
 
   def test_url_for_with_back
@@ -168,7 +168,7 @@ class UrlHelperTest < ActiveSupport::TestCase
   end
 
   def test_link_tag_with_host_option
-    hash = hash_for([:host, "www.example.com"])
+    hash = hash_for(:host => "www.example.com")
     expected = %q{<a href="http://www.example.com/">Test Link</a>}
     assert_dom_equal(expected, link_to('Test Link', hash))
   end
@@ -343,7 +343,7 @@ class UrlHelperTest < ActiveSupport::TestCase
   def test_current_page_with_params_that_match
     @request = request_for_url("/?order=desc&page=1")
 
-    assert current_page?(hash_for([:order, "desc", :page, "1"]))
+    assert current_page?(hash_for(:order => "desc", :page => "1"))
     assert current_page?("http://www.example.com/?order=desc&page=1")
   end
 
@@ -371,20 +371,20 @@ class UrlHelperTest < ActiveSupport::TestCase
     @request = request_for_url("/?order=desc&page=1")
 
     assert_equal "Showing",
-      link_to_unless_current("Showing", hash_for([:order, 'desc', :page, '1']))
+      link_to_unless_current("Showing", hash_for(:order => 'desc', :page => '1'))
     assert_equal "Showing",
       link_to_unless_current("Showing", "http://www.example.com/?order=desc&page=1")
 
     @request = request_for_url("/?order=desc")
 
     assert_equal %{<a href="/?order=asc">Showing</a>},
-      link_to_unless_current("Showing", hash_for([:order, :asc]))
+      link_to_unless_current("Showing", hash_for(:order => :asc))
     assert_equal %{<a href="http://www.example.com/?order=asc">Showing</a>},
       link_to_unless_current("Showing", "http://www.example.com/?order=asc")
 
     @request = request_for_url("/?order=desc")
     assert_equal %{<a href="/?order=desc&amp;page=2\">Showing</a>},
-      link_to_unless_current("Showing", hash_for([:order, "desc", :page, 2]))
+      link_to_unless_current("Showing", hash_for(:order => "desc", :page => 2))
     assert_equal %{<a href="http://www.example.com/?order=desc&amp;page=2">Showing</a>},
       link_to_unless_current("Showing", "http://www.example.com/?order=desc&page=2")