diff options
Diffstat (limited to 'actionpack')
18 files changed, 113 insertions, 63 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/abstract_controller/base.rb b/actionpack/lib/abstract_controller/base.rb index 16dec31938..d4317399ed 100644 --- a/actionpack/lib/abstract_controller/base.rb +++ b/actionpack/lib/abstract_controller/base.rb @@ -76,7 +76,7 @@ module AbstractController end end - # action_methods are cached and there is sometimes need to refresh + # action_methods are cached and there is sometimes a need to refresh # them. ::clear_action_methods! allows you to do that, so next time # you run action_methods, they will be recalculated. def clear_action_methods! diff --git a/actionpack/lib/action_controller/metal/data_streaming.rb b/actionpack/lib/action_controller/metal/data_streaming.rb index 59984a0028..6cd6130032 100644 --- a/actionpack/lib/action_controller/metal/data_streaming.rb +++ b/actionpack/lib/action_controller/metal/data_streaming.rb @@ -25,14 +25,13 @@ module ActionController #:nodoc: # * <tt>:filename</tt> - suggests a filename for the browser to use. # Defaults to <tt>File.basename(path)</tt>. # * <tt>:type</tt> - specifies an HTTP content type. - # You can specify either a string or a symbol for a registered type register with - # <tt>Mime::Type.register</tt>, for example :json - # If omitted, type will be guessed from the file extension specified in <tt>:filename</tt>. - # If no content type is registered for the extension, default type 'application/octet-stream' will be used. + # You can specify either a string or a symbol for a registered type with <tt>Mime::Type.register</tt>, for example :json. + # If omitted, the type will be inferred from the file extension specified in <tt>:filename</tt>. + # If no content type is registered for the extension, the default type 'application/octet-stream' will be used. # * <tt>:disposition</tt> - specifies whether the file will be shown inline or downloaded. # Valid values are 'inline' and 'attachment' (default). # * <tt>:status</tt> - specifies the status code to send with the response. Defaults to 200. - # * <tt>:url_based_filename</tt> - set to +true+ if you want the browser guess the filename from + # * <tt>:url_based_filename</tt> - set to +true+ if you want the browser to guess the filename from # the URL, which is necessary for i18n filenames on certain browsers # (setting <tt>:filename</tt> overrides this option). # @@ -79,14 +78,14 @@ module ActionController #:nodoc: # <tt>render plain: data</tt>, but also allows you to specify whether # the browser should display the response as a file attachment (i.e. in a # download dialog) or as inline data. You may also set the content type, - # the apparent file name, and other things. + # the file name, and other things. # # Options: # * <tt>:filename</tt> - suggests a filename for the browser to use. - # * <tt>:type</tt> - specifies an HTTP content type. Defaults to 'application/octet-stream'. You can specify - # either a string or a symbol for a registered type register with <tt>Mime::Type.register</tt>, for example :json. - # If omitted, type will be guessed from the file extension specified in <tt>:filename</tt>. - # If no content type is registered for the extension, default type 'application/octet-stream' will be used. + # * <tt>:type</tt> - specifies an HTTP content type. Defaults to 'application/octet-stream'. + # You can specify either a string or a symbol for a registered type with <tt>Mime::Type.register</tt>, for example :json. + # If omitted, type will be inferred from the file extension specified in <tt>:filename</tt>. + # If no content type is registered for the extension, the default type 'application/octet-stream' will be used. # * <tt>:disposition</tt> - specifies whether the file will be shown inline or downloaded. # Valid values are 'inline' and 'attachment' (default). # * <tt>:status</tt> - specifies the status code to send with the response. Defaults to 200. diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb index e31d65aac2..ea8e91ce24 100644 --- a/actionpack/lib/action_controller/metal/force_ssl.rb +++ b/actionpack/lib/action_controller/metal/force_ssl.rb @@ -2,17 +2,17 @@ require 'active_support/core_ext/hash/except' require 'active_support/core_ext/hash/slice' module ActionController - # This module provides a method which will redirect browser to use HTTPS + # This module provides a method which will redirect the browser to use HTTPS # protocol. This will ensure that user's sensitive information will be - # transferred safely over the internet. You _should_ always force browser + # transferred safely over the internet. You _should_ always force the browser # to use HTTPS when you're transferring sensitive information such as # user authentication, account information, or credit card information. # # Note that if you are really concerned about your application security, # you might consider using +config.force_ssl+ in your config file instead. # That will ensure all the data transferred via HTTPS protocol and prevent - # user from getting session hijacked when accessing the site under unsecured - # HTTP protocol. + # the user from getting their session hijacked when accessing the site over + # unsecured HTTP protocol. module ForceSSL extend ActiveSupport::Concern include AbstractController::Callbacks diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb index 53527c08b6..4639348509 100644 --- a/actionpack/lib/action_controller/metal/http_authentication.rb +++ b/actionpack/lib/action_controller/metal/http_authentication.rb @@ -310,9 +310,9 @@ module ActionController end # Might want a shorter timeout depending on whether the request - # is a PATCH, PUT, or POST, and if client is browser or web service. + # is a PATCH, PUT, or POST, and if the client is a browser or web service. # Can be much shorter if the Stale directive is implemented. This would - # allow a user to use new nonce without prompting user again for their + # allow a user to use new nonce without prompting the user again for their # username and password. def validate_nonce(secret_key, request, value, seconds_to_timeout=5*60) return false if value.nil? diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb index 885ea3fefd..624a6d5b76 100644 --- a/actionpack/lib/action_controller/metal/instrumentation.rb +++ b/actionpack/lib/action_controller/metal/instrumentation.rb @@ -75,8 +75,8 @@ module ActionController ActiveSupport::Notifications.instrument("halted_callback.action_controller", :filter => filter) end - # A hook which allows you to clean up any time taken into account in - # views wrongly, like database querying time. + # A hook which allows you to clean up any time, wrongly taken into account in + # views, like database querying time. # # def cleanup_view_runtime # super - time_taken_in_something_expensive diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb index fc20e7a421..6055fde4f7 100644 --- a/actionpack/lib/action_controller/metal/live.rb +++ b/actionpack/lib/action_controller/metal/live.rb @@ -3,7 +3,7 @@ require 'delegate' require 'active_support/json' module ActionController - # Mix this module in to your controller, and all actions in that controller + # Mix this module into your controller, and all actions in that controller # will be able to stream data to the client as it's written. # # class MyController < ActionController::Base @@ -20,7 +20,7 @@ module ActionController # end # end # - # There are a few caveats with this use. You *cannot* write headers after the + # There are a few caveats with this module. You *cannot* write headers after the # response has been committed (Response#committed? will return truthy). # Calling +write+ or +close+ on the response stream will cause the response # object to be committed. Make sure all headers are set before calling write diff --git a/actionpack/lib/action_controller/renderer.rb b/actionpack/lib/action_controller/renderer.rb index e4d19e9dba..2775a24e56 100644 --- a/actionpack/lib/action_controller/renderer.rb +++ b/actionpack/lib/action_controller/renderer.rb @@ -45,7 +45,7 @@ module ActionController }.freeze # Create a new renderer instance for a specific controller class. - def self.for(controller, env = {}, defaults = DEFAULTS) + def self.for(controller, env = {}, defaults = DEFAULTS.dup) new(controller, env, defaults) end 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/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index 69ae5a8468..384254b131 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -122,6 +122,7 @@ module ActionDispatch # params: { ref_id: 14 }, # headers: { "X-Test-Header" => "testvalue" } def request_via_redirect(http_method, path, *args) + ActiveSupport::Deprecation.warn('`request_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') process_with_kwargs(http_method, path, *args) follow_redirect! while redirect? @@ -131,35 +132,35 @@ module ActionDispatch # Performs a GET request, following any subsequent redirect. # See +request_via_redirect+ for more information. def get_via_redirect(path, *args) - ActiveSupport::Deprecation.warn('`get_via_redirect` is deprecated and will be removed in Rails 5.1. Please use follow_redirect! manually after the request call for the same behavior.') + ActiveSupport::Deprecation.warn('`get_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') request_via_redirect(:get, path, *args) end # Performs a POST request, following any subsequent redirect. # See +request_via_redirect+ for more information. def post_via_redirect(path, *args) - ActiveSupport::Deprecation.warn('`post_via_redirect` is deprecated and will be removed in Rails 5.1. Please use follow_redirect! manually after the request call for the same behavior.') + ActiveSupport::Deprecation.warn('`post_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') request_via_redirect(:post, path, *args) end # Performs a PATCH request, following any subsequent redirect. # See +request_via_redirect+ for more information. def patch_via_redirect(path, *args) - ActiveSupport::Deprecation.warn('`patch_via_redirect` is deprecated and will be removed in Rails 5.1. Please use follow_redirect! manually after the request call for the same behavior.') + ActiveSupport::Deprecation.warn('`patch_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') request_via_redirect(:patch, path, *args) end # Performs a PUT request, following any subsequent redirect. # See +request_via_redirect+ for more information. def put_via_redirect(path, *args) - ActiveSupport::Deprecation.warn('`put_via_redirect` is deprecated and will be removed in Rails 5.1. Please use follow_redirect! manually after the request call for the same behavior.') + ActiveSupport::Deprecation.warn('`put_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') request_via_redirect(:put, path, *args) end # Performs a DELETE request, following any subsequent redirect. # See +request_via_redirect+ for more information. def delete_via_redirect(path, *args) - ActiveSupport::Deprecation.warn('`delete_via_redirect` is deprecated and will be removed in Rails 5.1. Please use follow_redirect! manually after the request call for the same behavior.') + ActiveSupport::Deprecation.warn('`delete_via_redirect` is deprecated and will be removed in Rails 5.1. Please use `follow_redirect!` manually after the request call for the same behavior.') request_via_redirect(:delete, path, *args) end end diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index ad7166bafa..97571c1308 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -35,7 +35,7 @@ class SessionTest < ActiveSupport::TestCase path = "/somepath"; args = {:id => '1'}; headers = {"X-Test-Header" => "testvalue"} assert_called_with @session, :process, [:put, path, params: args, headers: headers] do @session.stub :redirect?, false do - @session.request_via_redirect(:put, path, params: args, headers: headers) + assert_deprecated { @session.request_via_redirect(:put, path, params: args, headers: headers) } end end end @@ -54,7 +54,7 @@ class SessionTest < ActiveSupport::TestCase value_series = [true, true, false] assert_called @session, :follow_redirect!, times: 2 do @session.stub :redirect?, ->{ value_series.shift } do - @session.request_via_redirect(:get, path, params: args, headers: headers) + assert_deprecated { @session.request_via_redirect(:get, path, params: args, headers: headers) } end end end @@ -63,7 +63,9 @@ class SessionTest < ActiveSupport::TestCase path = "/somepath"; args = {:id => '1'}; headers = {"X-Test-Header" => "testvalue"} @session.stub :redirect?, false do @session.stub :status, 200 do - assert_equal 200, @session.request_via_redirect(:get, path, params: args, headers: headers) + assert_deprecated do + assert_equal 200, @session.request_via_redirect(:get, path, params: args, headers: headers) + end end end end diff --git a/actionpack/test/controller/renderer_test.rb b/actionpack/test/controller/renderer_test.rb index 16d24fa82a..372c09bc23 100644 --- a/actionpack/test/controller/renderer_test.rb +++ b/actionpack/test/controller/renderer_test.rb @@ -87,6 +87,14 @@ class RendererTest < ActiveSupport::TestCase assert_equal "<p>1\n<br />2</p>", render[inline: '<%= simple_format "1\n2" %>'] end + test 'rendering with user specified defaults' do + ApplicationController.renderer.defaults.merge!({ hello: 'hello', https: true }) + renderer = ApplicationController.renderer.new + content = renderer.render inline: '<%= request.ssl? %>' + + assert_equal 'true', content + end + private def render @render ||= ApplicationController.renderer.method(:render) 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 |