Merge pull request #9039 from senny/warn_on_controller_option_with_ruby_constant_syntax

ruby constant syntax is not supported as routing `:controller` option.

4 files changed, 72 insertions, 9 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index fec35a36bd..c1181466f9 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,18 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   We don't support the `:controller` option for route definitions
+    with the ruby constant notation. This will now result in an
+    `ArgumentError`.
+
+    Example:
+            # This raises an ArgumentError:
+            resources :posts, :controller => "Admin::Posts"
+
+            # Use directory notation instead:
+            resources :posts, :controller => "admin/posts"
+
+    *Yves Senn*
+
 *   `assert_template` can be used to verify the locals of partials,
     which live inside a directory.
     Fixes #8516.
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 82ef1d0333..34f5f80d4d 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -246,6 +246,12 @@ module ActionDispatch
                 raise ArgumentError, "missing :action"
               end
 
+              if controller.is_a?(String) && controller !~ /\A[a-z_\/]+\z/
+                message = "'#{controller}' is not a supported controller name. This can lead to potential routing problems."
+                message << " See http://guides.rubyonrails.org/routing.html#specifying-a-controller-to-use"
+                raise ArgumentError, message
+              end
+
               hash = {}
               hash[:controller] = controller unless controller.blank?
               hash[:action]     = action unless action.blank?
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 9f31ce8127..fb1b8526d0 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -2833,21 +2833,52 @@ class TestNamespaceWithControllerOption < ActionDispatch::IntegrationTest
     end
   end
 
-  DefaultScopeRoutes = ActionDispatch::Routing::RouteSet.new
-  DefaultScopeRoutes.draw do
-    namespace :admin do
-      resources :storage_files, :controller => "StorageFiles"
-    end
+  def draw(&block)
+    @app = ActionDispatch::Routing::RouteSet.new
+    @app.draw(&block)
   end
 
-  def app
-    DefaultScopeRoutes
-  end
+  def test_valid_controller_options_inside_namespace
+    draw do
+      namespace :admin do
+        resources :storage_files, :controller => "storage_files"
+      end
+    end
 
-  def test_controller_options
     get '/admin/storage_files'
     assert_equal "admin/storage_files#index", @response.body
   end
+
+  def test_resources_with_valid_namespaced_controller_option
+    draw do
+      resources :storage_files, :controller => 'admin/storage_files'
+    end
+
+    get 'storage_files'
+    assert_equal "admin/storage_files#index", @response.body
+  end
+
+  def test_warn_with_ruby_constant_syntax_controller_option
+    e = assert_raise(ArgumentError) do
+      draw do
+        namespace :admin do
+          resources :storage_files, :controller => "StorageFiles"
+        end
+      end
+    end
+
+    assert_match "'admin/StorageFiles' is not a supported controller name", e.message
+  end
+
+  def test_warn_with_ruby_constant_syntax_namespaced_controller_option
+    e = assert_raise(ArgumentError) do
+      draw do
+        resources :storage_files, :controller => 'Admin::StorageFiles'
+      end
+    end
+
+    assert_match "'Admin::StorageFiles' is not a supported controller name", e.message
+  end
 end
 
 class TestDefaultScope < ActionDispatch::IntegrationTest
diff --git a/guides/source/routing.md b/guides/source/routing.md
index 14f23d4020..4614169653 100644
--- a/guides/source/routing.md
+++ b/guides/source/routing.md
@@ -832,6 +832,19 @@ will recognize incoming paths beginning with `/photos` but route to the `Images`
 
 NOTE: Use `photos_path`, `new_photo_path`, etc. to generate paths for this resource.
 
+For namespaced controllers you can use the directory notation. For example:
+
+```ruby
+resources :user_permissions, controller: 'admin/user_permissions'
+```
+
+This will route to the `Admin::UserPermissions` controller.
+
+NOTE: Only the directory notation is supported. specifying the
+controller with ruby constant notation (eg. `:controller =>
+'Admin::UserPermissions'`) can lead to routing problems and results in
+a warning.
+
 ### Specifying Constraints
 
 You can use the `:constraints` option to specify a required format on the implicit `id`. For example: