diff options
| -rw-r--r-- | actionpack/CHANGELOG.md | 7 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/journey/formatter.rb | 9 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/inspector.rb | 4 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/mapper.rb | 4 | ||||
| -rw-r--r-- | actionpack/lib/action_dispatch/routing/route_set.rb | 14 | ||||
| -rw-r--r-- | actionpack/test/controller/routing_test.rb | 4 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing/inspector_test.rb | 2 | ||||
| -rw-r--r-- | actionpack/test/dispatch/routing_test.rb | 62 |
8 files changed, 73 insertions, 33 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 370e3a1958..5c856f1b36 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,6 +1,11 @@ +* Routing: Refactor `:action` default handling to ensure that path + parameters are not mutated during route generation. + + *Andrew White* + * Add extension synonyms `yml` and `yaml` for MIME type `application/x-yaml`. - *bogdanvlviv* + *bogdanvlviv* * Adds support for including ActionController::Cookies in API controllers. Previously, including the module would raise when trying to define diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 0323360faa..200477b002 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -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] diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb index 5d30a545a2..2459a45827 100644 --- a/actionpack/lib/action_dispatch/routing/inspector.rb +++ b/actionpack/lib/action_dispatch/routing/inspector.rb @@ -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? diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index ffd5b83ad3..faa93ecc17 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -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 diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 16237bd564..c65dafe9e6 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -548,12 +548,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 @@ -572,11 +570,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 @@ -630,13 +623,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 diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index c477b4156c..168677829a 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -2064,11 +2064,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) diff --git a/actionpack/test/dispatch/routing/inspector_test.rb b/actionpack/test/dispatch/routing/inspector_test.rb index 9d0d23d6de..5aafcb23c2 100644 --- a/actionpack/test/dispatch/routing/inspector_test.rb +++ b/actionpack/test/dispatch/routing/inspector_test.rb @@ -347,7 +347,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 diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 09830c0c46..ade4b0c381 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3991,16 +3991,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 @@ -4012,7 +4002,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 @@ -4755,3 +4756,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 |