diff options
Diffstat (limited to 'actionpack')
20 files changed, 265 insertions, 49 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 4626c2650a..158b22c0cc 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,13 @@ +* Deprecate the `only_path` option on `*_path` helpers. + + In cases where this option is set to `true`, the option is redundant and can + be safely removed; otherwise, the corresponding `*_url` helper should be + used instead. + + Fixes #17294. + + *Dan Olson*, *Godfrey Chan* + * Improve Journey compliance to RFC 3986. The scanner in Journey failed to recognize routes that use literals diff --git a/actionpack/lib/action_controller/metal/exceptions.rb b/actionpack/lib/action_controller/metal/exceptions.rb index 3844dbf2a6..18e003741d 100644 --- a/actionpack/lib/action_controller/metal/exceptions.rb +++ b/actionpack/lib/action_controller/metal/exceptions.rb @@ -25,7 +25,7 @@ module ActionController end end - class ActionController::UrlGenerationError < RoutingError #:nodoc: + class ActionController::UrlGenerationError < ActionControllerError #:nodoc: end class MethodNotAllowed < ActionControllerError #:nodoc: diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb index c9ef3a3dad..1e13b3761f 100644 --- a/actionpack/lib/action_controller/metal/live.rb +++ b/actionpack/lib/action_controller/metal/live.rb @@ -102,7 +102,7 @@ module ActionController end end - message = json.gsub("\n", "\ndata: ") + message = json.gsub(/\n/, "\ndata: ") @stream.write "data: #{message}\n\n" end end diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 7038f0997f..a5ee1e2159 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -1,5 +1,6 @@ require 'active_support/core_ext/hash/indifferent_access' require 'active_support/core_ext/array/wrap' +require 'active_support/core_ext/string/filters' require 'active_support/deprecation' require 'active_support/rescuable' require 'action_dispatch/http/upload' @@ -114,10 +115,12 @@ module ActionController def self.const_missing(const_name) super unless const_name == :NEVER_UNPERMITTED_PARAMS - ActiveSupport::Deprecation.warn "`ActionController::Parameters::NEVER_UNPERMITTED_PARAMS`"\ - " has been deprecated. Use "\ - "`ActionController::Parameters.always_permitted_parameters` instead." - self.always_permitted_parameters + ActiveSupport::Deprecation.warn(<<-MSG.squish) + `ActionController::Parameters::NEVER_UNPERMITTED_PARAMS` has been deprecated. + Use `ActionController::Parameters.always_permitted_parameters` instead. + MSG + + always_permitted_parameters end # Returns a new instance of <tt>ActionController::Parameters</tt>. diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 20ae48d458..a5cd26a3c1 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -27,7 +27,7 @@ module ActionDispatch def symbolized_path_parameters ActiveSupport::Deprecation.warn( - "`symbolized_path_parameters` is deprecated. Please use `path_parameters`" + '`symbolized_path_parameters` is deprecated. Please use `path_parameters`.' ) path_parameters end diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index a8785ee6bf..2a7bb374a5 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -328,7 +328,7 @@ module ActionDispatch # Extracted into ActionDispatch::Request::Utils.deep_munge, but kept here for backwards compatibility. def deep_munge(hash) ActiveSupport::Deprecation.warn( - "This method has been extracted into ActionDispatch::Request::Utils.deep_munge. Please start using that instead." + 'This method has been extracted into `ActionDispatch::Request::Utils.deep_munge`. Please start using that instead.' ) Utils.deep_munge(hash) diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 99d46af953..33de2f8b5f 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -1,4 +1,5 @@ require 'active_support/core_ext/module/attribute_accessors' +require 'active_support/core_ext/string/filters' require 'active_support/deprecation' require 'action_dispatch/http/filter_redirect' require 'monitor' @@ -288,7 +289,12 @@ module ActionDispatch # :nodoc: # as arrays work, and "flattening" responses, cascading to the rack body! # Not sensible behavior. def to_ary - ActiveSupport::Deprecation.warn 'ActionDispatch::Response#to_ary no longer performs implicit conversion to an Array. Please use response.to_a instead, or a splat like `status, headers, body = *response`' + ActiveSupport::Deprecation.warn(<<-MSG.squish) + `ActionDispatch::Response#to_ary` no longer performs implicit conversion + to an array. Please use `response.to_a` instead, or a splat like `status, + headers, body = *response`. + MSG + to_a end diff --git a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb index 274f6f2f22..ff72592b94 100644 --- a/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb @@ -35,10 +35,23 @@ module ActionDispatch if env['action_dispatch.show_detailed_exceptions'] request = Request.new(env) + traces = traces_from_wrapper(wrapper) + + trace_to_show = 'Application Trace' + if traces[trace_to_show].empty? + trace_to_show = 'Full Trace' + end + + if source_to_show = traces[trace_to_show].first + source_to_show_id = source_to_show[:id] + end + template = ActionView::Base.new([RESCUES_TEMPLATE_PATH], request: request, exception: wrapper.exception, - traces: traces_from_wrapper(wrapper), + traces: traces, + show_source_idx: source_to_show_id, + trace_to_show: trace_to_show, routes_inspector: routes_inspector(exception), source_extract: wrapper.source_extract, line_number: wrapper.line_number, @@ -97,31 +110,28 @@ module ActionDispatch # Augment the exception traces by providing ids for all unique stack frame def traces_from_wrapper(wrapper) application_trace = wrapper.application_trace - framework_trace = wrapper.framework_trace - full_trace = wrapper.full_trace - - if application_trace && framework_trace - id_counter = 0 - - application_trace = application_trace.map do |trace| - prev = id_counter - id_counter += 1 - { id: prev, trace: trace } - end - - framework_trace = framework_trace.map do |trace| - prev = id_counter - id_counter += 1 - { id: prev, trace: trace } + framework_trace = wrapper.framework_trace + full_trace = wrapper.full_trace + + appplication_trace_with_ids = [] + framework_trace_with_ids = [] + full_trace_with_ids = [] + + if full_trace + full_trace.each_with_index do |trace, idx| + id_trace = { + id: idx, + trace: trace + } + appplication_trace_with_ids << id_trace if application_trace.include? trace + framework_trace_with_ids << id_trace if framework_trace.include? trace + full_trace_with_ids << id_trace end - - full_trace = application_trace + framework_trace end - { - "Application Trace" => application_trace, - "Framework Trace" => framework_trace, - "Full Trace" => full_trace + "Application Trace" => appplication_trace_with_ids, + "Framework Trace" => framework_trace_with_ids, + "Full Trace" => full_trace_with_ids } end end diff --git a/actionpack/lib/action_dispatch/middleware/static.rb b/actionpack/lib/action_dispatch/middleware/static.rb index e66c21ef85..002bf8b11a 100644 --- a/actionpack/lib/action_dispatch/middleware/static.rb +++ b/actionpack/lib/action_dispatch/middleware/static.rb @@ -24,9 +24,19 @@ module ActionDispatch path = URI.parser.unescape(path) return false unless path.valid_encoding? - paths = [path, "#{path}#{ext}", "#{path}/index#{ext}"] + paths = [path, "#{path}#{ext}", "#{path}/index#{ext}"].map { |v| + Rack::Utils.clean_path_info v + } - if match = paths.detect {|p| File.file?(File.join(@root, p)) } + if match = paths.detect { |p| + path = File.join(@root, p) + begin + File.file?(path) && File.readable?(path) + rescue SystemCallError + false + end + + } return ::Rack::Utils.escape(match) end end diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb index ba29e26930..eabac3a9d2 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb @@ -1,7 +1,7 @@ <% if @source_extract %> <% @source_extract.each_with_index do |extract_source, index| %> <% if extract_source[:code] %> - <div class="source <%="hidden" if index != 0%>" id="frame-source-<%=index%>"> + <div class="source <%="hidden" if @show_source_idx != index%>" id="frame-source-<%=index%>"> <div class="info"> Extracted source (around line <strong>#<%= extract_source[:line_number] %></strong>): </div> diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb index f62caf51d7..ab57b11c7d 100644 --- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb +++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_trace.html.erb @@ -12,7 +12,7 @@ <% end %> <% @traces.each do |name, trace| %> - <div id="<%= name.gsub(/\s/, '-') %>" style="display: <%= (name == "Application Trace") ? 'block' : 'none' %>;"> + <div id="<%= name.gsub(/\s/, '-') %>" style="display: <%= (name == @trace_to_show) ? 'block' : 'none' %>;"> <pre><code><% trace.each do |frame| %><a class="trace-frames" data-frame-id="<%= frame[:id] %>" href="#"><%= frame[:trace] %></a><br><% end %></code></pre> </div> <% end %> diff --git a/actionpack/lib/action_dispatch/routing/inspector.rb b/actionpack/lib/action_dispatch/routing/inspector.rb index ea3b2f419d..cfe2237512 100644 --- a/actionpack/lib/action_dispatch/routing/inspector.rb +++ b/actionpack/lib/action_dispatch/routing/inspector.rb @@ -48,7 +48,7 @@ module ActionDispatch def reqs @reqs ||= begin reqs = endpoint - reqs += " #{constraints.to_s}" unless constraints.empty? + reqs += " #{constraints}" unless constraints.empty? reqs end end diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index a121fef663..ac03ecb2c8 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -4,6 +4,7 @@ require 'active_support/core_ext/hash/slice' require 'active_support/core_ext/enumerable' require 'active_support/core_ext/array/extract_options' require 'active_support/core_ext/module/remove_method' +require 'active_support/core_ext/string/filters' require 'active_support/inflector' require 'action_dispatch/routing/redirection' require 'action_dispatch/routing/endpoint' @@ -282,11 +283,19 @@ module ActionDispatch def split_to(to) case to when Symbol - ActiveSupport::Deprecation.warn "defining a route where `to` is a symbol is deprecated. Please change \"to: :#{to}\" to \"action: :#{to}\"" + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Defining a route where `to` is a symbol is deprecated. + Please change `to: :#{to}` to `action: :#{to}`. + MSG + [nil, to.to_s] when /#/ then to.split('#') when String - ActiveSupport::Deprecation.warn "defining a route where `to` is a controller without an action is deprecated. Please change \"to: :#{to}\" to \"controller: :#{to}\"" + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Defining a route where `to` is a controller without an action is deprecated. + Please change `to: :#{to}` to `controller: :#{to}`. + MSG + [to, nil] else [] diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index f51bee3b31..a641ea3ea9 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -6,6 +6,7 @@ require 'active_support/core_ext/object/to_query' require 'active_support/core_ext/hash/slice' require 'active_support/core_ext/module/remove_method' require 'active_support/core_ext/array/extract_options' +require 'active_support/core_ext/string/filters' require 'action_controller/metal/exceptions' require 'action_dispatch/http/request' require 'action_dispatch/routing/endpoint' @@ -102,7 +103,10 @@ module ActionDispatch end def helpers - ActiveSupport::Deprecation.warn("`named_routes.helpers` is deprecated, please use `route_defined?(route_name)` to see if a named route was defined.") + ActiveSupport::Deprecation.warn(<<-MSG.squish) + `named_routes.helpers` is deprecated, please use `route_defined?(route_name)` + to see if a named route was defined. + MSG @path_helpers + @url_helpers end @@ -134,8 +138,8 @@ module ActionDispatch @url_helpers_module.send :undef_method, url_name end routes[key] = route - define_url_helper @path_helpers_module, route, path_name, route.defaults, name, PATH - define_url_helper @url_helpers_module, route, url_name, route.defaults, name, FULL + define_url_helper @path_helpers_module, route, path_name, route.defaults, name, LEGACY + define_url_helper @url_helpers_module, route, url_name, route.defaults, name, UNKNOWN @path_helpers << path_name @url_helpers << url_name @@ -322,6 +326,32 @@ module ActionDispatch PATH = ->(options) { ActionDispatch::Http::URL.path_for(options) } FULL = ->(options) { ActionDispatch::Http::URL.full_url_for(options) } UNKNOWN = ->(options) { ActionDispatch::Http::URL.url_for(options) } + LEGACY = ->(options) { + if options.key?(:only_path) + if options[:only_path] + ActiveSupport::Deprecation.warn(<<-MSG.squish) + You are calling a `*_path` helper with the `only_path` option + explicitly set to `true`. This option will stop working on + path helpers in Rails 5. Simply remove the `only_path: true` + argument from your call as it is redundant when applied to a + path helper. + MSG + + PATH.call(options) + else + ActiveSupport::Deprecation.warn(<<-MSG.squish) + You are calling a `*_path` helper with the `only_path` option + explicitly set to `false`. This option will stop working on + path helpers in Rails 5. Use the corresponding `*_url` helper + instead. + MSG + + FULL.call(options) + end + else + PATH.call(options) + end + } # :startdoc: attr_accessor :formatter, :set, :named_routes, :default_scope, :router diff --git a/actionpack/lib/action_dispatch/testing/assertions/tag.rb b/actionpack/lib/action_dispatch/testing/assertions/tag.rb index 5c2905d1ac..da98b1d6ce 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/tag.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/tag.rb @@ -1,3 +1,3 @@ require 'active_support/deprecation' -ActiveSupport::Deprecation.warn("ActionDispatch::Assertions::TagAssertions has been extracted to the rails-dom-testing gem.") +ActiveSupport::Deprecation.warn('`ActionDispatch::Assertions::TagAssertions` has been extracted to the rails-dom-testing gem.') diff --git a/actionpack/lib/action_pack/gem_version.rb b/actionpack/lib/action_pack/gem_version.rb index 6834845d4f..9b3ea30f69 100644 --- a/actionpack/lib/action_pack/gem_version.rb +++ b/actionpack/lib/action_pack/gem_version.rb @@ -8,7 +8,7 @@ module ActionPack MAJOR = 4 MINOR = 2 TINY = 0 - PRE = "beta2" + PRE = "beta4" STRING = [MAJOR, MINOR, TINY, PRE].compact.join(".") end diff --git a/actionpack/test/controller/mime/respond_to_test.rb b/actionpack/test/controller/mime/respond_to_test.rb index 1bc7ad3015..66d2fd7716 100644 --- a/actionpack/test/controller/mime/respond_to_test.rb +++ b/actionpack/test/controller/mime/respond_to_test.rb @@ -579,10 +579,10 @@ class RespondToControllerTest < ActionController::TestCase end get :using_defaults - assert_equal "using_defaults - #{[:html].to_s}", @response.body + assert_equal "using_defaults - #{[:html]}", @response.body get :using_defaults, :format => "xml" - assert_equal "using_defaults - #{[:xml].to_s}", @response.body + assert_equal "using_defaults - #{[:xml]}", @response.body end def test_format_with_custom_response_type diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 0add9fa3b0..f8851f0152 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -19,6 +19,10 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest @closed = true end + def method_that_raises + raise StandardError.new 'error in framework' + end + def call(env) env['action_dispatch.show_detailed_exceptions'] = @detailed req = ActionDispatch::Request.new(env) @@ -57,7 +61,8 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest {}) raise ActionView::Template::Error.new(template, e) end - + when "/framework_raises" + method_that_raises else raise "puke!" end @@ -280,4 +285,39 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest assert_select 'pre code', /\(eval\):1: syntax error, unexpected/ end end + + test 'debug exceptions app shows user code that caused the error in source view' do + @app = DevelopmentApp + Rails.stubs(:root).returns(Pathname.new('.')) + cleaner = ActiveSupport::BacktraceCleaner.new.tap do |bc| + bc.add_silencer { |line| line =~ /method_that_raises/ } + bc.add_silencer { |line| line !~ %r{test/dispatch/debug_exceptions_test.rb} } + end + + get '/framework_raises', {}, {'action_dispatch.backtrace_cleaner' => cleaner} + + # Assert correct error + assert_response 500 + assert_select 'h2', /error in framework/ + + # assert source view line is the call to method_that_raises + assert_select 'div.source:not(.hidden)' do + assert_select 'pre .line.active', /method_that_raises/ + end + + # assert first source view (hidden) that throws the error + assert_select 'div.source:first' do + assert_select 'pre .line.active', /raise StandardError\.new/ + end + + # assert application trace refers to line that calls method_that_raises is first + assert_select '#Application-Trace' do + assert_select 'pre code a:first', %r{test/dispatch/debug_exceptions_test\.rb:\d+:in `call} + end + + # assert framework trace that that threw the error is first + assert_select '#Framework-Trace' do + assert_select 'pre code a:first', /method_that_raises/ + end + end end diff --git a/actionpack/test/dispatch/routing/route_set_test.rb b/actionpack/test/dispatch/routing/route_set_test.rb index c465d56bde..a7acc0de41 100644 --- a/actionpack/test/dispatch/routing/route_set_test.rb +++ b/actionpack/test/dispatch/routing/route_set_test.rb @@ -69,6 +69,86 @@ module ActionDispatch end end + test "only_path: true with *_url and no :host option" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_equal '/foo', url_helpers.foo_url(only_path: true) + end + + test "only_path: false with *_url and no :host option" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_raises ArgumentError do + assert_equal 'http://example.com/foo', url_helpers.foo_url(only_path: false) + end + end + + test "only_path: false with *_url and local :host option" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_equal 'http://example.com/foo', url_helpers.foo_url(only_path: false, host: 'example.com') + end + + test "only_path: false with *_url and global :host option" do + @set.default_url_options = { host: 'example.com' } + + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_equal 'http://example.com/foo', url_helpers.foo_url(only_path: false) + end + + test "only_path: true with *_path" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_deprecated do + assert_equal '/foo', url_helpers.foo_path(only_path: true) + end + end + + test "only_path: false with *_path with global :host option" do + @set.default_url_options = { host: 'example.com' } + + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_deprecated do + assert_equal 'http://example.com/foo', url_helpers.foo_path(only_path: false) + end + end + + test "only_path: false with *_path with local :host option" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_deprecated do + assert_equal 'http://example.com/foo', url_helpers.foo_path(only_path: false, host: 'example.com') + end + end + + test "only_path: false with *_path with no :host option" do + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_deprecated do + assert_raises ArgumentError do + assert_equal 'http://example.com/foo', url_helpers.foo_path(only_path: false) + end + end + end + test "explicit keys win over implicit keys" do draw do resources :foo do diff --git a/actionpack/test/dispatch/static_test.rb b/actionpack/test/dispatch/static_test.rb index 6f7373201c..7f1207eaed 100644 --- a/actionpack/test/dispatch/static_test.rb +++ b/actionpack/test/dispatch/static_test.rb @@ -200,7 +200,8 @@ class StaticTest < ActiveSupport::TestCase } def setup - @app = ActionDispatch::Static.new(DummyApp, "#{FIXTURE_LOAD_PATH}/public", "public, max-age=60") + @root = "#{FIXTURE_LOAD_PATH}/public" + @app = ActionDispatch::Static.new(DummyApp, @root, "public, max-age=60") end def public_path @@ -208,11 +209,28 @@ class StaticTest < ActiveSupport::TestCase end include StaticTests + + def test_custom_handler_called_when_file_is_outside_root + filename = 'shared.html.erb' + assert File.exist?(File.join(@root, '..', filename)) + env = { + "REQUEST_METHOD"=>"GET", + "REQUEST_PATH"=>"/..%2F#{filename}", + "PATH_INFO"=>"/..%2F#{filename}", + "REQUEST_URI"=>"/..%2F#{filename}", + "HTTP_VERSION"=>"HTTP/1.1", + "SERVER_NAME"=>"localhost", + "SERVER_PORT"=>"8080", + "QUERY_STRING"=>"" + } + assert_equal(DummyApp.call(nil), @app.call(env)) + end end class StaticEncodingTest < StaticTest def setup - @app = ActionDispatch::Static.new(DummyApp, "#{FIXTURE_LOAD_PATH}/公共", "public, max-age=60") + @root = "#{FIXTURE_LOAD_PATH}/公共" + @app = ActionDispatch::Static.new(DummyApp, @root, "public, max-age=60") end def public_path |