Finally fix the bug where symbols and strings were not having the sam…

…e behavior in the router.

If you were using symbols before for methods like match/get/post/put/delete, it is likely that this commit will break your routes.
Everything should behave the same if you are using strings, if not, please open up a ticket.

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -44,7 +44,8 @@ class Mapping #:nodoc:
         IGNORE_OPTIONS = [:to, :as, :via, :on, :constraints, :defaults, :only, :except, :anchor, :shallow, :shallow_path, :shallow_prefix]
 
         def initialize(set, scope, path, options)
-          @set, @scope, @options = set, scope, options
+          @set, @scope = set, scope
+          @options = (@scope[:options] || {}).merge(options)
           @path = normalize_path(path)
           normalize_options!
         end
@@ -57,18 +58,11 @@ def to_route
 
           def normalize_options!
             path_without_format = @path.sub(/\(\.:format\)$/, '')
-            @options = (@scope[:options] || {}).merge(@options)
-
-            if @scope[:as] && !@options[:as].blank?
-              @options[:as] = "#{@scope[:as]}_#{@options[:as]}"
-            elsif @scope[:as] && @options[:as] == ""
-              @options[:as] = @scope[:as].to_s
-            end
 
             if using_match_shorthand?(path_without_format, @options)
               to_shorthand    = @options[:to].blank?
               @options[:to] ||= path_without_format[1..-1].sub(%r{/([^/]*)$}, '#\1')
-              @options[:as] ||= path_without_format[1..-1].gsub("/", "_")
+              @options[:as] ||= Mapper.normalize_name(path_without_format)
             end
 
             @options.merge!(default_controller_and_action(to_shorthand))
@@ -80,8 +74,8 @@ def using_match_shorthand?(path, options)
           end
 
           def normalize_path(path)
-            raise ArgumentError, "path is required" if @scope[:path].blank? && path.blank?
-            path = Mapper.normalize_path("#{@scope[:path]}/#{path}")
+            raise ArgumentError, "path is required" if path.blank?
+            path = Mapper.normalize_path(path)
 
             if path.match(':controller')
               raise ArgumentError, ":controller segment is not allowed within a namespace block" if @scope[:module]
@@ -93,7 +87,11 @@ def normalize_path(path)
               @options.reverse_merge!(:controller => /.+?/)
             end
 
-            path
+            if path.include?(":format")
+              path
+            else
+              "#{path}(.:format)"
+            end
           end
 
           def app
@@ -216,6 +214,10 @@ def self.normalize_path(path)
         path
       end
 
+      def self.normalize_name(name)
+        normalize_path(name)[1..-1].gsub("/", "_")
+      end
+
       module Base
         def initialize(set) #:nodoc:
           @set = set
@@ -324,13 +326,7 @@ def scope(*args)
             ActiveSupport::Deprecation.warn ":name_prefix was deprecated in the new router syntax. Use :as instead.", caller
           end
 
-          case args.first
-          when String
-            options[:path] = args.first
-          when Symbol
-            options[:controller] = args.first
-          end
-
+          options[:path] = args.first if args.first.is_a?(String)
           recover = {}
 
           options[:constraints] ||= {}
@@ -362,8 +358,9 @@ def scope(*args)
           @scope[:blocks]  = recover[:block]
         end
 
-        def controller(controller)
-          scope(controller.to_sym) { yield }
+        def controller(controller, options={})
+          options[:controller] = controller
+          scope(options) { yield }
         end
 
         def namespace(path, options = {})
@@ -444,9 +441,9 @@ def override_keys(child)
       module Resources
         # CANONICAL_ACTIONS holds all actions that does not need a prefix or
         # a path appended since they fit properly in their scope level.
-        VALID_ON_OPTIONS = [:new, :collection, :member]
-        CANONICAL_ACTIONS = [:index, :create, :new, :show, :update, :destroy]
-        RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except]
+        VALID_ON_OPTIONS  = [:new, :collection, :member]
+        RESOURCE_OPTIONS  = [:as, :controller, :path, :only, :except]
+        CANONICAL_ACTIONS = %w(index create new show update destroy)
 
         class Resource #:nodoc:
           DEFAULT_ACTIONS = [:index, :create, :new, :show, :update, :destroy, :edit]
