Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Passing :protocol => :relative to stylesheet_link_tag adds attribute to link tag #8388

Closed
jachenry opened this Issue · 16 comments

7 participants

@jachenry

On Rails 3.2.9, Passing :protocol => :relative to stylesheet_link_tag adds an attribute to the link rather than forcing a relative path on the url. Without a way to force relative protocols I experience unexpected behavior when caching pages that are accessible through http and https.

application.html.erb (layout)

<%= stylesheet_link_tag    "manifest_application", "manifest_home", :protocol => :relative %>
<%= javascript_include_tag "manifest_application", "manifest_home" %>

learn_more.html

<link href="https://subdomain.cloudfront.net/assets/manifest_application-digest.css" media="screen" protocol="relative" rel="stylesheet" type="text/css" />
<link href="https://subdomain.cloudfront.net/assets/manifest_home-digest.css" media="screen" protocol="relative" rel="stylesheet" type="text/css" />
@carlosantoniodasilva

@jachenry have you tried it in previous versions of Rails? Just confirming if it's something that might have changed in 3.2.9. Thanks!

@nashby nashby referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@jachenry jachenry closed this
@jachenry jachenry reopened this
@jachenry

3.2.8 ignores the protocol option for stylesheets.

<link href="http://subdomain.cloudfront.net/assets/manifest_application.css?body=1" media="screen" rel="stylesheet" type="text/css" />

javascript tags get handled correctly using

config.action_controller.default_asset_host_protocol = :relative
@carlosantoniodasilva

Well, so I believe that should change the behavior for both javascripts and stylesheets. Either way, passing :protocol should work as well.

@rafaelfranca
Owner

@carlosantoniodasilva is right. @nashby could you see if the configuration option is working too and update your pull request?

@steveklabnik
Collaborator

Why don't we generate network path URIs, ie, //subdomain.cloudfront.net.... every time? that'd be so much easier.

@rafaelfranca
Owner

@steveklabnik this means that who want to generate always http URLs in a https page will not can.

@steveklabnik
Collaborator

Yeah but that's bad behavior: it will cause warnings in all browsers, and is insecure.

@guilleiguaran

Can someone confirm that :protocol => :relative option worked in any previous version of Rails? I don't remember and I don't think it did.

AFAIK config.action_controller.default_asset_host_protocol = :relative was added for this case

Anyway I think #8389 is a good for Rails 4.0 but it shouldn't be backported to 3-2-stable if is not a regression

@jachenry

Based on #3660 I think relative protocols was the default for stylesheets but was later changed to use :protocol => :request in sprockets rails helper because of a bug in some rss clients.

@nashby nashby referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@nashby

@rafaelfranca I've added two spec with configuration option, please take a look.

@steveklabnik
Collaborator

@jachenry ugh. Thanks.

@jachenry

@guilleiguaran config.action_controller.default_asset_host_protocol = :relative doesn't work for stylesheet_link_tags. It seems like I should be able to pass the :protocol and it will be used when generating the href. What am I missing?

actionpack/lib/sprockets/helpers/rails_helper.rb

      def stylesheet_link_tag(*sources)
        options = sources.extract_options!
        debug   = options.key?(:debug) ? options.delete(:debug) : debug_assets?
        body    = options.key?(:body)  ? options.delete(:body)  : false
        digest  = options.key?(:digest)  ? options.delete(:digest)  : digest_assets?

        sources.collect do |source|
          if debug && asset = asset_paths.asset_for(source, 'css')
            asset.to_a.map { |dep|
              super(dep.pathname.to_s, { :href => path_to_asset(dep, :ext => 'css', :body => true, :protocol => :request, :digest => digest) }.merge!(options))
            }
          else
            super(source.to_s, { :href => path_to_asset(source, :ext => 'css', :body => body, :protocol => :request, :digest => digest) }.merge!(options))
          end
        end.uniq.join("\n").html_safe
      end

EDIT

Ah. I see. It is forcing :protocol => :request.

@guilleiguaran

@jachenry maybe we can track in Git history to check why it's forcing to :protocol => :request and if there isn't a reason remove it

@jachenry

6c64e1e

It was authored over a year ago so its probably too late to remove.

@jachenry

although @chriseppstein did say the ie bug causing duplicate loading of stylesheets has been fixed. We may want to get his thoughts.

@dkubb

I just hit this bug in a page cached page. If the first request to the page uses http, then the cached page is returned over ssl it still references the http URL. I has expected that the following would force relative protocol URLs for all assets:

config.action_controller.default_asset_host_protocol = :relative
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.