When a dynamic :controller segment is present in the path add a Regex…

…p constraint that allow matching on multiple path segments.

Using a namespace block isn't compatible with dynamic routes so we
raise an ArgumentError if we detect a :module present in the scope.

[#5052 state:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -65,6 +65,16 @@ def extract_path_and_options(args)
               end
             end
 
+            if path.match(':controller')
+              raise ArgumentError, ":controller segment is not allowed within a namespace block" if @scope[:module]
+
+              # Add a default constraint for :controller path segments that matches namespaced
+              # controllers with default routes like :controller/:action/:id(.:format), e.g:
+              # GET /admin/products/show/1
+              # => { :controller => 'admin/products', :action => 'show', :id => '1' }
+              options.reverse_merge!(:controller => /.+?/)
+            end
+
             path = normalize_path(path)
             path_without_format = path.sub(/\(\.:format\)$/, '')
 
@@ -93,7 +103,7 @@ def normalize_path(path)
 
           def app
             Constraints.new(
-              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults, :module => @scope[:module]),
+              to.respond_to?(:call) ? to : Routing::RouteSet::Dispatcher.new(:defaults => defaults),
               blocks,
               @set.request_class
             )
@@ -135,8 +145,11 @@ def default_controller_and_action
               defaults[:controller] ||= default_controller
               defaults[:action]     ||= default_action
 
-              defaults.delete(:controller) if defaults[:controller].blank?
-              defaults.delete(:action)     if defaults[:action].blank?
+              defaults.delete(:controller) if defaults[:controller].blank? || defaults[:controller].is_a?(Regexp)
+              defaults.delete(:action)     if defaults[:action].blank? || defaults[:action].is_a?(Regexp)
+
+              defaults[:controller] = defaults[:controller].to_s if defaults.key?(:controller)
+              defaults[:action] = defaults[:action].to_s if defaults.key?(:action)
 
               if defaults[:controller].blank? && segment_keys.exclude?("controller")
                 raise ArgumentError, "missing :controller"
@@ -185,15 +198,15 @@ def to
 
           def default_controller
             if @options[:controller]
-              @options[:controller].to_s
+              @options[:controller]
             elsif @scope[:controller]
-              @scope[:controller].to_s
+              @scope[:controller]
             end
           end
 
           def default_action
             if @options[:action]
-              @options[:action].to_s
+              @options[:action]
             end
           end
       end

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -14,7 +14,6 @@ class Dispatcher #:nodoc:
         def initialize(options={})
           @defaults = options[:defaults]
           @glob_param = options.delete(:glob)
-          @module = options.delete(:module)
           @controllers = {}
         end
 
@@ -39,12 +38,11 @@ def prepare_params!(params)
         # we should raise an error in case it's not found, because it usually means
         # an user error. However, if the controller was retrieved through a dynamic
         # segment, as in :controller(/:action), we should simply return nil and
-        # delegate the control back to Rack cascade. Besides, if this is not a default 
+        # delegate the control back to Rack cascade. Besides, if this is not a default
         # controller, it means we should respect the @scope[:module] parameter.
         def controller(params, default_controller=true)
           if params && params.key?(:controller)
-            controller_param = @module && !default_controller ?
-              "#{@module}/#{params[:controller]}" : params[:controller]
+            controller_param = params[:controller]
             controller_reference(controller_param)
           end
         rescue NameError => e

actionpack/test/dispatch/routing_test.rb

@@ -323,7 +323,7 @@ def self.matches?(request)
         end
       end
 
-      match "whatever/:controller(/:action(/:id))"
+      match "whatever/:controller(/:action(/:id))", :id => /\d+/
 
       resource :profile do
         get :settings
@@ -349,7 +349,6 @@ def self.matches?(request)
       namespace :private do
         root :to => redirect('/private/index')
         match "index", :to => 'private#index'
-        match ":controller(/:action(/:id))"
       end
 
       scope :only => [:index, :show] do
@@ -527,15 +526,14 @@ def test_namespace_redirect
   end
 
   def test_namespace_with_controller_segment
-    with_test_routes do
-      get '/private/foo'
-      assert_equal 'private/foo#index', @response.body
-
-      get '/private/foo/bar'
-      assert_equal 'private/foo#bar', @response.body
-
-      get '/private/foo/bar/1'
-      assert_equal 'private/foo#bar', @response.body
+    assert_raise(ArgumentError) do
+      self.class.stub_controllers do |routes|
+        routes.draw do
+          namespace :admin do
+            match '/:controller(/:action(/:id(.:format)))'
+          end
+        end
+      end
     end
   end
 
@@ -1354,6 +1352,22 @@ def test_url_generator_for_generic_route
     end
   end
 
+  def test_url_generator_for_namespaced_generic_route
+    with_test_routes do
+      get 'whatever/foo/bar/show'
+      assert_equal 'foo/bar#show', @response.body
+
+      get 'whatever/foo/bar/show/1'
+      assert_equal 'foo/bar#show', @response.body
+
+      assert_equal 'http://www.example.com/whatever/foo/bar/show',
+        url_for(:controller => "foo/bar", :action => "show")
+
+      assert_equal 'http://www.example.com/whatever/foo/bar/show/1',
+        url_for(:controller => "foo/bar", :action => "show", :id => '1')
+    end
+  end
+
   def test_assert_recognizes_account_overview
     with_test_routes do
       assert_recognizes({:controller => "account", :action => "overview"}, "/account/overview")