@@ -700,41 +697,27 @@ def match(*args)
             return member { match(*args) }
           end
 
-          path = options.delete(:path)
+          if resource_scope?
+            raise ArgumentError, "can't define route directly in resource(s) scope"
+          end
+
           action = args.first
+          path = path_for_action(action, options.delete(:path))
 
-          if action.is_a?(Symbol) || (resource_method_scope? && action.to_s =~ /^[A-Za-z_]\w*$/)
-            path = path_for_action(action, path)
-            options[:action] ||= action
+          if action.to_s =~ /^[\w\/]+$/
+            options[:action] ||= action unless action.to_s.include?("/")
             options[:as] = name_for_action(action, options[:as])
-
-            with_exclusive_scope do
-              return super(path, options)
-            end
-          elsif resource_method_scope?
-            path = path_for_custom_action
-            options[:as] = name_for_action(options[:as]) if options[:as]
-            args.push(options)
-
-            with_exclusive_scope do
-              scope(path) do
-                return super
-              end
-            end
-          end
-
-          if resource_scope?
-            raise ArgumentError, "can't define route directly in resource(s) scope"
+          else
+            options[:as] = name_for_action(options[:as])
           end
 
-          args.push(options)
-          super
+          super(path, options)
         end
 
         def root(options={})
           if @scope[:scope_level] == :resources
-            with_scope_level(:nested) do
-              scope(parent_resource.path, :as => parent_resource.collection_name) do
+            with_scope_level(:root) do
+              scope(parent_resource.path) do
                 super(options)
               end
             end
@@ -871,7 +854,7 @@ def id_constraint
           end
 
           def canonical_action?(action, flag)
-            flag && CANONICAL_ACTIONS.include?(action)
+            flag && resource_method_scope? && CANONICAL_ACTIONS.include?(action.to_s)
           end
 
           def shallow_scoping?
@@ -882,18 +865,10 @@ def path_for_action(action, path)
             prefix = shallow_scoping? ?
               "#{@scope[:shallow_path]}/#{parent_resource.path}/:id" : @scope[:path]
 
-            if canonical_action?(action, path.blank?)
-              "#{prefix}(.:format)"
-            else
-              "#{prefix}/#{action_path(action, path)}(.:format)"
-            end
-          end
-
-          def path_for_custom_action
-            if shallow_scoping?
-              "#{@scope[:shallow_path]}/#{parent_resource.path}/:id"
+            path = if canonical_action?(action, path.blank?)
+              prefix.to_s
             else
-              @scope[:path]
+              "#{prefix}/#{action_path(action, path)}"
             end
           end
 
@@ -913,6 +888,7 @@ def prefix_name_for_action(action, as)
 
           def name_for_action(action, as=nil)
             prefix = prefix_name_for_action(action, as)
+            prefix = Mapper.normalize_name(prefix) if prefix
             name_prefix = @scope[:as]
 
             if parent_resource
@@ -922,15 +898,18 @@ def name_for_action(action, as=nil)
 
             name = case @scope[:scope_level]
             when :collection
-              [name_prefix, collection_name]
+              [prefix, name_prefix, collection_name]
             when :new
-              [:new, name_prefix, member_name]
+              [prefix, :new, name_prefix, member_name]
+            when :member
+              [prefix, shallow_scoping? ? @scope[:shallow_prefix] : name_prefix, member_name]
+            when :root
+              [name_prefix, collection_name, prefix]
             else
-              [shallow_scoping? ? @scope[:shallow_prefix] : name_prefix, member_name]
+              [name_prefix, member_name, prefix]
             end
 
-            name.unshift(prefix)
-            name.select(&:present?).join("_")
+            name.select(&:present?).join("_").presence
           end
       end
 

