Skip to content
This repository
Browse code

Deprecate router generation "best match" sorting

  • Loading branch information...
commit 734e903af5913342c65d4c294e45f9095fa89986 1 parent 82dd725
Joshua Peek josh authored
4 actionpack/lib/action_controller/routing/resources.rb
@@ -529,13 +529,13 @@ def map_resource(entities, options = {}, &block)
529 529 resource = Resource.new(entities, options)
530 530
531 531 with_options :controller => resource.controller do |map|
  532 + map_associations(resource, options)
  533 +
532 534 map_collection_actions(map, resource)
533 535 map_default_collection_actions(map, resource)
534 536 map_new_actions(map, resource)
535 537 map_member_actions(map, resource)
536 538
537   - map_associations(resource, options)
538   -
539 539 if block_given?
540 540 with_options(options.slice(*INHERITABLE_OPTIONS).merge(:path_prefix => resource.nesting_path_prefix, :name_prefix => resource.nesting_name_prefix), &block)
541 541 end
32 actionpack/lib/action_controller/routing/route_set.rb
@@ -407,9 +407,24 @@ def generate(options, recall = {}, method=:generate)
407 407 # don't use the recalled keys when determining which routes to check
408 408 routes = routes_by_controller[controller][action][options.reject {|k,v| !v}.keys.sort_by { |x| x.object_id }]
409 409
410   - routes.each do |route|
  410 + routes[1].each_with_index do |route, index|
411 411 results = route.__send__(method, options, merged, expire_on)
412   - return results if results && (!results.is_a?(Array) || results.first)
  412 + if results && (!results.is_a?(Array) || results.first)
  413 +
  414 + # Compare results with Rails 3.0 behavior
  415 + if routes[0][index] != route
  416 + routes[0].each do |route2|
  417 + new_results = route2.__send__(method, options, merged, expire_on)
  418 + if new_results && (!new_results.is_a?(Array) || new_results.first)
  419 + ActiveSupport::Deprecation.warn "The URL you generated will use the first matching route in routes.rb rather than the \"best\" match. " +
  420 + "In Rails 3.0 #{new_results} would of been generated instead of #{results}"
  421 + break
  422 + end
  423 + end
  424 + end
  425 +
  426 + return results
  427 + end
413 428 end
414 429 end
415 430
@@ -448,7 +463,10 @@ def routes_by_controller
448 463 @routes_by_controller ||= Hash.new do |controller_hash, controller|
449 464 controller_hash[controller] = Hash.new do |action_hash, action|
450 465 action_hash[action] = Hash.new do |key_hash, keys|
451   - key_hash[keys] = routes_for_controller_and_action_and_keys(controller, action, keys)
  466 + key_hash[keys] = [
  467 + routes_for_controller_and_action_and_keys(controller, action, keys),
  468 + deprecated_routes_for_controller_and_action_and_keys(controller, action, keys)
  469 + ]
452 470 end
453 471 end
454 472 end
@@ -460,10 +478,16 @@ def routes_for(options, merged, expire_on)
460 478 merged = options if expire_on[:controller]
461 479 action = merged[:action] || 'index'
462 480
463   - routes_by_controller[controller][action][merged.keys]
  481 + routes_by_controller[controller][action][merged.keys][1]
464 482 end
465 483
466 484 def routes_for_controller_and_action_and_keys(controller, action, keys)
  485 + routes.select do |route|
  486 + route.matches_controller_and_action? controller, action
  487 + end
  488 + end
  489 +
  490 + def deprecated_routes_for_controller_and_action_and_keys(controller, action, keys)
467 491 selected = routes.select do |route|
468 492 route.matches_controller_and_action? controller, action
469 493 end
2  actionpack/test/controller/caching_test.rb
@@ -51,7 +51,7 @@ def setup
51 51 ActionController::Routing::Routes.clear!
52 52
53 53 ActionController::Routing::Routes.draw do |map|
54   - map.main '', :controller => 'posts'
  54 + map.main '', :controller => 'posts', :format => nil
55 55 map.formatted_posts 'posts.:format', :controller => 'posts'
56 56 map.resources :posts
57 57 map.connect ':controller/:action/:id'
8 actionpack/test/controller/routing_test.rb
@@ -1610,7 +1610,7 @@ def order_query_string(qs)
1610 1610 end
1611 1611 end
1612 1612
1613   -class RouteSetTest < Test::Unit::TestCase
  1613 +class RouteSetTest < ActiveSupport::TestCase
1614 1614 def set
1615 1615 @set ||= ROUTING::RouteSet.new
1616 1616 end
@@ -2191,8 +2191,10 @@ def test_generate_finds_best_fit
2191 2191 map.connect "/ws/people", :controller => "people", :action => "index", :ws => true
2192 2192 end
2193 2193
2194   - url = set.generate(:controller => "people", :action => "index", :ws => true)
2195   - assert_equal "/ws/people", url
  2194 + assert_deprecated {
  2195 + url = set.generate(:controller => "people", :action => "index", :ws => true)
  2196 + assert_equal "/ws/people", url
  2197 + }
2196 2198 end
2197 2199
2198 2200 def test_generate_changes_controller_module
6 actionpack/test/controller/url_rewriter_test.rb
@@ -304,7 +304,7 @@ def test_path_generation_for_symbol_parameter_keys
304 304 def test_named_routes_with_nil_keys
305 305 ActionController::Routing::Routes.clear!
306 306 ActionController::Routing::Routes.draw do |map|
307   - map.main '', :controller => 'posts'
  307 + map.main '', :controller => 'posts', :format => nil
308 308 map.resources :posts
309 309 map.connect ':controller/:action/:id'
310 310 end
@@ -314,9 +314,9 @@ def test_named_routes_with_nil_keys
314 314
315 315 controller = kls.new
316 316 params = {:action => :index, :controller => :posts, :format => :xml}
317   - assert_equal("http://www.basecamphq.com/posts.xml", controller.send(:url_for, params))
  317 + assert_equal("http://www.basecamphq.com/posts.xml", controller.send(:url_for, params))
318 318 params[:format] = nil
319   - assert_equal("http://www.basecamphq.com/", controller.send(:url_for, params))
  319 + assert_equal("http://www.basecamphq.com/", controller.send(:url_for, params))
320 320 ensure
321 321 ActionController::Routing::Routes.load!
322 322 end

0 comments on commit 734e903

Please sign in to comment.
Something went wrong with that request. Please try again.