From aa1fadd48fb40dd9396a383696134a259aa59db9 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Tue, 28 Oct 2014 09:43:33 -0700 Subject: [PATCH] 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. See also #17363. [Dan Olson, Godfrey Chan] --- actionpack/CHANGELOG.md | 10 +++ .../lib/action_dispatch/routing/route_set.rb | 28 ++++++- .../test/dispatch/routing/route_set_test.rb | 80 +++++++++++++++++++ 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 4626c2650abcd..158b22c0cce82 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_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index f51bee3b31f57..8d4a3c5670bda 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -134,8 +134,8 @@ def add(name, route) @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 +322,30 @@ def define_url_helper(mod, route, name, opts, route_key, url_strategy) 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 \ + "You are calling a `*_path` helper with the `only_path` option " \ + "expicitly 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." + + PATH.call(options) + else + ActiveSupport::Deprecation.warn \ + "You are calling a `*_path` helper with the `only_path` option " \ + "expicitly set to `false`. This option will stop working on " \ + "path helpers in Rails 5. Use the corresponding `*_url` helper " \ + "instead." + + FULL.call(options) + end + else + PATH.call(options) + end + } # :startdoc: attr_accessor :formatter, :set, :named_routes, :default_scope, :router diff --git a/actionpack/test/dispatch/routing/route_set_test.rb b/actionpack/test/dispatch/routing/route_set_test.rb index c465d56bde5be..a7acc0de41242 100644 --- a/actionpack/test/dispatch/routing/route_set_test.rb +++ b/actionpack/test/dispatch/routing/route_set_test.rb @@ -69,6 +69,86 @@ def call(env) 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