Ensure optional path scopes are properly handled.

2 files changed, 64 insertions, 8 deletions

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 5009ed001c..bad90df2e3 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -68,13 +68,9 @@ module ActionDispatch
           end
 
           def normalize_path(path)
-            path = nil if path == ""
-            path = "#{@scope[:path]}#{path}" if @scope[:path]
-            path = Rack::Mount::Utils.normalize_path(path) if path
-
-            raise ArgumentError, "path is required" unless path
-
-            path
+            path = "#{@scope[:path]}#{path}"
+            raise ArgumentError, "path is required" if path.empty?
+            Mapper.normalize_path(path)
           end
 
           def app
@@ -160,6 +156,14 @@ module ActionDispatch
           end
       end
 
+      # Invokes Rack::Mount::Utils.normalize path and ensure that
+      # (:locale) becomes (/:locale) instead of /(:locale).
+      def self.normalize_path(path)
+        path = Rack::Mount::Utils.normalize_path(path)
+        path.sub!(/^\/\(+\/?:/, '(/:')
+        path
+      end
+
       module Base
         def initialize(set)
           @set = set
@@ -245,7 +249,7 @@ module ActionDispatch
 
           if path = options.delete(:path)
             path_set = true
-            path, @scope[:path] = @scope[:path], Rack::Mount::Utils.normalize_path(@scope[:path].to_s + path.to_s)
+            path, @scope[:path] = @scope[:path], Mapper.normalize_path(@scope[:path].to_s + path.to_s)
           else
             path_set = false
           end
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 6a25deb40c..984cbffd9f 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -129,6 +129,16 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
         resources :rooms
       end
 
+      scope '(:locale)', :locale => /en|pl/ do
+        resources :descriptions
+      end
+
+      namespace :admin do
+        scope '(/:locale)', :locale => /en|pl/ do
+          resources :descriptions
+        end
+      end
+
       match '/info' => 'projects#info', :as => 'info'
 
       root :to => 'projects#index'
@@ -594,6 +604,48 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest
     self.host = previous_host
   end
 
+  def test_optional_scoped_path
+    with_test_routes do
+      assert_equal '/en/descriptions', descriptions_path("en")
+      assert_equal '/descriptions', descriptions_path(nil)
+      assert_equal '/en/descriptions/1', description_path("en", 1)
+      assert_equal '/descriptions/1', description_path(nil, 1)
+
+      get '/en/descriptions'
+      assert_equal 'descriptions#index', @response.body
+
+      get '/descriptions'
+      assert_equal 'descriptions#index', @response.body
+
+      get '/en/descriptions/1'
+      assert_equal 'descriptions#show', @response.body
+
+      get '/descriptions/1'
+      assert_equal 'descriptions#show', @response.body
+    end
+  end
+
+  def test_nested_optional_scoped_path
+    with_test_routes do
+      assert_equal '/admin/en/descriptions', admin_descriptions_path("en")
+      assert_equal '/admin/descriptions', admin_descriptions_path(nil)
+      assert_equal '/admin/en/descriptions/1', admin_description_path("en", 1)
+      assert_equal '/admin/descriptions/1', admin_description_path(nil, 1)
+
+      get '/admin/en/descriptions'
+      assert_equal 'admin/descriptions#index', @response.body
+
+      get '/admin/descriptions'
+      assert_equal 'admin/descriptions#index', @response.body
+
+      get '/admin/en/descriptions/1'
+      assert_equal 'admin/descriptions#show', @response.body
+
+      get '/admin/descriptions/1'
+      assert_equal 'admin/descriptions#show', @response.body
+    end
+  end
+
   private
     def with_test_routes
       real_routes, temp_routes = ActionController::Routing::Routes, Routes