-
Notifications
You must be signed in to change notification settings - Fork 121
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
Links to assets on same host should not include hostname #103
Comments
Just doing a quick brain dump to capture my current thoughts on what we should do here.
@MicahChalmer, @ms – Thoughts? |
Makes sense to me. Seems a shame to have to re-implement what's already there in Sinatra, but given the conflict with Rails, it looks like it's needed it to make the Rails embedding work. |
Sorry about the delay in feedback. I've been pretty busy. I actually hadn't read this bug report when I realized that the last version behaved like this. I downgraded to 0.9.11 on my production server. I think that even if I am actually surprised I didn't catch that. I'll try any future changes I make on prod before asking you to merge code. It would be really nice to have smart tests to check that all the assumptions we make about getting the correct url work in different scenarios. About point 3, I'm not really familiar with embedding Nesta in a Rails process. Doesn't Nesta need some Sinatra code to run? If that's the case, wouldn't the helpers be imported no matter what? Looking simply at 1 and 2, the solution seems to be to define our own Finally for 4, what type of information would we need to deal with paths? I haven't read the source code of Sinatra's url, but would we simply copy their code? |
@ms - I think that the key to working in as many scenarios as possible will be always using Rack to mount Nesta at a non-root path. Nginx et al will then just be able to send requests straight through and let Rack deal with any relative path stuff. I appreciate the thought about trying stuff in production, but there are too many scenarios to catch everything. Really, we need decent integration tests, and I'm not going to get into that while the tests are still in RSpec. I'm switching them to MiniTest, after which point they'll be easier to tidy up. Rack::Test will be very useful in making sure this path stuff works in different scenarios. To put Nesta in Rails, all you need is for Nesta's own helpers to work in Rails. I've got a Rails view helper class that deals with that; it includes The new url helper will be easy to write; we can mimic Sinatra's, and remove the API that they added to Rack::Request that won't work in Rails (it's a pointless addition). Then we can just guarantee that it makes is relative paths, and we're good… |
One question about this: the atom feed and sitemap need absolute URLs, and were using absolute URLs (obtained via Or is this a moot point in light of #109 and the impending removal of the cache code altogether, and this issue only open to get rid of the |
Good question – I'll investigate… |
Okay, yes it is (was) a problem. I just setup 0.9.11 behind Nginx on my desktop machine and it hard codes http://localhost:9393 into the link elements in the Atom feed. This is a problem! I'll have a think. |
Okay, I've been doing some reading. Anybody who is running Nginx in front of a Ruby web server that runs Nesta should be able to configure Nginx to cope with this problem themselves. In Nginx you need to set the
See http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_set_header I presume other proxies will support the same thing. What this means for this patch is that it's okay to render absolute URLs (but I still think we should only do it when we really need to, such as in an Atom feed). |
Cool. That's what my pull request #110 does--full URLs in the atom feed and sitemap, but just paths everywhere else. |
Aye. It's what Nesta did originally too; I think we're good. I'm going to merge it soon, and then update the API a bit so that |
Coming back to this with fresh eyes, I'm going to close this issue. I don't consider current behaviour to be a serious bug (given my previous comment about how to configure a reverse proxy to control what the URI is set to, and our code in #110). |
If you run Nesta behind a proxy server (such as nginx) with caching enabled, the files that get cached to disk contain absolute links such as
http://127.0.0.1/css/master.css
. They ought to just show/css/master.css
.I imagine the same problem applies for links made to pages.
I strongly suspect we introduced this bug in #94, with the
url
helper from Sinatra./cc @ms
The text was updated successfully, but these errors were encountered: