New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RACK_BASE_URI ignored in `url` with string paths #1535

Closed
amadanmath opened this Issue Dec 27, 2013 · 5 comments

Comments

Projects
None yet
2 participants
@amadanmath

amadanmath commented Dec 27, 2013

Pursuant to #1532, and related to #1534:

Routing::InstanceMethods#url completely delegates to Sinatra's url for a simple string argument like url '/foo'. However, Sinatra has no concept of RACK_BASE_URI, and thus when it is defined generates a wrong URL. As a consequence, url '/foo' behaviour is inconsistent with url :foo.

Likely fix:

The quoted code from Routing::ClassMethods#url should be extracted and applied to any URL starting with a slash (i.e. /foo), but not on absolute and relative URLs (i.e. http://example.com/foo or foo).

url[0,0] = conform_uri(uri_root) if defined?(uri_root)
url[0,0] = conform_uri(ENV['RACK_BASE_URI']) if ENV['RACK_BASE_URI']

Currently this is applied to URLs that are compiled by Padrino, but not to URLs received from Sinatra.

As noted in #1532, there is a workaround for this by using asset_path instead, so it is much lower priority than #1534.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 27, 2013

Member

If an app mounted to '/app' and has get 'test' do; end route:

url :test #=> '/app/test'
url 'foo' #=> 'foo'
url 'foo', true, true #=> 'http://10.0.0.10:3000/app/foo'
url '/foo' #=> '/app/foo'

absolute_url :test #=> 'http://10.0.0.10:3000/app/test'
absolute_url 'foo' #=> 'http://10.0.0.10:3000/foo'
absolute_url '/foo' #=> 'http://10.0.0.10:3000/app/foo'

If that's right, I have a commit ready.

@padrino/core-members any input?

Member

ujifgc commented Dec 27, 2013

If an app mounted to '/app' and has get 'test' do; end route:

url :test #=> '/app/test'
url 'foo' #=> 'foo'
url 'foo', true, true #=> 'http://10.0.0.10:3000/app/foo'
url '/foo' #=> '/app/foo'

absolute_url :test #=> 'http://10.0.0.10:3000/app/test'
absolute_url 'foo' #=> 'http://10.0.0.10:3000/foo'
absolute_url '/foo' #=> 'http://10.0.0.10:3000/app/foo'

If that's right, I have a commit ready.

@padrino/core-members any input?

@amadanmath

This comment has been minimized.

Show comment
Hide comment
@amadanmath

amadanmath Dec 27, 2013

Thank you! absolute_url 'foo' giving 'http://10.0.0.10:3000/foo' is a bit unintuitive for me, but other than that, looks great. Not a core member, though :)

(That is, I assume foo/test variation is a search/replace gone bad, otherwise url 'foo', true, true giving 'http://10.0.0.10:3000/app/test' is really weird! :D )

amadanmath commented Dec 27, 2013

Thank you! absolute_url 'foo' giving 'http://10.0.0.10:3000/foo' is a bit unintuitive for me, but other than that, looks great. Not a core member, though :)

(That is, I assume foo/test variation is a search/replace gone bad, otherwise url 'foo', true, true giving 'http://10.0.0.10:3000/app/test' is really weird! :D )

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 27, 2013

Member

What would be an intuitive thing to return on absolute_url 'foo'?

Member

ujifgc commented Dec 27, 2013

What would be an intuitive thing to return on absolute_url 'foo'?

@amadanmath

This comment has been minimized.

Show comment
Hide comment
@amadanmath

amadanmath Dec 27, 2013

It's probably not worth doing (seems a bit complicated, and you should pretty much always be able to replace relative links to rooted links in your own application), but if you're rendering http://example.com/app/bar/baz, I'd expect absolute_url 'foo' to return http://example.com/app/bar/foo, i.e. take the current request URL, and replace whatever is after the last slash in path with the argument. But request.url also gives misleading result, so it's not that simple...

amadanmath commented Dec 27, 2013

It's probably not worth doing (seems a bit complicated, and you should pretty much always be able to replace relative links to rooted links in your own application), but if you're rendering http://example.com/app/bar/baz, I'd expect absolute_url 'foo' to return http://example.com/app/bar/foo, i.e. take the current request URL, and replace whatever is after the last slash in path with the argument. But request.url also gives misleading result, so it's not that simple...

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 27, 2013

Member

Makes sense to me. I will try it in a while.

Member

ujifgc commented Dec 27, 2013

Makes sense to me. I will try it in a while.

@ghost ghost assigned ujifgc Dec 27, 2013

@ujifgc ujifgc closed this in 5a0e566 Dec 31, 2013

ujifgc added a commit that referenced this issue Dec 31, 2013

Merge pull request #1537 from padrino/fix-1535
rebase string urls to uri_root, closes #1535

Ortuna added a commit to Ortuna/padrino-framework that referenced this issue Jan 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment