Fix regression on s3_protocol

`s3_protocol` now returns the protocol without a colon. If you need a colon, you can pass in `true` as a second argument. This might not be a best solution, so any better patch that does not introduce a regression is welcome. Fixes thoughtbot#921
mickdwyer · Jul 4, 2012 · 37999b6 · 37999b6
1 parent 9c92cd9
commit 37999b6
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 11 deletions.
diff --git a/lib/paperclip/storage/s3.rb b/lib/paperclip/storage/s3.rb
@@ -150,13 +150,13 @@ def sanitize_hash(hash)
         end
 
         Paperclip.interpolates(:s3_alias_url) do |attachment, style|
-          "#{attachment.s3_protocol(style)}//#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{^/}, "")}"
+          "#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_alias}/#{attachment.path(style).gsub(%r{^/}, "")}"
         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{^/}, "")}"
+          "#{attachment.s3_protocol(style, true)}//#{attachment.s3_host_name}/#{attachment.bucket_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
         end unless Paperclip::Interpolations.respond_to? :s3_path_url
         Paperclip.interpolates(:s3_domain_url) do |attachment, style|
-          "#{attachment.s3_protocol(style)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
+          "#{attachment.s3_protocol(style, true)}//#{attachment.bucket_name}.#{attachment.s3_host_name}/#{attachment.path(style).gsub(%r{^/}, "")}"
         end unless Paperclip::Interpolations.respond_to? :s3_domain_url
         Paperclip.interpolates(:asset_host) do |attachment, style|
           "#{attachment.path(style).gsub(%r{^/}, "")}"
@@ -276,15 +276,15 @@ def s3_permissions(style = default_style)
         s3_permissions
       end
 
-      def s3_protocol(style = default_style)
-        protocol = if @s3_protocol.respond_to?(:call)
-          @s3_protocol.call(style, self).to_s
+      def s3_protocol(style = default_style, with_colon = false)
+        protocol = @s3_protocol
+        protocol = protocol.call(style, self) if protocol.respond_to?(:call)
+
+        if with_colon && !protocol.empty?
+          "#{protocol}:"
         else
-          @s3_protocol.to_s
+          protocol.to_s
         end
-
-        protocol = protocol.split(":").first + ":" unless protocol.empty?
-        protocol
       end
 
       def create_bucket
@@ -359,7 +359,7 @@ def find_credentials creds
       end
 
       def use_secure_protocol?(style_name)
-        s3_protocol(style_name) == "https:"
+        s3_protocol(style_name) == "https"
       end
     end
   end

diff --git a/test/storage/s3_test.rb b/test/storage/s3_test.rb
@@ -125,7 +125,22 @@ def teardown
     should "use the correct key" do
       assert_equal "avatars/stringio.txt", @dummy.avatar.s3_object.key
     end
+  end
+
+  context "s3_protocol" do
+    ["http", :http, ""].each do |protocol|
+      context "as #{protocol.inspect}" do
+        setup do
+          rebuild_model :storage => :s3, :s3_protocol => protocol
 
+          @dummy = Dummy.new
+        end
+
+        should "return the s3_protocol in string" do
+          assert_equal protocol.to_s, @dummy.avatar.s3_protocol
+        end
+      end
+    end
   end
 
   context ":s3_protocol => 'https'" do