Replaced the old Paperclip.run with the new CommandLine class.

lib/paperclip.rb

@@ -75,14 +75,6 @@ module Paperclip
       yield(self) if block_given?
     end
 
-    def path_for_command command #:nodoc:
-      if options[:image_magick_path]
-        warn("[DEPRECATION] :image_magick_path is deprecated and will be removed. Use :command_path instead")
-      end
-      path = [options[:command_path] || options[:image_magick_path], command].compact
-      File.join(*path)
-    end
-
     def interpolates key, &block
       Paperclip::Interpolations[key] = block
     end
@@ -104,39 +96,11 @@ module Paperclip
     # Paperclip.options[:log_command] is set to true (defaults to false). This
     # will only log if logging in general is set to true as well.
     def run cmd, *params
-      options           = params.last.is_a?(Hash) ? params.pop : {}
-      expected_outcodes = options[:expected_outcodes] || [0]
-      params            = quote_command_options(*params).join(" ")
-
-      command = %Q[#{path_for_command(cmd)} #{params}]
-      command = "#{command} 2>#{bit_bucket}" if Paperclip.options[:swallow_stderr]
-      Paperclip.log(command) if Paperclip.options[:log_command]
-
-      begin
-        output = `#{command}`
-
-        raise CommandNotFoundError if $?.exitstatus == 127
-
-        unless expected_outcodes.include?($?.exitstatus)
-          raise PaperclipCommandLineError,
-            "Error while running #{cmd}. Expected return code to be #{expected_outcodes.join(", ")} but was #{$?.exitstatus}",
-            output
-        end
-      rescue Errno::ENOENT => e
-        raise CommandNotFoundError
-      end
-
-      output
-    end
-
-    def quote_command_options(*options)
-      options.map do |option|
-        option.split("'").map{|m| "'#{m}'" }.join("\\'")
+      if options[:image_magick_path]
+        Paperclip.log("[DEPRECATION] :image_magick_path is deprecated and will be removed. Use :command_path instead")
       end
-    end
-
-    def bit_bucket #:nodoc:
-      File.exists?("/dev/null") ? "/dev/null" : "NUL"
+      CommandLine.path = options[:command_path] || options[:image_magick_path]
+      CommandLine.new(cmd, *params).run
     end
 
     def included base #:nodoc:

lib/paperclip/command_line.rb

@@ -22,7 +22,15 @@ module Paperclip
     end
 
     def run
-      output = `#{command}`
+      Paperclip.log(command)
+      begin
+        output = self.class.send(:'`', command)
+      rescue Errno::ENOENT
+        raise Paperclip::CommandNotFoundError
+      end
+      if $?.exitstatus == 127
+        raise Paperclip::CommandNotFoundError
+      end
       unless @expected_outcodes.include?($?.exitstatus)
         raise Paperclip::PaperclipCommandLineError, "Command '#{command}' returned #{$?.exitstatus}. Expected #{@expected_outcodes.join(", ")}"
       end

lib/paperclip/geometry.rb

@@ -16,7 +16,7 @@ module Paperclip
     def self.from_file file
       file = file.path if file.respond_to? "path"
       geometry = begin
-                   Paperclip.run("identify", "-format", "%wx%h", "#{file}[0]")
+                   Paperclip.run("identify", "-format %wx%h :file", :file => "#{file}[0]")
                  rescue PaperclipCommandLineError
                    ""
                  end

lib/paperclip/thumbnail.rb

@@ -49,15 +49,16 @@ module Paperclip
       dst.binmode
 
       begin
-        options = [
-          source_file_options,
-          "#{ File.expand_path(src.path) }[0]",
-          transformation_command,
-          convert_options,
-          "#{ File.expand_path(dst.path) }"
-        ].flatten.compact
+        parameters = []
+        parameters << source_file_options
+        parameters << ":source"
+        parameters << transformation_command
+        parameters << convert_options
+        parameters << ":dest"
 
-        success = Paperclip.run("convert", *options)
+        parameters = parameters.flatten.compact.join(" ").strip.squeeze(" ")
+
+        success = Paperclip.run("convert", parameters, :source => "#{File.expand_path(src.path)}[0]", :dest => File.expand_path(dst.path))
       rescue PaperclipCommandLineError => e
         raise PaperclipError, "There was an error processing the thumbnail for #{@basename}" if @whiny
       end
@@ -70,8 +71,8 @@ module Paperclip
     def transformation_command
       scale, crop = @current_geometry.transformation_to(@target_geometry, crop?)
       trans = []
-      trans << "-resize" << scale unless scale.nil? || scale.empty?
-      trans << "-crop" << crop << "+repage" if crop
+      trans << "-resize" << "'#{scale}'" unless scale.nil? || scale.empty?
+      trans << "-crop" << "'#{crop}'" << "+repage" if crop
       trans
     end
   end

lib/paperclip/upfile.rb

@@ -17,7 +17,7 @@ module Paperclip
       when "csv", "xml", "css"       then "text/#{type}"
       else
         # On BSDs, `file` doesn't give a result code of 1 if the file doesn't exist.
-        content_type = (Paperclip.run("file", "--mime-type", self.path).split(':').last.strip rescue "application/x-#{type}")
+        content_type = (Paperclip.run("file", "--mime-type :file", :file => self.path).split(':').last.strip rescue "application/x-#{type}")
         content_type = "application/x-#{type}" if content_type.match(/\(.*?\)/)
         content_type
       end

test/command_line_test.rb

@@ -62,7 +62,7 @@ class CommandLineTest < Test::Unit::TestCase
 
   should "run the #command it's given and return the output" do
     cmd = Paperclip::CommandLine.new("convert", "a.jpg b.png")
-    cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+    cmd.class.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
     with_exitstatus_returning(0) do
       assert_equal :correct_value, cmd.run
     end
@@ -70,7 +70,7 @@ class CommandLineTest < Test::Unit::TestCase
 
   should "raise a PaperclipCommandLineError if the result code isn't expected" do
     cmd = Paperclip::CommandLine.new("convert", "a.jpg b.png")
-    cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+    cmd.class.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
     with_exitstatus_returning(1) do
       assert_raises(Paperclip::PaperclipCommandLineError) do
         cmd.run
@@ -82,11 +82,18 @@ class CommandLineTest < Test::Unit::TestCase
     cmd = Paperclip::CommandLine.new("convert",
                                      "a.jpg b.png",
                                      :expected_outcodes => [0, 1])
-    cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+    cmd.class.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
     with_exitstatus_returning(1) do
       assert_nothing_raised do
         cmd.run
       end
     end
   end
+
+  should "log the command" do
+    cmd = Paperclip::CommandLine.new("convert", "a.jpg b.png")
+    cmd.class.stubs(:'`')
+    Paperclip.expects(:log).with("convert a.jpg b.png")
+    cmd.run
+  end
 end

test/helper.rb

@@ -27,6 +27,8 @@ require 'action_pack'
 
 puts "Testing against version #{ActiveRecord::VERSION::STRING}"
 
+`ruby -e 'exit 0'` # Prime $? with a value.
+
 begin
   require 'ruby-debug'
 rescue LoadError => e

test/integration_test.rb

@@ -204,7 +204,7 @@ class IntegrationTest < Test::Unit::TestCase
       @bad_file  = File.new(File.join(FIXTURES_DIR, "bad.png"), 'rb')
 
       assert @dummy.avatar = @file
-      assert @dummy.valid?
+      assert @dummy.valid?, @dummy.errors.full_messages.join(", ")
       assert @dummy.save
     end
 

test/paperclip_test.rb

 require 'test/helper'
 
 class PaperclipTest < Test::Unit::TestCase
-  [:image_magick_path, :command_path].each do |path|
-    context "Calling Paperclip.run with #{path} specified" do
-      setup do
-        Paperclip.options[:image_magick_path] = nil
-        Paperclip.options[:command_path]      = nil
-        Paperclip.options[path]               = "/usr/bin"
-      end
-
-      should "return the expected path for path_for_command" do
-        assert_equal "/usr/bin/convert", Paperclip.path_for_command("convert")
-      end
-
-      should "execute the right command" do
-        Paperclip.expects(:path_for_command).with("convert").returns("/usr/bin/convert")
-        Paperclip.expects(:bit_bucket).returns("/dev/null")
-        Paperclip.expects(:"`").with("/usr/bin/convert 'one.jpg' 'two.jpg' 2>/dev/null")
-        Paperclip.run("convert", "one.jpg", "two.jpg")
-      end
-    end
-  end
-
-  context "Calling Paperclip.run with no path specified" do
+  context "Calling Paperclip.run" do
     setup do
       Paperclip.options[:image_magick_path] = nil
       Paperclip.options[:command_path]      = nil
+      Paperclip::CommandLine.stubs(:'`')
     end
 
-    should "return the expected path fro path_for_command" do
-      assert_equal "convert", Paperclip.path_for_command("convert")
+    should "execute the right command with :image_magick_path" do
+      Paperclip.options[:image_magick_path] = "/usr/bin"
+      Paperclip.expects(:log).with(includes('[DEPRECATION]'))
+      Paperclip.expects(:log).with("/usr/bin/convert 'one.jpg' 'two.jpg'")
+      Paperclip::CommandLine.expects(:"`").with("/usr/bin/convert 'one.jpg' 'two.jpg'")
+      Paperclip.run("convert", ":one :two", :one => "one.jpg", :two => "two.jpg")
     end
 
-    should "execute the right command" do
-      Paperclip.expects(:path_for_command).with("convert").returns("convert")
-      Paperclip.expects(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:"`").with("convert 'one.jpg' 'two.jpg' 2>/dev/null")
-      Paperclip.run("convert", "one.jpg", "two.jpg")
-    end
-  end
-
-  context "Calling Paperclip.run and logging" do
-    should "log the command when :log_command is true" do
-      Paperclip.options[:image_magick_path] = nil
-      Paperclip.options[:command_path]      = nil
-      Paperclip.stubs(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:log).with("this 'is the command' 2>/dev/null")
-      Paperclip.expects(:"`").with("this 'is the command' 2>/dev/null")
-      Paperclip.options[:log_command] = true
-      Paperclip.run("this","is the command")
+    should "execute the right command with :command_path" do
+      Paperclip.options[:command_path] = "/usr/bin"
+      Paperclip::CommandLine.expects(:"`").with("/usr/bin/convert 'one.jpg' 'two.jpg'")
+      Paperclip.run("convert", ":one :two", :one => "one.jpg", :two => "two.jpg")
     end
 
-    should "not log the command when :log_command is false" do
-      Paperclip.options[:image_magick_path] = nil
-      Paperclip.options[:command_path]      = nil
-      Paperclip.stubs(:bit_bucket).returns("/dev/null")
-      Paperclip.expects(:log).with("this 'is the command' 2>/dev/null").never
-      Paperclip.expects(:"`").with("this 'is the command' 2>/dev/null")
-      Paperclip.options[:log_command] = false
-      Paperclip.run("this","is the command")
+    should "execute the right command with no path" do
+      Paperclip::CommandLine.expects(:"`").with("convert 'one.jpg' 'two.jpg'")
+      Paperclip.run("convert", ":one :two", :one => "one.jpg", :two => "two.jpg")
     end
-  end
 
-  context "Calling Paperclip.run when the command is not found" do
     should "tell you the command isn't there if the shell returns 127" do
-      begin
+      with_exitstatus_returning(127) do
         assert_raises(Paperclip::CommandNotFoundError) do
-          `ruby -e 'exit 127'` # Stub $?.exitstatus to be 127, i.e. Command Not Found.
-          Paperclip.stubs(:"`").returns("")
           Paperclip.run("command")
         end
-      ensure
-        `ruby -e 'exit 0'` # Unstub $?.exitstatus
       end
     end
+
     should "tell you the command isn't there if an ENOENT is raised" do
       assert_raises(Paperclip::CommandNotFoundError) do
-        Paperclip.stubs(:"`").raises(Errno::ENOENT)
+        Paperclip::CommandLine.stubs(:"`").raises(Errno::ENOENT)
         Paperclip.run("command")
       end
     end
   end
 
-  should "prevent dangerous characters in the command via quoting" do
-    Paperclip.options[:image_magick_path] = nil
-    Paperclip.options[:command_path]      = nil
-    Paperclip.options[:log_command]       = false
-    Paperclip.options[:swallow_stderr]    = false
-    Paperclip.expects(:"`").with(%q[this is 'jack'\''s' '`command`' 'line!'])
-    Paperclip.run("this", "is :one :two :three", :one => "jack's", :two => "`command`", :three => "line!")
-  end
-
-  context "Paperclip.bit_bucket" do
-    context "on systems without /dev/null" do
-      setup do
-        File.expects(:exists?).with("/dev/null").returns(false)
-      end
-
-      should "return 'NUL'" do
-        assert_equal "NUL", Paperclip.bit_bucket
-      end
-    end
-
-    context "on systems with /dev/null" do
-      setup do
-        File.expects(:exists?).with("/dev/null").returns(true)
-      end
-
-      should "return '/dev/null'" do
-        assert_equal "/dev/null", Paperclip.bit_bucket
-      end
-    end
-  end
-
   should "raise when sent #processor and the name of a class that exists but isn't a subclass of Processor" do
     assert_raises(Paperclip::PaperclipError){ Paperclip.processor(:attachment) }
   end

test/thumbnail_test.rb

@@ -91,8 +91,8 @@ class ThumbnailTest < Test::Unit::TestCase
       end
 
       should "send the right command to convert when sent #make" do
-        Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '.*?'}
+        Paperclip::CommandLine.expects(:"`").with do |arg|
+          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' -resize 'x50' -crop '100x50\+114\+0' \+repage '.*?'}
         end
         @thumb.make
       end
@@ -115,8 +115,8 @@ class ThumbnailTest < Test::Unit::TestCase
       end
 
       should "send the right command to convert when sent #make" do
-        Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert '-strip' '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '.*?'}
+        Paperclip::CommandLine.expects(:"`").with do |arg|
+          arg.match %r{convert -strip '#{File.expand_path(@thumb.file.path)}\[0\]' -resize 'x50' -crop '100x50\+114\+0' \+repage '.*?'}
         end
         @thumb.make
       end
@@ -153,8 +153,8 @@ class ThumbnailTest < Test::Unit::TestCase
       end
 
       should "send the right command to convert when sent #make" do
-        Paperclip.expects(:"`").with do |arg|
-          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' '-resize' 'x50' '-crop' '100x50\+114\+0' '\+repage' '-strip' '-depth' '8' '.*?'}
+        Paperclip::CommandLine.expects(:"`").with do |arg|
+          arg.match %r{convert '#{File.expand_path(@thumb.file.path)}\[0\]' -resize 'x50' -crop '100x50\+114\+0' \+repage -strip -depth 8 '.*?'}
         end
         @thumb.make
       end