Adds support for S3 scheme-less URL generation #1183

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

As-mentioned-but-lacking-a-folllow-up-in: #1098

I cherry-picked the commit without any difficulties.

@glebm glebm referenced this pull request in comfy/comfortable-mexican-sofa Mar 5, 2013
Closed

HTTPS URLs for page_file #266

@djcp djcp commented on the diff Mar 5, 2013
lib/paperclip/storage/s3.rb
Paperclip.interpolates(:s3_alias_url) do |attachment, style|
- "#{attachment.s3_protocol(style)}://#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{^/}, "")}"
+ url_prefix = (s3_protocol = attachment.s3_protocol(style)).present? ? "#{s3_protocol}:" : ""
djcp
djcp Mar 5, 2013 Contributor

We're probably guilty of breaking this rule all over the place in paperclip, but the non-ternary longer form of this is a bit more legible.

jyurek
jyurek Mar 8, 2013 Member

Similarly, since this is used in a few places, I would rather see it extracted into a helper.

@djcp djcp commented on the diff Mar 5, 2013
lib/paperclip/storage/s3.rb
@@ -39,8 +39,9 @@ module Storage
# :s3_permissions => :private
#
# * +s3_protocol+: The protocol for the URLs generated to your S3 assets. Can be either
- # 'http' or 'https'. Defaults to 'http' when your :s3_permissions are :public_read (the
- # default), and 'https' when your :s3_permissions are anything else.
+ # 'http', 'https', or an empty string to generate scheme-less URLs. Defaults to 'http'
djcp
djcp Mar 5, 2013 Contributor

"schema-less"

jyurek
jyurek Mar 8, 2013 Member

No, it's "scheme"

@djcp djcp commented on the diff Mar 5, 2013
lib/paperclip/storage/s3.rb
end unless Paperclip::Interpolations.respond_to? :s3_alias_url
Paperclip.interpolates(:s3_path_url) do |attachment, style|
- "#{attachment.s3_protocol(style)}://#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
+ url_prefix = (s3_protocol = attachment.s3_protocol(style)).present? ? "#{s3_protocol}:" : ""
djcp
djcp Mar 5, 2013 Contributor

same as line 131, ternaries lead to very dense lines that're hard to understand immediately.

@djcp djcp commented on the diff Mar 5, 2013
test/storage/s3_test.rb
@@ -129,6 +129,38 @@ def teardown
end
+ context ":s3_protocol => 'https'" do
djcp
djcp Mar 5, 2013 Contributor

Is there a test for when s3_protocol is explictly set to "http" and when it's not set at all?

Hmm, sorry, this was really just a cherry-pick from master into 2.7. Do you want me to clean up this code against master, or would you cherry pick the improvements back into master from this PR?

Contributor
djcp commented Mar 6, 2013

Ok, I got confused because a pull request was issued against our repo.

Aye, the pull request was because I needed this myself and ran into the ticket here (#1098) saying it would be desired to have it against the 2.7 branch, but nobody every followed up on that.

Member
jyurek commented Mar 8, 2013

@cbrunsdon This seems like something we'd want to pull in. If you'd like to clean it up from master (or from v2.7 if that's what you want) that would be great.

Member
jyurek commented Mar 15, 2013

I'm going to close this for now, but as we mentioned, if you want to clean this up for master, we'd be interested.

@jyurek jyurek closed this Mar 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment