Validations work as expected with invalid images.

git-svn-id: https://svn.thoughtbot.com/plugins/paperclip/trunk@245 7bbfaf0e-4d1d-0410-9690-a8bb5f8ef2aa

Rakefile

@@ -24,7 +24,7 @@ end
 desc 'Clean up files.'
 desc 'Clean up files.'
 task :clean do |t|
 task :clean do |t|
   FileUtils.rm_rf "doc"
   FileUtils.rm_rf "doc"
-  FileUtils.rm_rf "repository"
+  FileUtils.rm_rf "test/public"
   FileUtils.rm_rf "tmp"
   FileUtils.rm_rf "tmp"
   FileUtils.rm "test/debug.log" rescue nil
   FileUtils.rm "test/debug.log" rescue nil
   FileUtils.rm "test/paperclip.db" rescue nil
   FileUtils.rm "test/paperclip.db" rescue nil

lib/paperclip.rb

@@ -78,7 +78,7 @@ module Paperclip
     def styles
     def styles
       @styles ||= thumbnails.merge(:original => nil)
       @styles ||= thumbnails.merge(:original => nil)
     end
     end
-    
+
     def thumbnails
     def thumbnails
       @thumbnails ||= @options[:thumbnails] || {}
       @thumbnails ||= @options[:thumbnails] || {}
     end
     end
@@ -114,9 +114,18 @@ module Paperclip
       @name       = name
       @name       = name
       @errors     = []
       @errors     = []
 
 
+      # class << @errors
+      #   def push_with_log arg
+      #     puts "Pushing '#{arg}' onto errors."
+      #     push_without_log arg
+      #   end
+      #   alias_method :push_without_log, :<<
+      #   alias_method :<<, :push_with_log
+      # end
+
       clear_files
       clear_files
       @dirty = true
       @dirty = true
-      
+
       self.original_filename  = @instance["#{name}_file_name"]
       self.original_filename  = @instance["#{name}_file_name"]
       self.content_type       = @instance["#{name}_content_type"]
       self.content_type       = @instance["#{name}_content_type"]
       self.original_file_size = @instance["#{name}_file_size"]
       self.original_file_size = @instance["#{name}_file_size"]
@@ -129,11 +138,11 @@ module Paperclip
       self.original_filename  = sanitize_filename(uploaded_file.original_filename)
       self.original_filename  = sanitize_filename(uploaded_file.original_filename)
       self.content_type       = uploaded_file.content_type
       self.content_type       = uploaded_file.content_type
       self.original_file_size = uploaded_file.size
       self.original_file_size = uploaded_file.size
-      self[:original]         = uploaded_file
+      self[:original]         = uploaded_file.read
       @dirty                  = true
       @dirty                  = true
 
 
       if definition.attachment_type == :image
       if definition.attachment_type == :image
-        make_thumbnails_from uploaded_file
+        make_thumbnails_from(self[:original])
       end
       end
     end
     end
 
 
@@ -153,8 +162,7 @@ module Paperclip
 
 
     def for_attached_files
     def for_attached_files
       @files.each do |style, data|
       @files.each do |style, data|
-        data.rewind if data && data.respond_to?(:rewind)
-        yield style, (data.respond_to?(:read) ? data.read : data)
+        yield style, data
       end
       end
     end
     end
 
 
@@ -204,10 +212,10 @@ module Paperclip
       end
       end
       interpolate( style, pattern )
       interpolate( style, pattern )
     end
     end
-    
+
     def read style = nil
     def read style = nil
       style ||= definition.default_style
       style ||= definition.default_style
-      self[style] ? (self[style].rewind && self[style].read) : IO.read(file_name(style))  
+      self[style] ? self[style] : IO.read(file_name(style))  
     end
     end
 
 
     def validate_existence *constraints
     def validate_existence *constraints
@@ -239,11 +247,11 @@ module Paperclip
         begin
         begin
           FileUtils.rm file_path if file_path
           FileUtils.rm file_path if file_path
         rescue SystemCallError => e
         rescue SystemCallError => e
-          raise PaperclipError, "Could not delete thumbnail." if Paperclip.options[:whiny_deletes] || complain
+          raise PaperclipError, "could not be deleted." if Paperclip.options[:whiny_deletes] || complain
         end
         end
       end
       end
     end
     end
-    
+
     def file_name style = nil
     def file_name style = nil
       style ||= definition.default_style
       style ||= definition.default_style
       interpolate( style, definition.path )
       interpolate( style, definition.path )
@@ -267,7 +275,7 @@ module Paperclip
     def make_thumbnails_from data
     def make_thumbnails_from data
       begin
       begin
         definition.thumbnails.each do |style, geometry|
         definition.thumbnails.each do |style, geometry|
-          self[style] = make_thumbnail geometry, data
+          self[style] = Thumbnail.make(geometry, data)
         end
         end
       rescue PaperclipError => e
       rescue PaperclipError => e
         errors << e.message
         errors << e.message
@@ -275,72 +283,9 @@ module Paperclip
         self[:original] = data
         self[:original] = data
       end
       end
     end
     end
-    
-    protected
-
-    def make_thumbnail geometry, data
-      return data if geometry.nil?
-
-      operator = geometry[-1,1]
-      begin
-        geometry, crop_geometry = geometry_for_crop(geometry, data) if operator == '#'
-        convert = Paperclip.path_for_command("convert")
-        command = "#{convert} - -scale '#{geometry}' #{operator == '#' ? "-crop '#{crop_geometry}'" : ""} - 2>/dev/null"
-        thumb = IO.popen(command, "w+") do |io|
-          data.rewind
-          io.write(data.read)
-          io.close_write
-          StringIO.new(io.read)
-        end
-      rescue Errno::EPIPE => e
-        raise PaperclipError, "could not be thumbnailed. Is ImageMagick or GraphicsMagick installed and available?"
-      rescue SystemCallError => e
-        raise PaperclipError, "could not be thumbnailed."
-      end
-      if Paperclip.options[:whiny_thumbnails] && !$?.success?
-        raise PaperclipError, "could not be thumbnailed because of an error with 'convert'."
-      end
-      thumb
-    end
-
-    def geometry_for_crop geometry, orig_io
-      identify = Paperclip.path_for_command("identify")
-      IO.popen("#{identify} -", "w+") do |io|
-        orig_io.rewind
-        io.write(orig_io.read)
-        io.close_write
-        if match = io.read.split[2].match(/(\d+)x(\d+)/)
-          src   = match[1,2].map(&:to_f)
-          srch  = src[0] > src[1]
-          dst   = geometry.match(/(\d+)x(\d+)/)[1,2].map(&:to_f)
-          dsth  = dst[0] > dst[1]
-          ar    = src[0] / src[1]
-
-          scale_geometry, scale = if dst[0] == dst[1]
-            if srch
-              [ "x#{dst[1].to_i}", src[1] / dst[1] ]
-            else
-              [ "#{dst[0].to_i}x", src[0] / dst[0] ]
-            end
-          elsif dsth
-            [ "#{dst[0].to_i}x", src[0] / dst[0] ]
-          else
-            [ "x#{dst[1].to_i}", src[1] / dst[1] ]
-          end
-
-          crop_geometry = if dsth
-            "%dx%d+%d+%d" % [ dst[0], dst[1], 0, (src[1] / scale - dst[1]) / 2 ]
-          else
-            "%dx%d+%d+%d" % [ dst[0], dst[1], (src[0] / scale - dst[0]) / 2, 0 ]
-          end
-
-          [ scale_geometry, crop_geometry ]
-        end
-      end
-    end
 
 
     # Helper Methods
     # Helper Methods
-    
+
     public
     public
 
 
     def interpolations
     def interpolations
@@ -379,7 +324,7 @@ module Paperclip
     def to_s
     def to_s
       url
       url
     end
     end
-    
+
     protected
     protected
 
 
     def is_a_file? data
     def is_a_file? data
@@ -393,6 +338,86 @@ module Paperclip
     end
     end
   end
   end
 
 
