rails/rails — pull request #11858 by pithyless (2013-08-12 19:52:42+00:00)
Title: Refactor Url generation into ActionDispatch::Url

Inspired by @gilesbowkett, I took a shot at DRYing up `#url_for` in Rails core a little.

As a first step, I've pulled out the logic related to generating a URL string into a separate class and documented all the edge cases handled in code with tests.

In a separate commit I've shown how `ActionDispatch::Http::URL#url_for` can be refactored to use this new helper class. No integration tests were removed in this PR (to demonstrate that this not break current functionality), but eventually some of the URL generating integration tests could be removed by ensuring the unit tests cover all the edge cases.

The idea behind this PR is not to change the `url_for` API in any way, but only to simplify the internal implementation and document all the edge cases. With a first-class citizen Url object, I think we could move on to reducing the number of things that have to know how to generate URLs.

Outstanding issues:

1. Will this kind of refactoring even be considered for rails-core? :)

2. Is `ActionDispatch::Url` a good name for this class? I did not want to conflict with the existing `ActionDispatch::Http::URL`.

3. There is one test that is currently not passing in master:

```ruby
# actionpack/test/dispatch/request_test.rb:17
assert_equal 'http://www.example.com/books/?spareslashes=////',
url_for(trailing_slash: true, path: '/books?spareslashes=////')
```

Currently, my patch changes this to:
```ruby
assert_equal 'http://www.example.com/books/?spareslashes=%2F%2F%2F%2F',
url_for(trailing_slash: true, path: '/books?spareslashes=////')
```

I believe this is more correct behavior, because if the same param was passed into options[:params], the slashes would indeed be escaped via #to_query:

```ruby
# actionpack/test/url_test.rb
assert_equal '?spareslashes=%2F%2F%2F%2F', url(host: 'www.example.com',
path: '/books?spareslashes=////').params_string
assert_equal '?spareslashes=%2F%2F%2F%2F', url(host: 'www.example.com',
params: { spareslashes: '////' }).params_string
```

If there is a strong push to not escape params that are passed in via options[:path], I can change it. After all, the main purpose of this PR is only to fully document the edge cases and encapsulate it in a reusable class.