mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Refactor handling of :action default in routing
The longstanding convention in Rails is that if the :action parameter is missing or nil then it defaults to 'index'. Up until Rails 5.0.0.beta1 this was handled slightly differently than other routing defaults by deleting it from the route options and adding it to the recall parameters. With the recent focus of removing unnecessary duplications this has exposed a problem in this strategy - we are now mutating the request's path parameters and causing problems for later url generation. This will typically affect url_for rather a named url helper since the latter explicitly pass :controller, :action, etc. The fix is to add a default for :action in the route class if the path contains an :action segment and no default is passed. This change also revealed an issue with the parameterized part expiry in that it doesn't follow a right to left order - as soon as a dynamic segment is required then all other segments become required. Fixes #23019.
This commit is contained in:
parent
3dd9fe5db4
commit
8ca8a2d773
8 changed files with 74 additions and 32 deletions
|
@ -1,3 +1,10 @@
|
|||
* Refactor handling of `:action` default in routing so that the request's
|
||||
`path_parameters` is not mutated when generating a route.
|
||||
|
||||
Fixes #23019.
|
||||
|
||||
*Andrew White*
|
||||
|
||||
* Add request encoding and response parsing to integration tests.
|
||||
|
||||
What previously was:
|
||||
|
|
|
@ -32,8 +32,13 @@ module ActionDispatch
|
|||
|
||||
defaults = route.defaults
|
||||
required_parts = route.required_parts
|
||||
parameterized_parts.keep_if do |key, value|
|
||||
(defaults[key].nil? && value.present?) || value.to_s != defaults[key].to_s || required_parts.include?(key)
|
||||
|
||||
route.parts.reverse_each do |key|
|
||||
break if defaults[key].nil? && parameterized_parts[key].present?
|
||||
break if parameterized_parts[key].to_s != defaults[key].to_s
|
||||
break if required_parts.include?(key)
|
||||
|
||||
parameterized_parts.delete(key)
|
||||
end
|
||||
|
||||
return [route.format(parameterized_parts), params]
|
||||
|
|
|
@ -33,11 +33,11 @@ module ActionDispatch
|
|||
end
|
||||
|
||||
def controller
|
||||
requirements[:controller] || ':controller'
|
||||
parts.include?(:controller) ? ':controller' : requirements[:controller]
|
||||
end
|
||||
|
||||
def action
|
||||
requirements[:action] || ':action'
|
||||
parts.include?(:action) ? ':action' : requirements[:action]
|
||||
end
|
||||
|
||||
def internal?
|
||||
|
|
|
@ -137,6 +137,10 @@ module ActionDispatch
|
|||
@conditions = Hash[conditions]
|
||||
@defaults = formats[:defaults].merge(@defaults).merge(normalize_defaults(options))
|
||||
|
||||
if path_params.include?(:action) && !@requirements.key?(:action)
|
||||
@defaults[:action] ||= 'index'
|
||||
end
|
||||
|
||||
@required_defaults = (split_options[:required_defaults] || []).map(&:first)
|
||||
end
|
||||
|
||||
|
|
|
@ -533,12 +533,10 @@ module ActionDispatch
|
|||
@recall = recall
|
||||
@set = set
|
||||
|
||||
normalize_recall!
|
||||
normalize_options!
|
||||
normalize_controller_action_id!
|
||||
use_relative_controller!
|
||||
normalize_controller!
|
||||
normalize_action!
|
||||
end
|
||||
|
||||
def controller
|
||||
|
@ -557,11 +555,6 @@ module ActionDispatch
|
|||
end
|
||||
end
|
||||
|
||||
# Set 'index' as default action for recall
|
||||
def normalize_recall!
|
||||
@recall[:action] ||= 'index'
|
||||
end
|
||||
|
||||
def normalize_options!
|
||||
# If an explicit :controller was given, always make :action explicit
|
||||
# too, so that action expiry works as expected for things like
|
||||
|
@ -615,13 +608,6 @@ module ActionDispatch
|
|||
end
|
||||
end
|
||||
|
||||
# Move 'index' action from options to recall
|
||||
def normalize_action!
|
||||
if @options[:action] == 'index'.freeze
|
||||
@recall[:action] = @options.delete(:action)
|
||||
end
|
||||
end
|
||||
|
||||
# Generates a path from routes, returns [path, params].
|
||||
# If no route is generated the formatter will raise ActionController::UrlGenerationError
|
||||
def generate
|
||||
|
|
|
@ -1964,11 +1964,11 @@ class RackMountIntegrationTests < ActiveSupport::TestCase
|
|||
def test_extras
|
||||
params = {:controller => 'people'}
|
||||
assert_equal [], @routes.extra_keys(params)
|
||||
assert_equal({:controller => 'people'}, params)
|
||||
assert_equal({:controller => 'people', :action => 'index'}, params)
|
||||
|
||||
params = {:controller => 'people', :foo => 'bar'}
|
||||
assert_equal [:foo], @routes.extra_keys(params)
|
||||
assert_equal({:controller => 'people', :foo => 'bar'}, params)
|
||||
assert_equal({:controller => 'people', :action => 'index', :foo => 'bar'}, params)
|
||||
|
||||
params = {:controller => 'people', :action => 'create', :person => { :name => 'Josh'}}
|
||||
assert_equal [:person], @routes.extra_keys(params)
|
||||
|
|
|
@ -339,7 +339,7 @@ module ActionDispatch
|
|||
end
|
||||
|
||||
assert_equal ["Prefix Verb URI Pattern Controller#Action",
|
||||
" GET /:controller(/:action) (?-mix:api\\/[^\\/]+)#:action"], output
|
||||
" GET /:controller(/:action) :controller#:action"], output
|
||||
end
|
||||
|
||||
def test_inspect_routes_shows_resources_route_when_assets_disabled
|
||||
|
|
|
@ -3964,16 +3964,6 @@ class TestUnicodePaths < ActionDispatch::IntegrationTest
|
|||
end
|
||||
|
||||
class TestMultipleNestedController < ActionDispatch::IntegrationTest
|
||||
module ::Foo
|
||||
module Bar
|
||||
class BazController < ActionController::Base
|
||||
def index
|
||||
render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
|
||||
app.draw do
|
||||
namespace :foo do
|
||||
|
@ -3985,7 +3975,18 @@ class TestMultipleNestedController < ActionDispatch::IntegrationTest
|
|||
end
|
||||
end
|
||||
|
||||
include Routes.url_helpers
|
||||
module ::Foo
|
||||
module Bar
|
||||
class BazController < ActionController::Base
|
||||
include Routes.url_helpers
|
||||
|
||||
def index
|
||||
render :inline => "<%= url_for :controller => '/pooh', :action => 'index' %>"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
APP = build_app Routes
|
||||
def app; APP end
|
||||
|
||||
|
@ -4717,3 +4718,42 @@ class TestPartialDynamicPathSegments < ActionDispatch::IntegrationTest
|
|||
assert_equal(params, request.path_parameters)
|
||||
end
|
||||
end
|
||||
|
||||
class TestPathParameters < ActionDispatch::IntegrationTest
|
||||
Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
|
||||
app.draw do
|
||||
scope module: 'test_path_parameters' do
|
||||
scope ':locale', locale: /en|ar/ do
|
||||
root to: 'home#index'
|
||||
get '/about', to: 'pages#about'
|
||||
end
|
||||
end
|
||||
|
||||
get ':controller(/:action/(:id))'
|
||||
end
|
||||
end
|
||||
|
||||
class HomeController < ActionController::Base
|
||||
include Routes.url_helpers
|
||||
|
||||
def index
|
||||
render inline: "<%= root_path %>"
|
||||
end
|
||||
end
|
||||
|
||||
class PagesController < ActionController::Base
|
||||
include Routes.url_helpers
|
||||
|
||||
def about
|
||||
render inline: "<%= root_path(locale: :ar) %> | <%= url_for(locale: :ar) %>"
|
||||
end
|
||||
end
|
||||
|
||||
APP = build_app Routes
|
||||
def app; APP end
|
||||
|
||||
def test_path_parameters_are_not_mutated
|
||||
get '/en/about'
|
||||
assert_equal "/ar | /ar/about", @response.body
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue