Skip to content

Commit

Permalink
Deprecate the only_path option on *_path helpers.
Browse files Browse the repository at this point in the history 
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]
  • Loading branch information
@chancancode
chancancode committed Oct 28, 2014
1 parent 8607577 commit aa1fadd
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 2 deletions.
10 changes: 10 additions & 0 deletions actionpack/CHANGELOG.md
View file
@@ -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. * Improve Journey compliance to RFC 3986.


The scanner in Journey failed to recognize routes that use literals The scanner in Journey failed to recognize routes that use literals
Expand Down
28 changes: 26 additions & 2 deletions actionpack/lib/action_dispatch/routing/route_set.rb
View file
Expand Up @@ -134,8 +134,8 @@ def add(name, route)
@url_helpers_module.send :undef_method, url_name @url_helpers_module.send :undef_method, url_name
end end
routes[key] = route routes[key] = route
define_url_helper @path_helpers_module, route, path_name, route.defaults, name, PATH 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, FULL define_url_helper @url_helpers_module, route, url_name, route.defaults, name, UNKNOWN


@path_helpers << path_name @path_helpers << path_name
@url_helpers << url_name @url_helpers << url_name
Expand Down Expand Up @@ -322,6 +322,30 @@ def define_url_helper(mod, route, name, opts, route_key, url_strategy)
PATH = ->(options) { ActionDispatch::Http::URL.path_for(options) } PATH = ->(options) { ActionDispatch::Http::URL.path_for(options) }
FULL = ->(options) { ActionDispatch::Http::URL.full_url_for(options) } FULL = ->(options) { ActionDispatch::Http::URL.full_url_for(options) }
UNKNOWN = ->(options) { ActionDispatch::Http::URL.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 " \

This comment has been minimized.

Sign in to view

Copy link
@carlosantoniodasilva

carlosantoniodasilva Oct 28, 2014

Member

I think you meant explicitly (both branches)

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 28, 2014

Member

Yeah, and regarding style I don't find this to be very idiomatic, I would personally prefer a heredoc with a strip_heredoc call as we do elsewhere.

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Oct 28, 2014

Author Member

Typo, will fix 👍

@fxn regarding the deprecation messages, I think that was discussed somewhere recently. I'll try to find it. The main reason for using this style is that heredoc encodes the line wrapping as part of the string, which results in some awkward output if they user's terminal is not exactly 80 characters wide. Since the deprecation messages don't have a long life span we figured we would just put up with this (and be careful not to miss a \ on one of those lines. Is there a better alternative?

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 28, 2014

Member

@chancancode yeah, the heredoc encodes the newlines, and strip_heredoc was thought to be able to write indented heredocs without the indentation in the final string.

There's then a rule to keep deprecation messages in one single line?

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Oct 28, 2014

Author Member

I think it was 8c52480 and some campfire discussion.. should have looped you in 😄

I guess it's more of a recent "trend", because our deprecation messages got more detailed over time, often with code snippets that suggest migration paths to the user (which is great, I think)... so it becomes more desirable to have control over when a newline is inserted and when it's not (e.g.

rails/activerecord/lib/active_record/reflection.rb

Lines 795 to 800 in 777142d

"Ambiguous source reflection for through association. Please " \
"specify a :source directive on your declaration like:\n" \
"\n" \
" class #{active_record.name} < ActiveRecord::Base\n" \
" #{macro} :#{name}, #{example_options}\n" \
" end"
), and the console width problem becomes more prominent with these long messages.

If that doesn't make sense we can definitely revisit this though!

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Oct 28, 2014

Author Member

Another example where the indentation and line breaks are important:

rails/activerecord/lib/active_record/transactions.rb

Lines 6 to 14 in 8c52480

CALLBACK_WARN_MESSAGE = "Currently, Active Record suppresses errors raised " \
"within `after_rollback`/`after_commit` callbacks and only print them to " \
"the logs. In the next version, these errors will no longer be suppressed. " \
"Instead, the errors will propagate normally just like in other Active " \
"Record callbacks.\n" \
"\n" \
"You can opt into the new behavior and remove this warning by setting:\n" \
"\n" \
" config.active_record.raise_in_transactional_callbacks = true"

It looks pretty gross, but fortunately we can remove it as soon as we branch.

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 28, 2014

Member

All those literals with quotes and "\n"s and backslashes are a smell to me. Let's dig a little bit into it to see what can be done.

There are two different topics:

  1. Warn messages should be in one line no matter how long they are?
  2. Given an answer, which code style could we use that keeps the code clean?

In general, I would understand to have one single line if the purpose is to be log-friendly. Even if the line is as long as some we have (for example this one). That's a trade-off, it is not very readable, but it takes one line in the log.

If that was the consensus, we could use squish. See,

ActiveSupport::Deprecation.warn \
  "The association scope '#{name}' is instance dependent (the scope " \
  "block takes an argument). Preloading happens before the individual " \
  "instances are created. This means that there is no instance being " \
  "passed to the association scope. This will most likely result in " \
  "broken or incorrect behavior. Joining, Preloading and eager loading " \
  "of these associations is deprecated and will be removed in the future."

compared to

ActiveSupport::Deprecation.warn(<<-WARNING.squish)
  The association scope '#{name}' is instance dependent (the scope
  block takes an argument). Preloading happens before the individual
  instances are created. This means that there is no instance being
  passed to the association scope. This will most likely result in
  broken or incorrect behavior. Joining, Preloading and eager loading
  of these associations is deprecated and will be removed in the future.
WARNING

If the trade-off is not necessary, then strip_heredoc preserves relative indentation. That is, it computes the min indent and shifts everything that amount to the left. In particular blank lines and code blocks are preserved (relative to the indented shift).

I would prefer warnings to be optimized for people to read, rather than those long lines, but it's not big deal for me if in general one lines are preferred, only the source code could be prettier.

Indeed warn could enforce squish unless verbatim is true or something, not to push this global guideline to all client code.

If we like any of these ideas I would make a pass and unify warnings in one shot. What do you think?

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Oct 28, 2014

Author Member

@fxn I might have been unclear, but the main motivation to improve (human) readability by allowing the user's console to wrap the lines according to its width, rather than enforcing a 80-character wrap in the code.

In this case, (I believe) the new lines and indentation is supposed to draw attention to the migration path (the config to make the warning go away)... and make that easily copy-and-pastable. Would squish take that away? (Or do we care in the end?)

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 28, 2014

Member

That's a strange optimization to me, if your console width is 50 columns maybe the line breaks are cumbersome, but with the current long lines if your console is wide (I have one with 171 columns right now over here) then the message is as wide.

It doesn't matter though, if that was discussed I am fine with it. In that case I'd go with squish in warn surely, with a flag that allows you to opt-out.

This comment has been minimized.

Sign in to view

Copy link
@fxn

fxn Oct 28, 2014

Member

Ah, regarding that particular case I didn't answer, yeah that would be a use case to opt-out. I think it makes sense to display that config in a code block.
"path helpers in Rails 5. Use the corresponding `*_url` helper " \
"instead."

FULL.call(options)
end
else
PATH.call(options)
end
}
# :startdoc: # :startdoc:


attr_accessor :formatter, :set, :named_routes, :default_scope, :router attr_accessor :formatter, :set, :named_routes, :default_scope, :router
Expand Down
80 changes: 80 additions & 0 deletions actionpack/test/dispatch/routing/route_set_test.rb
View file
Expand Up @@ -69,6 +69,86 @@ def call(env)
end end
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 test "explicit keys win over implicit keys" do
draw do draw do
resources :foo do resources :foo do
Expand Down

3 comments on commit aa1fadd

@chancancode
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @tenderlove @fxn

@fxn
Copy link
Member

@fxn fxn commented on aa1fadd Oct 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think this API makes sense.

@demonodojo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have problems testing decorators with this deprecation. I'm not in a controller, so I get the deprecation warning. How can I mock this to avoid deprecation warning?

Please sign in to comment.