Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

`asset_path` respects SCRIPT_NAME. #9646

Closed
wants to merge 2 commits into from

4 participants

@senny
Owner

Closes #2992

The code on 3-2-stable is very different but It should be an easy fix too.

@senny
Owner

@steveklabnik @guilleiguaran can you take a look?

@carlosantoniodasilva carlosantoniodasilva commented on the diff
actionpack/lib/action_view/helpers/asset_url_helper.rb
@@ -133,6 +133,8 @@ def asset_path(source, options = {})
end
relative_url_root = defined?(config.relative_url_root) && config.relative_url_root
+ request = self.request if respond_to?(:request)
+ relative_url_root ||= request.script_name if request

if respond_to?(:request) should be enough?

@senny Owner
senny added a note
@josevalim Owner

Yes, it returns nil for ActionMailer, for example.

@senny Owner
senny added a note

I guess we should keep it the way it is then. If this PR makes it in, I'd like to perform a quick cleanup/refactoring on the that helper code.

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

I am afraid this will cause unintended side effects. If you have a mountable engine, the script_name will also be set but in this case, asset_path still should not consider script path because the entity responsible for delivering assets is in the main application (sprockets in this case).

So I think we should create a mountable engine and ensure we can still access its assets after this patch. Things can get even trickier, because you have can have a mountable engine with Passenger's script_name and in this case we should use only the one coming from passenger. Here are a couple places to look at:

@senny
Owner

As far as I can tell AC still has a relative_url_root option but I think this won't fix the problem. If I interpret this test correctly: https://github.com/rails/rails/blob/master/actionpack/test/template/asset_tag_helper_test.rb#L558-L573
That option is already considered when computing the asset path and it won't be set when you change the rack mount point.

@pixeltrix
Owner

@senny you're meant to set config.relative_url_root to the same as the mount point for the application. IIRC, the original intention for Rails 3.0 was to deprecate it and the environment variable RAILS_RELATIVE_URL_ROOT and use SCRIPT_NAME exclusively. However it was undeprecated because of the need to be able get the relative url root outside of a request context like precompiling assets.

I think we need to have a CF chat about the various options and come up with a plan for sorting this out.

@senny
Owner

@pixeltrix thanks for clarifying. I put my work on hold and catch you on CF.

@senny
Owner

I'm closing this one as this is not a viable solution. I'm working on a different approach.

@senny senny closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 10, 2013
  1. @senny

    `asset_path` respects SCRIPT_NAME.

    senny authored
    Closes #2992
  2. @senny

    remove trailing whitespace

    senny authored
This page is out of date. Refresh to see the latest.
View
2  actionpack/CHANGELOG.md
@@ -1,5 +1,7 @@
## Rails 4.0.0 (unreleased) ##
+* `asset_path` respects SCRIPT_NAME. Fixes #2992. *Yves Senn*
+
* Fix incorrectly appended square brackets to a multiple select box
if an explicit name has been given and it already ends with "[]"
View
2  actionpack/lib/action_view/helpers/asset_url_helper.rb
@@ -133,6 +133,8 @@ def asset_path(source, options = {})
end
relative_url_root = defined?(config.relative_url_root) && config.relative_url_root
+ request = self.request if respond_to?(:request)
+ relative_url_root ||= request.script_name if request

if respond_to?(:request) should be enough?

@senny Owner
senny added a note
@josevalim Owner

Yes, it returns nil for ActionMailer, for example.

@senny Owner
senny added a note

I guess we should keep it the way it is then. If this PR makes it in, I'd like to perform a quick cleanup/refactoring on the that helper code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if relative_url_root
source = "#{relative_url_root}#{source}" unless source.starts_with?("#{relative_url_root}/")
end
View
14 actionpack/test/template/asset_tag_helper_test.rb
@@ -443,8 +443,8 @@ def test_image_alt
[nil, '/', '/foo/bar/', 'foo/bar/'].each do |prefix|
assert_equal 'Rails', image_alt("#{prefix}rails.png")
assert_equal 'Rails', image_alt("#{prefix}rails-9c0a079bdd7701d7e729bd956823d153.png")
- assert_equal 'Long file name with hyphens', image_alt("#{prefix}long-file-name-with-hyphens.png")
- assert_equal 'Long file name with underscores', image_alt("#{prefix}long_file_name_with_underscores.png")
+ assert_equal 'Long file name with hyphens', image_alt("#{prefix}long-file-name-with-hyphens.png")
+ assert_equal 'Long file name with underscores', image_alt("#{prefix}long_file_name_with_underscores.png")
end
end
@@ -733,6 +733,16 @@ def request
assert_equal "http://www.example.com/foo", @module.asset_url("foo")
end
+ def test_asset_url_with_script_name
+ @module.instance_eval do
+ def request
+ Struct.new(:base_url, :script_name).new("http://www.example.com", "/app")
+ end
+ end
+
+ assert_equal "http://www.example.com/app/foo", @module.asset_url("foo")
+ end
+
def test_asset_url_with_config_asset_host
@module.instance_eval do
def config
Something went wrong with that request. Please try again.