Fix double slash at start of paths when mounting an engine at the root. #2577

Merged
merged 1 commit into from Sep 6, 2011

Projects

None yet

7 participants

@rails-noob

This seems to fix bug #2131.

@guilleiguaran
Ruby on Rails member

The test case for this is in #2230

@rails-noob do you verify the test is passing?

@rails-noob

Actually it doesn't pass, it's a slightly different bug.

@guilleiguaran
Ruby on Rails member

ok, then we will need a test for this pull request :)

@rails-noob

Reported it in bug #2579.

@spastorino
Ruby on Rails member

@rails-noob can you add a test case for that?

@rails-noob

This should work. Also, I merged pull request #2230 since it shares some test code, but feel free to remove it if you don't want tests that fail.

@rails-noob

Is there anything preventing this from getting merged?

@spastorino spastorino commented on an outdated diff Sep 4, 2011
actionpack/lib/action_dispatch/routing/mapper.rb
@@ -452,7 +452,9 @@ module ActionDispatch
prefix_options = options.slice(*_route.segment_keys)
# we must actually delete prefix segment keys to avoid passing them to next url_for
_route.segment_keys.each { |k| options.delete(k) }
- _routes.url_helpers.send("#{name}_path", prefix_options)
+ mount_path = _routes.url_helpers.send("#{name}_path", prefix_options)
+ # when the mount path is "/", the prefix must be an empty string
@spastorino
spastorino Sep 4, 2011

Remove this comment, is saying exactly what the next line does

@spastorino
Ruby on Rails member

Squash the commits and push -f please

@rails-noob

Is this ok?

@vijaydev vijaydev commented on an outdated diff Sep 6, 2011
actionpack/lib/action_dispatch/routing/mapper.rb
@@ -452,7 +452,8 @@ module ActionDispatch
prefix_options = options.slice(*_route.segment_keys)
# we must actually delete prefix segment keys to avoid passing them to next url_for
_route.segment_keys.each { |k| options.delete(k) }
- _routes.url_helpers.send("#{name}_path", prefix_options)
+ mount_path = _routes.url_helpers.send("#{name}_path", prefix_options)
+ mount_path == "/" ? "" : mount_path
@vijaydev
vijaydev Sep 6, 2011

prefer mount_path = '' if mount_path == '/'

@rails-noob

Does it make sense to modify the mount_path? It's really the prefix that's empty when mount_path == '/'. So I think an if statement is clearest, although it's a bit heavy.

rails-noob Fix bug #2579.
Avoids double slash at start of paths when mounting an engine at the root.
43fbb1e
@rails-noob

I ended up simply using prefix instead of mount_path:

                prefix = _routes.url_helpers.send("#{name}_path", prefix_options)
                prefix = '' if prefix == '/'
                prefix
@spastorino spastorino merged commit ef14a0e into rails:master Sep 6, 2011
@arunagw
Ruby on Rails member

It's causing a test fail. It's new added test http://travis-ci.org/#!/rails/rails/builds/134540/L107

@rails-noob

Yes, I mentioned I merged pull request #2230 and that it would fail, but nobody objected. See pull request #2898 to fix it.

@jonleighton
Ruby on Rails member
Ruby on Rails member

@rails-noob can you fix it please?

@jonleighton jonleighton added a commit that referenced this pull request Sep 8, 2011
@jonleighton jonleighton Remove failing test which was wrongly introduced.
This test was introduced in pull request #2577. The author of the pull
request included the test, which was originally in #2230, at the same
time as fixing a separate (but related) bug. However, the author did not
include a fix for this test, so when #2577 was merged, a failing test
was introduced.

So I am removing the failing test for now. If anyone wants to fix the
bug, please do so and submit a complete pull request with a test and
a fix.
562277b
@jonleighton jonleighton added a commit that referenced this pull request Sep 8, 2011
@jonleighton jonleighton Remove failing test which was wrongly introduced.
This test was introduced in pull request #2577. The author of the pull
request included the test, which was originally in #2230, at the same
time as fixing a separate (but related) bug. However, the author did not
include a fix for this test, so when #2577 was merged, a failing test
was introduced.

So I am removing the failing test for now. If anyone wants to fix the
bug, please do so and submit a complete pull request with a test and
a fix.
04baa4b
@trevor

the patch to mapper worked for me in two different deployments, looking forward to solid test.

thanks @rails-noob

@ttosch ttosch pushed a commit that referenced this pull request Jan 19, 2015
@jonleighton jonleighton Remove failing test which was wrongly introduced.
This test was introduced in pull request #2577. The author of the pull
request included the test, which was originally in #2230, at the same
time as fixing a separate (but related) bug. However, the author did not
include a fix for this test, so when #2577 was merged, a failing test
was introduced.

So I am removing the failing test for now. If anyone wants to fix the
bug, please do so and submit a complete pull request with a test and
a fix.
28661a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment