url_for no longer escapes HTML, the :escape option is also gone

Rationale: url_for is just a path/URL generator, it is the responsability of the caller to escape conveniently HTML needs it, JavaScript needs different escaping, a text mail needs no escaping at all, etc.

5 files changed, 14 insertions, 33 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index cdd64edec0..80be915407 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *Rails 3.0.0 [Release Candidate] (unreleased)*
 
+* url_for returns always unescaped strings, and the :escape option is gone. [fxn]
+
 * Added accept-charset parameter and _snowman hidden field to force the contents
   of Rails POSTed forms to be in UTF-8 [Yehuda Katz]
 
diff --git a/actionpack/lib/action_view/helpers/form_tag_helper.rb b/actionpack/lib/action_view/helpers/form_tag_helper.rb
index efa1446d96..7fea5eb055 100644
--- a/actionpack/lib/action_view/helpers/form_tag_helper.rb
+++ b/actionpack/lib/action_view/helpers/form_tag_helper.rb
@@ -529,6 +529,8 @@ module ActionView
         def html_options_for_form(url_for_options, options, *parameters_for_url)
           returning options.stringify_keys do |html_options|
             html_options["enctype"] = "multipart/form-data" if html_options.delete("multipart")
+            # The following URL is unescaped, this is just a hash of options, and it is the
+            # responsability of the caller to escape all the values.
             html_options["action"]  = url_for(url_for_options, *parameters_for_url)
             html_options["accept-charset"] = "UTF-8"
             html_options["data-remote"] = true if html_options.delete("remote")
diff --git a/actionpack/lib/action_view/helpers/prototype_helper.rb b/actionpack/lib/action_view/helpers/prototype_helper.rb
index 678e6348b6..5c0ff5d59c 100644
--- a/actionpack/lib/action_view/helpers/prototype_helper.rb
+++ b/actionpack/lib/action_view/helpers/prototype_helper.rb
@@ -131,7 +131,7 @@ module ActionView
 
         url_options = options[:url]
         url_options = url_options.merge(:escape => false) if url_options.is_a?(Hash)
-        function << "'#{escape_javascript(url_for(url_options))}'"
+        function << "'#{html_escape(escape_javascript(url_for(url_options)))}'"
         function << ", #{javascript_options})"
 
         function = "#{options[:before]}; #{function}" if options[:before]
diff --git a/actionpack/lib/action_view/helpers/url_helper.rb b/actionpack/lib/action_view/helpers/url_helper.rb
index cbde9b94a7..7d1d00d1fa 100644
--- a/actionpack/lib/action_view/helpers/url_helper.rb
+++ b/actionpack/lib/action_view/helpers/url_helper.rb
@@ -38,9 +38,6 @@ module ActionView
       # <tt>:only_path</tt> is <tt>true</tt> so you'll get the relative "/controller/action"
       # instead of the fully qualified URL like "http://example.com/controller/action".
       #
-      # When called from a view, +url_for+ returns an HTML escaped url. If you
-      # need an unescaped url, pass <tt>:escape => false</tt> in the +options+.
-      #
       # ==== Options
       # * <tt>:anchor</tt> - Specifies the anchor name to be appended to the path.
       # * <tt>:only_path</tt> - If true, returns the relative URL (omitting the protocol, host name, and port) (<tt>true</tt> by default unless <tt>:host</tt> is specified).
@@ -50,7 +47,6 @@ module ActionView
       # * <tt>:protocol</tt> - Overrides the default (current) protocol if provided.
       # * <tt>:user</tt> - Inline HTTP authentication (only plucked out if <tt>:password</tt> is also present).
       # * <tt>:password</tt> - Inline HTTP authentication (only plucked out if <tt>:user</tt> is also present).
-      # * <tt>:escape</tt> - Determines whether the returned URL will be HTML escaped or not (<tt>true</tt> by default).
       #
       # ==== Relying on named routes
       #
@@ -72,10 +68,7 @@ module ActionView
       #   <%= url_for(:action => 'play', :anchor => 'player') %>
       #   # => /messages/play/#player
       #
