ActionView.url_for doesn't escape by default

ActionView::Helpers::UrlHelper#url_for used to escape the URLs it generated by
default.  This was most commonly seen when generating a path with multiple
query parameters, e.g.

  url_for(:controller => :foo, :action => :bar, :this => 123, :that => 456)

would return

  http://example.com/foo/bar?that=456&this=123

escaping an ampersand that shouldn't be escaped.  This is both wrong and
inconsistent with the behavior of ActionController#url_for, and is changed.

Signed-off-by: Michael Koziarski <michael@koziarski.com>

2 files changed, 14 insertions, 4 deletions

diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb
index e651bc17a9..44e7073227 100644
--- a/actionpack/lib/action_view/helpers/url_helper.rb
+++ b/actionpack/lib/action_view/helpers/url_helper.rb
@@ -83,7 +83,7 @@ module ActionView
           options
         when Hash
           options = { :only_path => options[:host].nil? }.update(options.symbolize_keys)
-          escape  = options.key?(:escape) ? options.delete(:escape) : true
+          escape  = options.key?(:escape) ? options.delete(:escape) : false
           @controller.send(:url_for, options)
         when :back
           escape = false
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index 111a7619b5..cc3b2455d7 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -22,7 +22,7 @@ class UrlHelperTest < ActionView::TestCase
 
   def test_url_for_escapes_urls
     @controller.url = "http://www.example.com?a=b&c=d"
-    assert_equal "http://www.example.com?a=b&amp;c=d", url_for(:a => 'b', :c => 'd')
+    assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd')
     assert_equal "http://www.example.com?a=b&amp;c=d", url_for(:a => 'b', :c => 'd', :escape => true)
     assert_equal "http://www.example.com?a=b&c=d", url_for(:a => 'b', :c => 'd', :escape => false)
   end
@@ -42,6 +42,16 @@ class UrlHelperTest < ActionView::TestCase
     assert_equal 'javascript:history.back()', url_for(:back)
   end
 
+  def test_url_for_from_hash_doesnt_escape_ampersand
+    @controller = TestController.new
+    @view = ActionView::Base.new
+    @view.controller = @controller
+
+    path = @view.url_for(:controller => :cheeses, :foo => :bar, :baz => :quux)
+
+    assert_equal '/cheeses?baz=quux&foo=bar', path
+  end
+
   # todo: missing test cases
   def test_button_to_with_straight_url
     assert_dom_equal "<form method=\"post\" action=\"http://www.example.com\" class=\"button-to\"><div><input type=\"submit\" value=\"Hello\" /></div></form>", button_to("Hello", "http://www.example.com")
@@ -298,7 +308,7 @@ class UrlHelperTest < ActionView::TestCase
     @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc&page=1")
     @controller.url = "http://www.example.com/weblog/show?order=desc&page=1"
     assert_equal "Showing", link_to_unless_current("Showing", { :action => "show", :controller => "weblog", :order=>'desc', :page=>'1' })
-    assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&amp;page=1")
+    assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=1")
     assert_equal "Showing", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=1")
 
     @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc")
@@ -308,7 +318,7 @@ class UrlHelperTest < ActionView::TestCase
 
     @controller.request = RequestMock.new("http://www.example.com/weblog/show?order=desc&page=1")
     @controller.url = "http://www.example.com/weblog/show?order=desc&page=2"
-    assert_equal "<a href=\"http://www.example.com/weblog/show?order=desc&amp;page=2\">Showing</a>", link_to_unless_current("Showing", { :action => "show", :controller => "weblog" })
+    assert_equal "<a href=\"http://www.example.com/weblog/show?order=desc&page=2\">Showing</a>", link_to_unless_current("Showing", { :action => "show", :controller => "weblog" })
     assert_equal "<a href=\"http://www.example.com/weblog/show?order=desc&amp;page=2\">Showing</a>", link_to_unless_current("Showing", "http://www.example.com/weblog/show?order=desc&page=2")