actionpack/test/dispatch/routing_test.rb

@@ -40,6 +40,13 @@ def self.matches?(request)
         get  :new, :path => "build"
         post :create, :path => "create", :as => ""
         put  :update
+        get  :remove, :action => :destroy, :as => :remove
+      end
+
+      scope "pagemark", :controller => "pagemarks", :as => :pagemark do
+        get  "new", :path => "build"
+        post "create", :as => ""
+        put  "update"
         get  "remove", :action => :destroy, :as => :remove
       end
 
@@ -203,11 +210,12 @@ def self.matches?(request)
       end
 
       resources :customers do
-        get "recent" => "customers#recent", :on => :collection
-        get "profile" => "customers#profile", :on => :member
+        get :recent, :on => :collection
+        get "profile", :on => :member
+        get "secret/profile" => "customers#secret", :on => :member
         post "preview" => "customers#preview", :as => :another_preview, :on => :new
         resource :avatar do
-          get "thumbnail(.:format)" => "avatars#thumbnail", :as => :thumbnail, :on => :member
+          get "thumbnail" => "avatars#thumbnail", :as => :thumbnail, :on => :member
         end
         resources :invoices do
           get "outstanding" => "invoices#outstanding", :as => :outstanding, :on => :collection
@@ -648,22 +656,42 @@ def test_bookmarks
     with_test_routes do
       get '/bookmark/build'
       assert_equal 'bookmarks#new', @response.body
-      assert_equal '/bookmark/build', new_bookmark_path
+      assert_equal '/bookmark/build', bookmark_new_path
 
       post '/bookmark/create'
       assert_equal 'bookmarks#create', @response.body
       assert_equal '/bookmark/create', bookmark_path
 
-      put '/bookmark'
+      put '/bookmark/update'
       assert_equal 'bookmarks#update', @response.body
-      assert_equal '/bookmark', update_bookmark_path
+      assert_equal '/bookmark/update', bookmark_update_path
 
       get '/bookmark/remove'
       assert_equal 'bookmarks#destroy', @response.body
       assert_equal '/bookmark/remove', bookmark_remove_path
     end
   end
 
+  def test_pagemarks
+    with_test_routes do
+      get '/pagemark/build'
+      assert_equal 'pagemarks#new', @response.body
+      assert_equal '/pagemark/build', pagemark_new_path
+
+      post '/pagemark/create'
+      assert_equal 'pagemarks#create', @response.body
+      assert_equal '/pagemark/create', pagemark_path
+
+      put '/pagemark/update'
+      assert_equal 'pagemarks#update', @response.body
+      assert_equal '/pagemark/update', pagemark_update_path
+
+      get '/pagemark/remove'
+      assert_equal 'pagemarks#destroy', @response.body
+      assert_equal '/pagemark/remove', pagemark_remove_path
+    end
+  end
+
   def test_admin
     with_test_routes do
       get '/admin', {}, {'REMOTE_ADDR' => '192.168.1.100'}
@@ -1564,6 +1592,7 @@ def test_custom_resource_routes_are_scoped
     with_test_routes do
       assert_equal '/customers/recent', recent_customers_path
       assert_equal '/customers/1/profile', profile_customer_path(:id => '1')
+      assert_equal '/customers/1/secret/profile', secret_profile_customer_path(:id => '1')
       assert_equal '/customers/new/preview', another_preview_new_customer_path
       assert_equal '/customers/1/avatar/thumbnail.jpg', thumbnail_customer_avatar_path(:customer_id => '1', :format => :jpg)
       assert_equal '/customers/1/invoices/outstanding', outstanding_customer_invoices_path(:customer_id => '1')
@@ -1577,6 +1606,9 @@ def test_custom_resource_routes_are_scoped
 
       get '/customers/1/invoices/overdue'
       assert_equal 'invoices#overdue', @response.body
+
+      get '/customers/1/secret/profile'
+      assert_equal 'customers#secret', @response.body
     end
   end