Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Pass full source to compute_asset_host #14119

Open
wants to merge 3 commits into from

5 participants

@dudeman

Currently if the source contains a query string that string is not available when computing the asset host.
This commit merges the the tail back onto the source before passing it to the compute_asset_host that way any query string paramaters will be available when computing the asset host.

@flevour

Hi @dudeman, how "making any query string param available to compute_asset_host" would benefit the final users? A use case description would help understand this issue better!

@dudeman

Hi @flevour,
Sure! Having the full source available to compute_asset_host also means that full source is available to a configured ActionController::Base.config.asset_host proc. This allow you to vary the asset host based on params in the query string of the source.

For example:
If you're importing production data into your staging environment, you may need images created in staging to be on your staging host, while images created in production are served on your production host. Adding a query string and looking for that in your asset_host proc is one way to do that.
Some example code, assuming a system where you let users upload a profile photo, stored on s3, and then import those profiles into your staging environment for testing.

# app/helpers/user_helper.rb
module UserHelper
  ...
  def profile_image_path(profile)
    profile.staging_upload? ? "#{profile.image_path}?staging=true" : profile.image_path
  end
  ...
end

# config/environments/staging.rb
...
  config.action_controller.asset_host = Proc.new do |source, request|
    staging_upload = source =~ /staging=true/
    if staging_upload 
      // return staging asset host
    else
      // production asset host
    end
  end
...

# app/views/user/profile.html.erb
<%= image_tag profile_image_url(profile) %>

Also, it's worth noting that this is how it used to behave in Rails 2 and 3. The full source had always been available to compute_asset_host (and the asset_host proc if you have one) until Rails 4.

@robin850 robin850 added the actionview label
@matthewd
Collaborator

@dudeman this sounds pretty reasonable. Could you please add a test case to ensure this doesn't go away again?

@dudeman

Happily. I've added a test, does that look good to you @matthewd ?

@arunagw
Collaborator

So this PR needs a rebase with master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  actionview/lib/action_view/helpers/asset_url_helper.rb
@@ -137,7 +137,7 @@ def asset_path(source, options = {})
source = "#{relative_url_root}#{source}" unless source.starts_with?("#{relative_url_root}/")
end
- if host = compute_asset_host(source, options)
+ if host = compute_asset_host("#{source}#{tail}", options)
source = "#{host}#{source}"
end
View
9 actionview/test/template/asset_tag_helper_test.rb
@@ -527,6 +527,15 @@ def test_should_not_modify_source_string
assert_equal copy, source
end
+ def test_query_string_is_passed_to_asset_host_proc
+ @controller.config.asset_host = Proc.new do |source|
+ assert_equal '/images/image.png?query=string', source
+ "cdn.example.com"
+ end
+
+ assert_equal 'http://cdn.example.com/images/image.png?query=string', image_path('image.png?query=string')
+ end
+
def test_image_path_with_asset_host_proc_returning_nil
@controller.config.asset_host = Proc.new do |source|
unless source.end_with?("tiff")
Something went wrong with that request. Please try again.