-      #   <%= url_for(:action => 'checkout', :anchor => 'tax&ship') %>
-      #   # => /testing/jump/#tax&amp;ship
-      #
-      #   <%= url_for(:action => 'checkout', :anchor => 'tax&ship', :escape => false) %>
+      #   <%= url_for(:action => 'jump', :anchor => 'tax&ship') %>
       #   # => /testing/jump/#tax&ship
       #
       #   <%= url_for(Workshop.new) %>
@@ -100,21 +93,17 @@ module ActionView
         options ||= {}
         url = case options
         when String
-          escape = true
           options
         when Hash
           options = { :only_path => options[:host].nil? }.update(options.symbolize_keys)
-          escape  = options.key?(:escape) ? options.delete(:escape) : true
           super
         when :back
-          escape = false
           controller.request.env["HTTP_REFERER"] || 'javascript:history.back()'
         else
-          escape = false
           polymorphic_path(options)
         end
 
-        escape ? escape_once(url).html_safe : url
+        url
       end
 
       # Creates a link tag of the given +name+ using a URL created by the set
@@ -254,8 +243,8 @@ module ActionView
             tag_options = nil
           end
 
-          href_attr = "href=\"#{url}\"" unless href
-          "<a #{href_attr}#{tag_options}>#{ERB::Util.h(name || url)}</a>".html_safe
+          href_attr = "href=\"#{escape_once(url)}\"" unless href
+          "<a #{href_attr}#{tag_options}>#{html_escape(name || url)}</a>".html_safe
         end
       end
 
@@ -574,7 +563,7 @@ module ActionView
                 "in a #request method"
         end
 
-        url_string = CGI.unescapeHTML(url_for(options))
+        url_string = url_for(options)
 
         # We ignore any extra parameters in the request_uri if the
         # submitted url doesn't have any either.  This lets the function
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index 897228677b..035f501f03 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -40,20 +40,8 @@ class UrlHelperTest < ActiveSupport::TestCase
   end
   alias url_hash hash_for
 
-  def test_url_for_escapes_urls
-    assert_equal "/?a=b&amp;c=d", url_for(abcd)
-    assert_equal "/?a=b&amp;c=d", url_for(abcd(:escape => true))
-    assert_equal "/?a=b&c=d", url_for(abcd(:escape => false))
-  end
-
-  def test_url_for_escaping_is_safety_aware
-    assert url_for(abcd(:escape => true)).html_safe?, "escaped urls should be html_safe?"
-    assert !url_for(abcd(:escape => false)).html_safe?, "non-escaped urls should not be html_safe?"
-  end
-
-  def test_url_for_escapes_url_once
-    assert_equal "/?a=b&amp;c=d", url_for("/?a=b&amp;c=d")
-    assert_equal "/?a=b&amp;c=d", url_for(abcd)
+  def test_url_for_does_not_escape_urls
+    assert_equal "/?a=b&c=d", url_for(abcd)
   end
 
   def test_url_for_with_back
@@ -151,7 +139,7 @@ class UrlHelperTest < ActiveSupport::TestCase
   end
 
   def test_link_tag_with_query_and_no_name
-    link = link_to(nil, "http://www.example.com?q1=v1&amp;q2=v2")
+    link = link_to(nil, "http://www.example.com?q1=v1&q2=v2")
     expected = %{<a href="http://www.example.com?q1=v1&amp;q2=v2">http://www.example.com?q1=v1&amp;q2=v2</a>}
     assert_dom_equal expected, link
   end
@@ -308,7 +296,7 @@ class UrlHelperTest < ActiveSupport::TestCase
     @request = request_for_url("/?order=desc&page=1")
 
     assert current_page?(hash_for(:order => "desc", :page => "1"))
-    assert current_page?("http://www.example.com/?order=desc&amp;page=1")
+    assert current_page?("http://www.example.com/?order=desc&page=1")
   end
 
   def test_link_unless_current