Skip to content
This repository has been archived by the owner on Sep 13, 2017. It is now read-only.

Commit

Permalink
Merge pull request #24 from carlosantoniodasilva/script-name-root-url
Browse files Browse the repository at this point in the history
Fix to not allow trailing slash on script name when matching
  • Loading branch information
tenderlove committed Mar 9, 2012
2 parents 805efa6 + 61ddc45 commit b96ea6f
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 10 deletions.
2 changes: 1 addition & 1 deletion lib/journey/router.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def call env
@params_key)

unless route.path.anchored
env['SCRIPT_NAME'] = script_name.to_s + match.to_s
env['SCRIPT_NAME'] = (script_name.to_s + match.to_s).chomp('/')
env['PATH_INFO'] = match.post_match

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Mar 11, 2012

The PATH_INFO environment variable is missing it's leading slash here - e.g. the following assertion will fail in the test below:

assert_equal '/weblog', env['PATH_INFO']

The best solution for 1-0-stable is to just run it through normalize_path and this is essentially what happens for mounted engines, however mounted rack apps won't work, e.g. a get '/weblog' in a Sinatra app will fail. Ideally for master I'd like to have a stab at implementing #17 and eliminating normalize_path - I'll discuss in that ticket what I think the semantics should be.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 12, 2012

Member

Yeah, this assertion will fail hard. I can provide another patch to add normalize_path to fix that while you work on issue #17, if that sounds good. Thanks @pixeltrix.

This comment has been minimized.

Copy link
@tenderlove

tenderlove Mar 12, 2012

Author Member

@carlosantoniodasilva I agree with @pixeltrix. Please send a normalize_path patch. :-)

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 13, 2012

Member

Done :)

This comment has been minimized.

Copy link
@tenderlove

tenderlove Mar 13, 2012

Author Member

@carlosantoniodasilva ❤️❤️❤️❤️❤️❤️❤️

end

Expand Down
30 changes: 21 additions & 9 deletions test/test_router.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ def test_dashes

def test_unicode
router = Router.new(routes, {})

#match the escaped version of /ほげ
exp = Router::Strexp.new '/%E3%81%BB%E3%81%92', {}, ['/.?']
exp = Router::Strexp.new '/%E3%81%BB%E3%81%92', {}, ['/.?']
path = Path::Pattern.new exp

routes.add_route nil, path, {}, {:id => nil}, {}
Expand Down Expand Up @@ -195,6 +195,18 @@ def test_X_Cascade
assert_equal 404, resp.first
end

def test_clear_trailing_slash_from_script_name_on_root_unanchored_routes
strexp = Router::Strexp.new("/", {}, ['/', '.', '?'], false)
path = Path::Pattern.new strexp
app = lambda { |env| [200, {}, ['success!']] }
route = @router.routes.add_route(app, path, {}, {}, {})

env = rack_env('SCRIPT_NAME' => '', 'PATH_INFO' => '/weblog')
resp = @router.call(env)
assert_equal ['success!'], resp.last
assert_equal '', env['SCRIPT_NAME']
end

def test_defaults_merge_correctly
path = Path::Pattern.new '/foo(/:id)'
@router.routes.add_route nil, path, {}, {:id => nil}, {}
Expand Down Expand Up @@ -278,13 +290,6 @@ def test_recall_should_be_used_when_scoring
assert_equal "/messages/index/10", path
end

def add_routes router, paths
paths.each do |path|
path = Path::Pattern.new path
router.routes.add_route nil, path, {}, {}, {}
end
end

def test_nil_path_parts_are_ignored
path = Path::Pattern.new "/:controller(/:action(.:format))"
@router.routes.add_route nil, path, {}, {}, {}
Expand Down Expand Up @@ -499,6 +504,13 @@ def test_recognize_cares_about_verbs

private

def add_routes router, paths
paths.each do |path|
path = Path::Pattern.new path
router.routes.add_route nil, path, {}, {}, {}
end
end

RailsEnv = Struct.new(:env)

def rails_env env
Expand Down

0 comments on commit b96ea6f

Please sign in to comment.