From e6e056c2c141ec94eb8e79a30ee766f77fdaf30d Mon Sep 17 00:00:00 2001
From: Paul Grayson <paul@pololu.com>
Date: Fri, 14 Aug 2015 15:19:56 -0700
Subject: In url_for, never append ? when the query string is empty anyway. It
 used to behave like this:

    url_for(controller: 'x', action: 'y', q: {})
    # -> "/x/y?"

We previously avoided empty query strings in most cases by removing
nil values, then checking whether params was empty.  But as you can
see above, even non-empty params can yield an empty query string.  So
I changed the code to just directly check whether the query string
ended up empty.

(To make everything more consistent, the "removing nil values"
functionality should probably move to ActionPack's Hash#to_query, the
place where empty hashes and arrays get removed.  However, this would
change a lot more behavior.)
---
 actionpack/CHANGELOG.md                         | 4 ++++
 actionpack/lib/action_dispatch/http/url.rb      | 3 ++-
 actionpack/test/dispatch/url_generation_test.rb | 7 +++++++
 actionview/test/template/url_helper_test.rb     | 4 ++++
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 4f95a9bab9..833aa48258 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   In url_for, never append a question mark to the URL when the query string
+    is empty anyway.  (It used to do that when called like `url_for(controller:
+    'x', action: 'y', q: {})`.)
+
 *   Catch invalid UTF-8 querystring values and respond with BadRequest
 
     Check querystring params for invalid UTF-8 characters, and raise an
diff --git a/actionpack/lib/action_dispatch/http/url.rb b/actionpack/lib/action_dispatch/http/url.rb
index 92b10b6d3b..37f41ae988 100644
--- a/actionpack/lib/action_dispatch/http/url.rb
+++ b/actionpack/lib/action_dispatch/http/url.rb
@@ -81,7 +81,8 @@ module ActionDispatch
         def add_params(path, params)
           params = { params: params } unless params.is_a?(Hash)
           params.reject! { |_,v| v.to_param.nil? }
-          path << "?#{params.to_query}" unless params.empty?
+          query = params.to_query
+          path << "?#{query}" unless query.empty?
         end
 
         def add_anchor(path, anchor)
diff --git a/actionpack/test/dispatch/url_generation_test.rb b/actionpack/test/dispatch/url_generation_test.rb
index fd4ede4d1b..8c9782bb90 100644
--- a/actionpack/test/dispatch/url_generation_test.rb
+++ b/actionpack/test/dispatch/url_generation_test.rb
@@ -129,6 +129,13 @@ module TestUrlGeneration
       )
     end
 
+    test "generating URLS with empty querystring" do
+      assert_equal "/bars.json", bars_path(
+        a: {},
+        format: 'json'
+      )
+    end
+
   end
 end
 
diff --git a/actionview/test/template/url_helper_test.rb b/actionview/test/template/url_helper_test.rb
index 50b7865f88..b8f4d13812 100644
--- a/actionview/test/template/url_helper_test.rb
+++ b/actionview/test/template/url_helper_test.rb
@@ -38,6 +38,10 @@ class UrlHelperTest < ActiveSupport::TestCase
     assert_equal "/?a=b&c=d", url_for(hash_for(a: :b, c: :d))
   end
 
+  def test_url_for_does_not_include_empty_hashes
+    assert_equal "/", url_for(hash_for(a: {}))
+  end
+
   def test_url_for_with_back
     referer = 'http://www.example.com/referer'
     @controller = Struct.new(:request).new(Struct.new(:env).new("HTTP_REFERER" => referer))
-- 
cgit v1.2.3