+  class Thumbnail
+    attr_accessor :geometry, :data
+    def initialize geometry, data
+      @geometry, @data = geometry, data
+    end
+    
+    def self.make geometry, data
+      new(geometry, data).make
+    end
+      
+    def make
+      return data if geometry.nil?
+      operator = geometry[-1,1]
+      begin
+        scale_geometry = geometry
+        scale_geometry, crop_geometry = geometry_for_crop if operator == '#'
+        convert = Paperclip.path_for_command("convert")
+        command = "#{convert} - -scale '#{scale_geometry}' #{operator == '#' ? "-crop '#{crop_geometry}'" : ""} - 2>/dev/null"
+        thumb = piping data, :to => command
+      rescue Errno::EPIPE => e
+        raise PaperclipError, "could not be thumbnailed. Is ImageMagick or GraphicsMagick installed and available?"
+      rescue SystemCallError => e
+        raise PaperclipError, "could not be thumbnailed."
+      end
+      if Paperclip.options[:whiny_thumbnails] && !$?.success?
+        raise PaperclipError, "could not be thumbnailed because of an error with 'convert'."
+      end
+      thumb
+    end
+
+    def geometry_for_crop
+      identify = Paperclip.path_for_command("identify")
+      piping data, :to => "#{identify} - 2>/dev/null" do |pipeout|
+        dimensions = pipeout.split[2]
+        if dimensions && (match = dimensions.match(/(\d+)x(\d+)/))
+          src   = match[1,2].map(&:to_f)
+          srch  = src[0] > src[1]
+          dst   = geometry.match(/(\d+)x(\d+)/)[1,2].map(&:to_f)
+          dsth  = dst[0] > dst[1]
+          ar    = src[0] / src[1]
+
+          scale_geometry, scale = if dst[0] == dst[1]
+            if srch
+              [ "x#{dst[1].to_i}", src[1] / dst[1] ]
+            else
+              [ "#{dst[0].to_i}x", src[0] / dst[0] ]
+            end
+          elsif dsth
+            [ "#{dst[0].to_i}x", src[0] / dst[0] ]
+          else
+            [ "x#{dst[1].to_i}", src[1] / dst[1] ]
+          end
+
+          crop_geometry = if dsth
+            "%dx%d+%d+%d" % [ dst[0], dst[1], 0, (src[1] / scale - dst[1]) / 2 ]
+          else
+            "%dx%d+%d+%d" % [ dst[0], dst[1], (src[0] / scale - dst[0]) / 2, 0 ]
+          end
+
+          [ scale_geometry, crop_geometry ]
+        else
+          raise PaperclipError, "does not contain a valid image."
+        end
+      end
+    end
+
+    def piping data, command, &block
+      self.class.piping(data, command, &block)
+    end
+    
+    def self.piping data, command, &block
+      command = command[:to] if command.respond_to?(:[]) && command[:to]
+      block ||= lambda {|d| d }
+      IO.popen(command, "w+") do |io|
+        io.write(data)
+        io.close_write
+        block.call(io.read)
+      end
+    end
+  end
 
 
   module ClassMethods
   module ClassMethods
     def has_attached_file *attachment_names
     def has_attached_file *attachment_names
@@ -401,7 +426,7 @@ module Paperclip
       include InstanceMethods
       include InstanceMethods
       after_save :save_attached_files
       after_save :save_attached_files
       before_destroy :destroy_attached_files
       before_destroy :destroy_attached_files
-      
+
       #class_inheritable_hash :attachment_definitions
       #class_inheritable_hash :attachment_definitions
       @attachment_definitions ||= {}
       @attachment_definitions ||= {}
       @attachment_names       ||= []
       @attachment_names       ||= []
@@ -424,7 +449,7 @@ module Paperclip
     def attached_files
     def attached_files
       @attachment_names
       @attachment_names
     end
     end
-    
+
     def attachment_definition_for attachment
     def attachment_definition_for attachment
       @attachment_definitions[attachment]
       @attachment_definitions[attachment]
     end
     end
@@ -434,7 +459,7 @@ module Paperclip
     def validates_attached_file *attachment_names
     def validates_attached_file *attachment_names
       validates_each *attachment_names do |record, name, attachment|
       validates_each *attachment_names do |record, name, attachment|
         attachment.errors.each do |error|
         attachment.errors.each do |error|
-          record.add(name, error)
+          record.errors.add(name, error)
         end
         end
       end
       end
     end
     end
@@ -453,13 +478,13 @@ module Paperclip
       @attachments ||= {}
       @attachments ||= {}
       @attachments[name] ||= Attachment.new(self, name, self.class.attachment_definition_for(name))
       @attachments[name] ||= Attachment.new(self, name, self.class.attachment_definition_for(name))
     end
     end
-    
+
     def save_attached_files
     def save_attached_files
       @attachments.each do |name, attachment|
       @attachments.each do |name, attachment|
         attachment.save
         attachment.save
       end
       end
     end
     end
-    
+
     def destroy_attached_files
     def destroy_attached_files
       @attachments.each do |name, attachment|
       @attachments.each do |name, attachment|
         attachment.destroy!
         attachment.destroy!

test/simply_shoulda.rb

@@ -24,10 +24,17 @@ class << Test::Unit::TestCase
   def should name, &test
   def should name, &test
     context_setups    = @context_setups.dup
     context_setups    = @context_setups.dup
     context_teardowns = @context_teardowns.dup
     context_teardowns = @context_teardowns.dup
-    define_method(["test:", @contexts, "should", name].join(" ")) do
+    define_method(["test:", @contexts, "should", name].flatten.join(" ")) do
       context_setups.each { |setup| self.instance_eval(&setup) }
       context_setups.each { |setup| self.instance_eval(&setup) }
       self.instance_eval(&test)
       self.instance_eval(&test)
       context_teardowns.each { |teardown| self.instance_eval(&teardown) }
       context_teardowns.each { |teardown| self.instance_eval(&teardown) }
     end
     end
   end
   end
+  
+  def should_eventually name
+    define_method(["test:", @contexts, "should eventually", name].flatten.join(" ")) do
+      STDOUT.print "X"
+      assert true
+    end
+  end
 end
 end
\ No newline at end of file

test/test_attachment.rb

@@ -8,12 +8,6 @@ class TestAttachment < Test::Unit::TestCase
       @definition = Paperclip::AttachmentDefinition.new("thing", {})
       @definition = Paperclip::AttachmentDefinition.new("thing", {})
       @attachment = Paperclip::Attachment.new(@dummy, "thing", @definition)
       @attachment = Paperclip::Attachment.new(@dummy, "thing", @definition)
     end
     end
-    
-    should "calculate geometries for cropping images" do
-      @file = File.new(File.join(File.dirname(__FILE__), "fixtures", "test_image.jpg"))
-      assert_equal ["50x", "50x25+0+10"], @attachment.send(:geometry_for_crop, "50x25", @file)
-      assert_equal ["x50", "50x50+2+0"], @attachment.send(:geometry_for_crop, "50x50", @file)
-    end
   end
   end
   
   
   context "The class Foo" do
   context "The class Foo" do
@@ -45,6 +39,7 @@ class TestAttachment < Test::Unit::TestCase
       end
       end
       
       
       should "be able to retrieve the data as a blob" do
       should "be able to retrieve the data as a blob" do
+        @file.rewind
         assert_equal @file.read, @foo.image.read
         assert_equal @file.read, @foo.image.read
       end
       end
       
       
@@ -78,7 +73,7 @@ class TestAttachment < Test::Unit::TestCase
       end
       end
       
       
       should "have no errors" do
       should "have no errors" do
-        assert @foo.image.errors.blank?
+        assert @foo.image.errors.blank?, @foo.image.errors.inspect
         assert @foo.errors.blank?
         assert @foo.errors.blank?
       end
       end
 
 
@@ -108,6 +103,19 @@ class TestAttachment < Test::Unit::TestCase
     end
     end
     
     
     context "with an invalid image attached to :image" do
     context "with an invalid image attached to :image" do
+      setup do
+        assert Foo.has_attached_file(:image, :thumbnails => {:small => "16x16", :medium => "100x100", :large => "250x250", :square => "32x32#"})
+        assert Foo.validates_attached_file(:image)
+        @foo = Foo.new
+        @file = File.new(File.join(File.dirname(__FILE__), "fixtures", "test_invalid_image.jpg"))
+        assert_nothing_raised{ @foo.image = @file }
+      end
+
+      should "not save" do
+        assert !@foo.save
+        assert @foo.errors.on(:image)
+        assert @foo.errors.on(:image).any?{|e| e.match(/does not contain a valid image/) }
+      end
     end
     end
   end
   end
 end
 end
\ No newline at end of file

test/test_thumbnailer.rb

+require 'test/unit'
+require File.dirname(__FILE__) + "/test_helper.rb"
+
+class TestThumbnailer < Test::Unit::TestCase
+  context "The Thumbnailer" do
+    should "calculate geometries for cropping images" do
+      @file = IO.read(File.join(File.dirname(__FILE__), "fixtures", "test_image.jpg"))
+      assert_equal ["50x", "50x25+0+10"], Paperclip::Thumbnail.new("50x25", @file).geometry_for_crop
+      assert_equal ["x50", "50x50+2+0"],  Paperclip::Thumbnail.new("50x50", @file).geometry_for_crop
+      assert_equal ["x50", "25x50+14+0"], Paperclip::Thumbnail.new("25x50", @file).geometry_for_crop
+    end
+    
+    should "be able to pipe commands" do
+      doc = %w(one two three four five)
+      expected = %w(five four one three two)
+      assert_equal expected, Paperclip::Thumbnail.piping(doc.join("\n"), "sort").split("\n")
+      assert_equal "Hello, World!\n", Paperclip::Thumbnail.piping("World", "ruby -e 'puts %Q{Hello, \#{STDIN.read}!}'")
+    end
+  
+    [
+      [:square, 125, 125],
+      [:long,   53,  225],
+      [:tall,   225, 53],
+      [:tiny,   16,  16]
+    ].each do |size, width, height|
+      context "given a #{size} image" do
+        setup do
+          @base_file = IO.read(File.join(File.dirname(__FILE__), "fixtures", "test_image.jpg"))
+          @file = Paperclip::Thumbnail.piping(@base_file, "convert - -scale '#{width}x#{height}!' -")
+          assert_match /#{width}x#{height}/, Paperclip::Thumbnailer.piping(@file, "identify -")
+          
+          @targets = {
+            :small => "50x50",
+            :same => nil,
+            :large => "100x100",
+            :shrink_to_large => "100x100>",
+            :crop_medium => "75x75#"
+          }
+        end
+        
+        should_eventually "generate correct thumbnails for the image"
+      end
+    end
+  end
+end
\ No newline at end of file