-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
can't precompile assets when action_controller.asset_host accepts a 2nd argument #2947
Comments
Can you upload to a Git repo a minimal sample app to reproduce the issue? Thanks!!! |
@guilleiguaran, I reproduced the issue. frist, write above code in config/production.rb and Sprockets::Helpers::RailsHelper#asset_path |
The problem is that when the assets are precompiled the request isn't available and the helpers couldn't know calculate the host that need to be used for assets. I guess this bug will be difficult to solve |
I don't think so. Don't you just want assets:precompile to ignore the protocol? You definitely don't want "http" or "https" in your stylesheets, right? That is, all references should be relative to the domain: url(/assets/foo.png) - that way they work on both protocols. |
@crankharder, I think so. In actionpack/lib/sprockets/assets.rake Always ignore hostRails.application.config.action_controller.asset_host = nil It's work. |
@crankharder, Yes, the protocol is ignored but the host isn't ignored and in your example the host depends on the protocol. The assets are using protocol relative url, then if your hosts are config.action_controller.asset_host = Proc.new { |source, request| request.ssl? ? "ssl.assets.example.org" : "assets.example.org" } Maybe a better idea use only the |
@kennyj, If I have in my app site <link href="http://assets.example.org/application.css" rel="stylesheet" type="text/css" /> and I have inside the CSS a reference to an image: .logo { background: url(/assets/logo.png); } The image will be find in same domain of css ( |
@crankharder, @kennyj You are right, just ignore the asset_host in the context of the erb files during precompile will work fine. |
@guilleiguaran, |
@kennyj, yes, just confirmed same, then I think ignoring asset_host is fine during precompile |
@guilleiguaran - the link and script tags don't contain the host/protocol, they're relative - as they should be:
|
To be clear, I'm agreeing with you - asset_host should be ignored during precompile. |
commited on my repo. |
@crankharder: I think the link/script tags must include the asset host when the asset path is calculated inside a view (isn't equal to the domain app domain in all the cases): <link href="https://ssl.example.org/assets/application-aea14f68042284f50eb1c0d3154737e5.css" media="screen" rel="stylesheet" type="text/css" /> But it should be ignore inside the CSS files when .logo { background: url(/assets/logo-aea14f68042284f50eb1c0d3154737e5.png); } |
@guilleiguaran, I think so :-) |
@kennyj I think your fix can be merged but you will need to provide a test case. |
@guilleiguaran I go to sleep today and will continue tomorrow (in japan, it is 3:18 a.m. now.). |
@guilleiguaran this commit include testcase. |
@kennyj please squash the commits and send a pull request :) |
@guilleiguaran @crankharder I send a pull request #2987. |
Pushed |
Hey all. Thanks! Much appreciated. |
The use of asset hosts was one of the compelling features of the asset pipeline over alternatives. Couldn't your proc have just been changed to make the request argument optional? This change takes away the ability to use asset hosts in compiled assets from those that were not relying on the request object and doesn't even give them a configuration option. |
@cgriego This doesn't change the behavior of precompile:assets. It just makes it work without raising an error. You can still use <%= asset_path(...) %> in your css/js. |
The related pull request overwrites my application's config. How does that not change the behavior? |
It only overwrites it for the duration of the rake task. There's no request inside of a rake task... |
Right, I understand that piece, but the duration of the rake task is what precompiles the assets. So now none of the CSS, where the bulk of the images are used (images being the bulk of the http requests), benefit from asset hosts and there's nothing I can do about it. |
Can you show an asset_host as an example? |
Here, this is taken almost exactly from the Rails framework documentation. config.action_controller.asset_host = Proc.new { |source|
"//assets#{Digest::MD5.hexdigest(source).to_i(16) % 2 + 1}.example.com"
} |
I'm pretty sure that @cgriego has valid point. With this patch included all assets in the css will be served up on the domain that served that stylesheet. So the benefit of rotating assets on multiple subdomains is completely removed. I think the correct answer to my original question/problem is not a patch, but recognition that you can't serve secure assets on a single domain and also rotate unsecure assets on 4 subdomains - which is what my very first post is trying to achieve. I think I'm going to change my asset_host to this: config.action_controller.asset_host = Proc.new { |source| ....and get a wildcard cert. This would solve my problem without the patch. Ultimately, I think the patch should be reverted. |
Maybe revert and print a warning if config.action_controller.asset_host is a Proc with 2 args |
So... we should probably get this issue reopened? |
Thanks everyone! |
Is this issue resolved? I am currently in the same situation where I dont need absolute url of image but the image-url adds asset_host. config.action_controller.asset_host = Proc.new do |source, request| Anyone in same situation? |
Hope this link helps: |
fix rails/rails#2947 (with testcase)
So this, is basically straight from the docs:
config.action_controller.asset_host = Proc.new { |source, request|
if request.ssl?
"#{request.protocol}#{request.host_with_port}"
else
"#{request.protocol}assets#{(source.length % 4) + 1}.example.com"
end
}
http://api.rubyonrails.org/classes/ActionView/Helpers/AssetTagHelper.html (about 1/4 the way down, can't link, sorry.)
Anyways, deploying:
The text was updated successfully, but these errors were encountered: