diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index ae663deed8..e9738f6a86 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,5 +1,10 @@ ## Rails 4.0.0 (unreleased) ## +* When building a URL fails, add missing keys provided by Journey. Failed URL + generation now returns a 500 status instead of a 404. + + *Richard Schneeman* + * Deprecate availbility of ActionView::RecordIdentifier in controllers by default. It's view specific and can be easily included in controller manually if someone really needs it. RecordIdentifier will be removed from ActionController::Base diff --git a/actionpack/lib/action_controller/metal/exceptions.rb b/actionpack/lib/action_controller/metal/exceptions.rb index 8fd8f4797c..3c9d0c86a7 100644 --- a/actionpack/lib/action_controller/metal/exceptions.rb +++ b/actionpack/lib/action_controller/metal/exceptions.rb @@ -16,6 +16,9 @@ module ActionController end end + class ActionController::UrlGenerationError < RoutingError #:nodoc: + end + class MethodNotAllowed < ActionControllerError #:nodoc: def initialize(*allowed_methods) super("Only #{allowed_methods.to_sentence(:locale => :en)} requests are allowed.") diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 32d267d1d6..1fe40d8458 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -438,12 +438,11 @@ module ActionDispatch attr_reader :options, :recall, :set, :named_route - def initialize(options, recall, set, extras = false) + def initialize(options, recall, set) @named_route = options.delete(:use_route) @options = options.dup @recall = recall.dup @set = set - @extras = extras normalize_options! normalize_controller_action_id! @@ -526,20 +525,12 @@ module ActionDispatch recall[:action] = options.delete(:action) if options[:action] == 'index' end + # Generates a path from routes, returns [path, params] + # if no path is returned the formatter will raise Journey::Router::RoutingError def generate - path, params = @set.formatter.generate(:path_info, named_route, options, recall, PARAMETERIZE) - - raise_routing_error unless path - - return [path, params.keys] if @extras - - [path, params] - rescue Journey::Router::RoutingError - raise_routing_error - end - - def raise_routing_error - raise ActionController::RoutingError, "No route matches #{options.inspect}" + @set.formatter.generate(:path_info, named_route, options, recall, PARAMETERIZE) + rescue Journey::Router::RoutingError => e + raise ActionController::UrlGenerationError, "No route matches #{options.inspect} #{e.message}" end def different_controller? @@ -564,11 +555,12 @@ module ActionDispatch end def generate_extras(options, recall={}) - generate(options, recall, true) + path, params = generate(options, recall) + return path, params.keys end - def generate(options, recall = {}, extras = false) - Generator.new(options, recall, self, extras).generate + def generate(options, recall = {}) + Generator.new(options, recall, self).generate end RESERVED_OPTIONS = [:host, :protocol, :port, :subdomain, :domain, :tld_length, diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 57ab325683..f0430e516f 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -275,7 +275,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase mount lambda {} => "/foo" end - assert_raises(ActionController::RoutingError) do + assert_raises(ActionController::UrlGenerationError) do url_for(rs, :controller => "omg", :action => "lol") end end @@ -514,7 +514,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase rs.draw do get 'post/:id' => 'post#show', :constraints => { :id => /\d+/ }, :as => 'post' end - assert_raise(ActionController::RoutingError) do + assert_raise(ActionController::UrlGenerationError) do url_for(rs, { :controller => 'post', :action => 'show', :bad_param => "foo", :use_route => "post" }) end end @@ -594,7 +594,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase assert_equal '/post/10', url_for(rs, { :controller => 'post', :action => 'show', :id => 10 }) - assert_raise ActionController::RoutingError do + assert_raise(ActionController::UrlGenerationError) do url_for(rs, { :controller => 'post', :action => 'show' }) end end @@ -760,7 +760,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase get 'foos/:id' => 'foos#show', :as => 'foo_with_requirement', :constraints => { :id => /\d+/ } end - assert_raise(ActionController::RoutingError) do + assert_raise(ActionController::UrlGenerationError) do setup_for_named_route.send(:foo_with_requirement_url, "I am Against the constraints") end end @@ -1051,7 +1051,7 @@ class RouteSetTest < ActiveSupport::TestCase set.draw do get "/people" => "missing#index" end - + assert_raise(ActionController::RoutingError) { set.recognize_path("/people", :method => :get) } @@ -1459,7 +1459,7 @@ class RouteSetTest < ActiveSupport::TestCase url = url_for(set, { :controller => 'pages', :action => 'show', :name => 'david' }) assert_equal "/page/david", url - assert_raise ActionController::RoutingError do + assert_raise(ActionController::UrlGenerationError) do url_for(set, { :controller => 'pages', :action => 'show', :name => 'davidjamis' }) end url = url_for(set, { :controller => 'pages', :action => 'show', :name => 'JAMIS' }) diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 6ff651ad52..d236b14e02 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -37,6 +37,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest raise ActionView::Template::Error.new('template', AbstractController::ActionNotFound.new) when "/bad_request" raise ActionController::BadRequest + when "/missing_keys" + raise ActionController::UrlGenerationError, "No route matches" else raise "puke!" end @@ -113,6 +115,15 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_match(/AbstractController::ActionNotFound/, body) end + test "named urls missing keys raise 500 level error" do + @app = DevelopmentApp + + get "/missing_keys", {}, {'action_dispatch.show_exceptions' => true} + assert_response 500 + + assert_match(/ActionController::UrlGenerationError/, body) + end + test "show the controller name in the diagnostics template when controller name is present" do @app = DevelopmentApp get("/runtime_error", {}, { diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index b029131ad8..856248e2ac 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -1799,7 +1799,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest get '/movies/00001' assert_equal 'Not Found', @response.body - assert_raises(ActionController::RoutingError){ movie_path(:id => '00001') } + assert_raises(ActionController::UrlGenerationError){ movie_path(:id => '00001') } get '/movies/0001/reviews' assert_equal 'reviews#index', @response.body @@ -1807,7 +1807,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest get '/movies/00001/reviews' assert_equal 'Not Found', @response.body - assert_raises(ActionController::RoutingError){ movie_reviews_path(:movie_id => '00001') } + assert_raises(ActionController::UrlGenerationError){ movie_reviews_path(:movie_id => '00001') } get '/movies/0001/reviews/0001' assert_equal 'reviews#show', @response.body @@ -1815,7 +1815,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest get '/movies/00001/reviews/0001' assert_equal 'Not Found', @response.body - assert_raises(ActionController::RoutingError){ movie_path(:movie_id => '00001', :id => '00001') } + assert_raises(ActionController::UrlGenerationError){ movie_path(:movie_id => '00001', :id => '00001') } get '/movies/0001/trailer' assert_equal 'trailers#show', @response.body @@ -1823,7 +1823,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest get '/movies/00001/trailer' assert_equal 'Not Found', @response.body - assert_raises(ActionController::RoutingError){ movie_trailer_path(:movie_id => '00001') } + assert_raises(ActionController::UrlGenerationError){ movie_trailer_path(:movie_id => '00001') } end def test_only_should_be_read_from_scope @@ -2142,7 +2142,7 @@ class TestRoutingMapper < ActionDispatch::IntegrationTest get '/lists/2/todos/1' assert_equal 'Not Found', @response.body - assert_raises(ActionController::RoutingError){ list_todo_path(:list_id => '2', :id => '1') } + assert_raises(ActionController::UrlGenerationError){ list_todo_path(:list_id => '2', :id => '1') } end def test_named_routes_collision_is_avoided_unless_explicitly_given_as @@ -2625,7 +2625,7 @@ class TestNamedRouteUrlHelpers < ActionDispatch::IntegrationTest get "/categories/1" assert_response :success - assert_raises(ActionController::RoutingError) { product_path(nil) } + assert_raises(ActionController::UrlGenerationError) { product_path(nil) } end end