Permalink
Browse files

Only use Tempfile subclass in 1.8.6 and below. Fixes #278.

  • Loading branch information...
1 parent 05498d2 commit 1fef4c302d076575a1ca9691e01eb96ee9262ebc @jyurek jyurek committed Oct 6, 2010
Showing with 21 additions and 8 deletions.
  1. +1 −1 lib/paperclip/iostream.rb
  2. +13 −4 lib/paperclip/processor.rb
  3. +4 −1 lib/paperclip/storage/s3.rb
  4. +1 −1 lib/paperclip/thumbnail.rb
  5. +2 −1 test/thumbnail_test.rb
@@ -5,7 +5,7 @@ module IOStream
# Returns a Tempfile containing the contents of the readable object.
def to_tempfile
name = respond_to?(:original_filename) ? original_filename : (respond_to?(:path) ? path : "stream")
- tempfile = Paperclip::Tempfile.new("stream" + File.extname(name))
+ tempfile = Paperclip::Tempfile.new(["stream", File.extname(name)])
tempfile.binmode
self.stream_to(tempfile)
end
View
@@ -40,10 +40,19 @@ def self.make file, options = {}, attachment = nil
# on this blog post:
# http://marsorange.com/archives/of-mogrify-ruby-tempfile-dynamic-class-definitions
class Tempfile < ::Tempfile
- # Replaces Tempfile's +make_tmpname+ with one that honors file extensions.
- def make_tmpname(basename, n)
- extension = File.extname(basename)
- sprintf("%s,%d,%d%s", File.basename(basename, extension), $$, n.to_i, extension)
+ # This is Ruby 1.8.7's implementation.
+ if RUBY_VERSION <= "1.8.6"
+ def make_tmpname(basename, n)
+ case basename
+ when Array
+ prefix, suffix = *basename
+ else
+ prefix, suffix = basename, ''
+ end
+
+ t = time.now.strftime("%y%m%d")
+ path = "#{prefix}#{t}-#{$$}-#{rand(0x100000000).to_s(36)}-#{n}#{suffix}"
+ end
end
end
end
@@ -127,7 +127,10 @@ def s3_protocol
# style, in the format most representative of the current storage.
def to_file style = default_style
return @queued_for_write[style] if @queued_for_write[style]
- file = Tempfile.new(path(style))
+ filename = path(style).split(".")
@r3ap3r2004

r3ap3r2004 Oct 11, 2010

This is a bug waiting to happen. If path(style) returns something like /Some/Path/To/my.files/myfile.jpg then you end up with a 3 element array. Of course it doesn't really matter since the following two lines are also broken.

@kylecrum

kylecrum Oct 17, 2010

Yes, I'm getting that bug now. I'll have to revert to an older commit for the meantime.

@dolzenko

dolzenko Oct 20, 2010

+1 same problem here, had to revert to 2.3.3

@nikz

nikz Oct 24, 2010

+1 same problem, this is definitely a bug - patches welcome?

@pascalbetz

pascalbetz Oct 25, 2010

+1 same problem. don't split as File.extname does not take Arrays

+ extname = File.extname(filename)
@r3ap3r2004

r3ap3r2004 Oct 11, 2010

This doesn't work because filename is now an array instead of a string so you get this error:
"failed with TypeError: can't convert Array into String"
Same is true for the next line.

@benhutton

benhutton Oct 12, 2010

I'm getting this error as well! I'd go write a patch for it, except I don't really understand what you're trying to do here.

@nielsm

nielsm Oct 20, 2010

We also started getting the can't convert Array into String error after updating to 2.3.4. This change should be reverted or better guarded against.

@pdeffendol

pdeffendol Oct 20, 2010

Changing the previous line to:

filename = path(style)

seems to work, or am I missing something here?

@matthutchinson

matthutchinson Oct 25, 2010

doing that then gives an error on TempFile.new (dir not existing); I patched this method like so;

def to_file style = default_style
  return @queued_for_write[style] if @queued_for_write[style]
  filename = path(style)
  extname  = File.extname(path(style).split(".").last)
  basename = File.basename(filename, extname)
  file = Tempfile.new(basename)
  file.write(AWS::S3::S3Object.value(path(style), bucket_name))
  file.rewind
  return file
end
@masterkain

masterkain Oct 26, 2010

jyurek, please apply a patch if you can, thanks.

@matthutchinson

matthutchinson Oct 26, 2010

Actually this is safer, for cases where the uploaded file has a filename with no extension

def to_file style = default_style
    return @queued_for_write[style] if @queued_for_write[style]
    filename = path(style)           
    extname  = filename.include?('.') ? File.extname(filename.split(".").last) : ''
    basename = File.basename(filename, extname)
    file = Tempfile.new(basename)
    file.write(AWS::S3::S3Object.value(path(style), bucket_name))
    file.rewind
    return file
  end
@pdeffendol

pdeffendol Oct 26, 2010

Why the .split...last? File.extname always returns the file extension (including the dot) and an empty string if there is no extension. Isn't this more compact:

def to_file style = default_style
  return @queued_for_write[style] if @queued_for_write[style]
  filename = path(style)           
  extname  = File.extname(filename)
  basename = File.basename(filename, extname)
  file = Tempfile.new(basename)
  file.write(AWS::S3::S3Object.value(filename, bucket_name))
  file.rewind
  return file
end

See the documentation for File.extname - http://corelib.rubyonrails.org/classes/File.html#M000031

@matthutchinson

matthutchinson Oct 26, 2010

Sorry, missed that, yep that'll work

@jyurek

jyurek Oct 26, 2010

Member

Sorry about this, guys. This is fixed in 1d77573.

+ basename = File.basename(filename, extname)
+ file = Tempfile.new(basename, extname)
file.write(AWS::S3::S3Object.value(path(style), bucket_name))
file.rewind
return file
@@ -45,7 +45,7 @@ def convert_options?
# that contains the new image.
def make
src = @file
- dst = Tempfile.new([@basename, @format].compact.join("."))
+ dst = Tempfile.new([@basename, @format ? ".#{@format}" : ''])
dst.binmode
begin
View
@@ -4,10 +4,11 @@ class ThumbnailTest < Test::Unit::TestCase
context "A Paperclip Tempfile" do
setup do
- @tempfile = Paperclip::Tempfile.new("file.jpg")
+ @tempfile = Paperclip::Tempfile.new(["file", ".jpg"])
end
should "have its path contain a real extension" do
+ p @tempfile.path
assert_equal ".jpg", File.extname(@tempfile.path)
end

0 comments on commit 1fef4c3

Please sign in to comment.