Deprecate router generation "best match" sorting

5 files changed, 39 insertions, 13 deletions

diff --git a/actionpack/lib/action_controller/routing/resources.rb b/actionpack/lib/action_controller/routing/resources.rb
index 7fd3ffd3f1..4862cf7115 100644
--- a/actionpack/lib/action_controller/routing/resources.rb
+++ b/actionpack/lib/action_controller/routing/resources.rb
@@ -529,13 +529,13 @@ module ActionController
         resource = Resource.new(entities, options)
 
         with_options :controller => resource.controller do |map|
+          map_associations(resource, options)
+
           map_collection_actions(map, resource)
           map_default_collection_actions(map, resource)
           map_new_actions(map, resource)
           map_member_actions(map, resource)
 
-          map_associations(resource, options)
-
           if block_given?
             with_options(options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix), &block)
           end
diff --git a/actionpack/lib/action_controller/routing/route_set.rb b/actionpack/lib/action_controller/routing/route_set.rb
index 0c499f3b46..09f6024d39 100644
--- a/actionpack/lib/action_controller/routing/route_set.rb
+++ b/actionpack/lib/action_controller/routing/route_set.rb
@@ -407,9 +407,24 @@ module ActionController
           # don't use the recalled keys when determining which routes to check
           routes = routes_by_controller[controller][action][options.reject {|k,v| !v}.keys.sort_by { |x| x.object_id }]
 
-          routes.each do |route|
+          routes[1].each_with_index do |route, index|
             results = route.__send__(method, options, merged, expire_on)
-            return results if results && (!results.is_a?(Array) || results.first)
+            if results && (!results.is_a?(Array) || results.first)
+
+              # Compare results with Rails 3.0 behavior
+              if routes[0][index] != route
+                routes[0].each do |route2|
+                  new_results = route2.__send__(method, options, merged, expire_on)
+                  if new_results && (!new_results.is_a?(Array) || new_results.first)
+                    ActiveSupport::Deprecation.warn "The URL you generated will use the first matching route in routes.rb rather than the \"best\" match. " +
+                      "In Rails 3.0 #{new_results} would of been generated instead of #{results}"
+                    break
+                  end
+                end
+              end
+
+              return results
+            end
           end
         end
 
@@ -448,7 +463,10 @@ module ActionController
         @routes_by_controller ||= Hash.new do |controller_hash, controller|
           controller_hash[controller] = Hash.new do |action_hash, action|
             action_hash[action] = Hash.new do |key_hash, keys|
-              key_hash[keys] = routes_for_controller_and_action_and_keys(controller, action, keys)
+              key_hash[keys] = [
+                routes_for_controller_and_action_and_keys(controller, action, keys),
+                deprecated_routes_for_controller_and_action_and_keys(controller, action, keys)
+              ]
             end
           end
         end
@@ -460,10 +478,16 @@ module ActionController
         merged = options if expire_on[:controller]
         action = merged[:action] || 'index'
 
-        routes_by_controller[controller][action][merged.keys]
+        routes_by_controller[controller][action][merged.keys][1]
       end
 
       def routes_for_controller_and_action_and_keys(controller, action, keys)
+        routes.select do |route|
+          route.matches_controller_and_action? controller, action
+        end
+      end
+
+      def deprecated_routes_for_controller_and_action_and_keys(controller, action, keys)
         selected = routes.select do |route|
           route.matches_controller_and_action? controller, action
         end
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index 68529cc8f7..346fa09414 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -51,7 +51,7 @@ class PageCachingTest < ActionController::TestCase
     ActionController::Routing::Routes.clear!
 
     ActionController::Routing::Routes.draw do |map|
-      map.main '', :controller => 'posts'
+      map.main '', :controller => 'posts', :format => nil
       map.formatted_posts 'posts.:format', :controller => 'posts'
       map.resources :posts
       map.connect ':controller/:action/:id'
diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb
index 5f9ae6292c..2534c232c7 100644
--- a/actionpack/test/controller/routing_test.rb
+++ b/actionpack/test/controller/routing_test.rb
@@ -1610,7 +1610,7 @@ class RouteTest < Test::Unit::TestCase
     end
 end
 
-class RouteSetTest < Test::Unit::TestCase
+class RouteSetTest < ActiveSupport::TestCase
   def set
     @set ||= ROUTING::RouteSet.new
   end
@@ -2191,8 +2191,10 @@ class RouteSetTest < Test::Unit::TestCase
       map.connect "/ws/people", :controller => "people", :action => "index", :ws => true
     end
 
-    url = set.generate(:controller => "people", :action => "index", :ws => true)
-    assert_equal "/ws/people", url
+    assert_deprecated {
+      url = set.generate(:controller => "people", :action => "index", :ws => true)
+      assert_equal "/ws/people", url
+    }
   end
 
   def test_generate_changes_controller_module
diff --git a/actionpack/test/controller/url_rewriter_test.rb b/actionpack/test/controller/url_rewriter_test.rb
index 863f8414c5..0e149cf8ae 100644
--- a/actionpack/test/controller/url_rewriter_test.rb
+++ b/actionpack/test/controller/url_rewriter_test.rb
@@ -304,7 +304,7 @@ class UrlWriterTests < ActionController::TestCase
   def test_named_routes_with_nil_keys
     ActionController::Routing::Routes.clear!
     ActionController::Routing::Routes.draw do |map|
-      map.main '', :controller => 'posts'
+      map.main '', :controller => 'posts', :format => nil
       map.resources :posts
       map.connect ':controller/:action/:id'
     end
@@ -314,9 +314,9 @@ class UrlWriterTests < ActionController::TestCase
 
     controller = kls.new
     params = {:action => :index, :controller => :posts, :format => :xml}
-    assert_equal("http://www.basecamphq.com/posts.xml", controller.send(:url_for, params))    
+    assert_equal("http://www.basecamphq.com/posts.xml", controller.send(:url_for, params))
     params[:format] = nil
-    assert_equal("http://www.basecamphq.com/", controller.send(:url_for, params))    
+    assert_equal("http://www.basecamphq.com/", controller.send(:url_for, params))
   ensure
     ActionController::Routing::Routes.load!
   end