Skip to content

Loading…

Fix empty host for an asset url when asset_host proc returns nil #16161

Merged
merged 1 commit into from

3 participants

@jpawlyn

If a CDN can be dynamically enabled in an app, when it is switched off and the asset_host proc returns nil, all asset urls are relative instead of absolute. This fix makes the urls absolute.

@matthewd
Ruby on Rails member

Per @pixeltrix's comment, please also assert that asset_path looks right here. (Though at a glance, it sounds like it will.)

@jpawlyn

Is this looking better or have I misunderstood?

@matthewd matthewd commented on an outdated diff
actionview/test/template/asset_tag_helper_test.rb
@@ -546,6 +546,14 @@ def test_image_path_with_asset_host_proc_returning_nil
assert_equal "http://cdn.example.com/images/file.png", image_path("file.png")
end
+ def test_image_url_with_asset_host_proc_returning_nil
+ @controller.config.asset_host = Proc.new { nil }
+ @controller.request = Struct.new(:base_url, :script_name).new("http://www.example.com", nil)
+
+ assert_equal "/foo", asset_path("foo")
@matthewd Ruby on Rails member
assert_equal "/images/rails.png", image_path("rails.png")

.. just as a direct contrast to image_url, because the other similar test that I saw doesn't set up base_url.

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

Cool, makes sense

@pixeltrix
Ruby on Rails member

@jpawlyn can you squash your commits and add an entry to CHANGELOG - thanks.

Jolyon Pawlyn Return an absolute instead of relative path from an asset url in the …
…case of the `asset_host` proc returning nil
d005777
@jpawlyn

@pixeltrix All done

@pixeltrix
Ruby on Rails member

@jpawlyn okay, thanks - just waiting on the CI and then I'll do the backports to 4-1-stable and 4-0-stable.

@pixeltrix pixeltrix merged commit c3f4d6c into rails:master

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@pixeltrix
Ruby on Rails member

Backported to 4-1-stable (08e528d) and 4-0-stable (0997ceb)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 15, 2014
  1. Return an absolute instead of relative path from an asset url in the …

    Jolyon Pawlyn committed
    …case of the `asset_host` proc returning nil
View
5 actionview/CHANGELOG.md
@@ -1,3 +1,8 @@
+* Return an absolute instead of relative path from an asset url in the case
+ of the `asset_host` proc returning nil
+
+ *Jolyon Pawlyn*
+
* Fix `html_escape_once` to properly handle hex escape sequences (e.g. ᨫ)
*John F. Douthat*
View
2 actionview/lib/action_view/helpers/asset_url_helper.rb
@@ -203,7 +203,6 @@ def compute_asset_host(source = "", options = {})
request = self.request if respond_to?(:request)
host = options[:host]
host ||= config.asset_host if defined? config.asset_host
- host ||= request.base_url if request && options[:protocol] == :request
if host.respond_to?(:call)
arity = host.respond_to?(:arity) ? host.arity : host.method(:call).arity
@@ -214,6 +213,7 @@ def compute_asset_host(source = "", options = {})
host = host % (Zlib.crc32(source) % 4)
end
+ host ||= request.base_url if request && options[:protocol] == :request
return unless host
if host =~ URI_REGEXP
View
8 actionview/test/template/asset_tag_helper_test.rb
@@ -546,6 +546,14 @@ def test_image_path_with_asset_host_proc_returning_nil
assert_equal "http://cdn.example.com/images/file.png", image_path("file.png")
end
+ def test_image_url_with_asset_host_proc_returning_nil
+ @controller.config.asset_host = Proc.new { nil }
+ @controller.request = Struct.new(:base_url, :script_name).new("http://www.example.com", nil)
+
+ assert_equal "/images/rails.png", image_path("rails.png")
+ assert_equal "http://www.example.com/images/rails.png", image_url("rails.png")
+ end
+
def test_caching_image_path_with_caching_and_proc_asset_host_using_request
@controller.config.asset_host = Proc.new do |source, request|
if request.ssl?
Something went wrong with that request. Please try again.