Append trailing slash when the path is generated #43287
Conversation
|
@casperisfine there's a small rubocop offense |
| @@ -42,9 +42,6 @@ class RequestUrlFor < BaseRequestTest | |||
|
|
|||
| assert_equal "/books", url_for(only_path: true, path: "/books") | |||
|
|
|||
| assert_equal "http://www.example.com/books/?q=code", url_for(trailing_slash: true, path: "/books?q=code") | |||
There was a problem hiding this comment.
Why were those tests removed?
There was a problem hiding this comment.
Because I believe they test something that's the underlying implementation an not public API #8704
Assuming that url_for(path: "/foo") isn't public API (it isn't documented as far as I can tell).
There was a problem hiding this comment.
But why not delete the test above that as well? I was not sure if it was because the behavior changed and the test failed, or just because we were cleaning up implementation detail tests.
In general we test non public API in the rails test suite if it is necessary, but if it is being tested using the public API it is fine to remove.
There was a problem hiding this comment.
But why not delete the test above that as well? I was not sure if it was because the behavior changed and the test failed
Right. These two failed because they were testing the feature through a private API.
There was a problem hiding this comment.
So there is a change in behavior? I find that concerning. Would this change reintroduce the behavior #8704 was supposed to fix?
There was a problem hiding this comment.
No, I don't think so. But I feel like we're talking past each others, it's my fault I didn't take the time to properly braindump in the PR description, I'll do that.
There was a problem hiding this comment.
@rafaelfranca I added some more context to the PR and also added some more integration style tests.
casperisfine
force-pushed
the
append-slash
branch
from
46aee6a to
0e60951
Compare
Instead of trying to parse the path later to insert the slash or bail out if the path look like it has a format indicator. This patch is based on the assumption that `url_for(path: '/blah')` is not an actual public API.
casperisfine
force-pushed
the
append-slash
branch
from
0e60951 to
f593e97
Compare
| @@ -71,7 +71,8 @@ def path_for(options) | |||
| path = options[:script_name].to_s.chomp("/") | |||
| path << options[:path] if options.key?(:path) | |||
|
|
|||
| add_trailing_slash(path) if options[:trailing_slash] | |||
| path = "/" if options[:trailing_slash] && path.blank? | |||
Ref: rails#43287 Fix: rails#44373 The `OptimizedUrlHelper` wasn't considering this option.
Ref: rails#43287 Fix: rails#44373 The `OptimizedUrlHelper` wasn't considering this option.
Fix: #43219
Instead of trying to parse the path later to insert the slash or bail out if the path look like it has a format indicator.
This patch is based on the assumption that
url_for(path: '/blah')is not an actual public API. Which isn't something I'm certain of. But at the very least it doesn't seem documented.Git History
The
trailing_slashoption was added in db4f421. However it was tacked on as a string operation on the return value of the URL Generator.I don't understand why it wasn't added as an option of the generator itself.
The consequence is that then URL then have to be parsed to know where to insert the slash and wether it is safe or not to do.
#8704 fixed a few corner cases and added the following two tests:
But as far as I can tell they don't correspond to any real behavior. Even if you use
url_forwith the implicit current request arguments as to normalize the current URL, thepath:option is not passed and the URL generated again from:controllerandaction.