From bf9b28f942c95cd2876fdca8a563e59dcf09c84e Mon Sep 17 00:00:00 2001
From: Joe Atzberger <ohiocore@gmail.com>
Date: Tue, 7 Feb 2017 19:34:55 -0800
Subject: [PATCH 1/6] Rework #filename tests for DRYness and robustness

- Use explicit subject w/ 1-liner `it` blocks
- Don't stub, use a real object.

In particular `original_filename` has enough complexity that it is
unclear that the behavior of stubbed objects corresponds to instances that
are actually possible to create in the wild.

**Case in point**, the nix directory test no longer passes, though it is
ostensibly doing the same thing!

```
expected: "_."
     got: "carrierwave"
```
That is a result of, effectively, `File.basename(File.expand_path(@file))`
between `original_filename` ane `path`.
---
 spec/sanitized_file_spec.rb | 62 ++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/spec/sanitized_file_spec.rb b/spec/sanitized_file_spec.rb
index aa33a4840..324d914e4 100644
--- a/spec/sanitized_file_spec.rb
+++ b/spec/sanitized_file_spec.rb
@@ -103,56 +103,56 @@
   end
 
   describe "#filename" do
-    let(:sanitized_file) { CarrierWave::SanitizedFile.new(nil) }
+    subject { described_class.new(args).filename }
 
-    it "should default to the original filename if it is valid" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("llama.jpg")
-      expect(sanitized_file.filename).to eq("llama.jpg")
+    describe 'should default to the original filename if it is valid' do
+      let(:args) { 'llama.jpg' }
+      it { is_expected.to eq args }
     end
 
-    it "should remove illegal characters from a filename" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("test-s,%&m#st?.jpg")
-      expect(sanitized_file.filename).to eq("test-s___m_st_.jpg")
+    describe 'should remove illegal characters from a filename' do
+      let(:args) { 'test-s,%&m#st?.jpg' }
+      it { is_expected.to eq 'test-s___m_st_.jpg' }
     end
 
-    it "should remove slashes from the filename" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("../../very_tricky/foo.bar")
-      expect(sanitized_file.filename).not_to match(/[\\\/]/)
+    describe 'should remove slashes from the filename' do
+      let(:args) { '../../very_tricky/foo.bar' }
+      it { is_expected.not_to match(/[\\\/]/) }
     end
 
-    it "should remove illegal characters if there is no extension" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("`*foo")
-      expect(sanitized_file.filename).to eq("__foo")
+    describe 'should remove illegal characters if there is no extension' do
+      let(:args) { '`*foo' }
+      it { is_expected.to eq '__foo' }
     end
 
-    it "should remove the path prefix on Windows" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return('c:\temp\foo.txt')
-      expect(sanitized_file.filename).to eq("foo.txt")
+    describe 'should remove the path prefix on Windows' do
+      let(:args) { 'c:\temp\foo.txt' }
+      it { is_expected.to eq 'foo.txt' }
     end
 
-    it "should make sure the *nix directory thingies can't be used as filenames" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return(".")
-      expect(sanitized_file.filename).to eq("_.")
+    describe 'should make sure the *nix directory thingies cannot be used as filenames' do
+      let(:args) { '.' }
+      it { is_expected.to eq '_.' }
     end
 
-    it "should maintain uppercase filenames" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("DSC4056.JPG")
-      expect(sanitized_file.filename).to eq("DSC4056.JPG")
+    describe 'should maintain uppercase filenames' do
+      let(:args) { 'DSC4056.JPG' }
+      it { is_expected.to eq args }
     end
 
-    it "should remove illegal characters from a non-ASCII filename" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("⟲«Du côté des chars lourds»_123.doc")
-      expect(sanitized_file.filename).to eq("__Du_côté_des_chars_lourds__123.doc")
+    describe 'should remove illegal characters from a non-ASCII filename' do
+      let(:args) { '⟲«Du côté des chars lourds»_123.doc' }
+      it { is_expected.to eq '__Du_côté_des_chars_lourds__123.doc' }
     end
 
-    it "should default to the original non-ASCII filename if it is valid" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("тестовый.jpg")
-      expect(sanitized_file.filename).to eq("тестовый.jpg")
+    describe 'should default to the original non-ASCII filename if it is valid' do
+      let(:args) { 'тестовый.jpg' }
+      it { is_expected.to eq 'тестовый.jpg' }
     end
 
-    it "should downcase non-ASCII characters properly" do
-      expect(sanitized_file).to receive(:original_filename).at_least(:once).and_return("ТестоВый Ёжик.jpg")
-      expect(sanitized_file.filename).to eq("ТестоВый_Ёжик.jpg")
+    describe 'should downcase non-ASCII characters properly' do
+      let(:args) { 'ТестоВый Ёжик.jpg' }
+      it { is_expected.to eq 'ТестоВый_Ёжик.jpg' }
     end
   end
 

From 8ac31f6f21707f5a6e1509dd98db9eeda21bd552 Mon Sep 17 00:00:00 2001
From: Joe Atzberger <ohiocore@gmail.com>
Date: Wed, 8 Feb 2017 14:41:11 -0800
Subject: [PATCH 2/6] New test, new try_uri method

The point is to avoid calling `File.expand_path` or `File.basename` on a thing that is not,
in fact, a local path: i.e., if it is a plausible URI that has a hostname.

In the future this might be improved to create and cache a URI object as
an attribute, clearing it when `file=` is called.

Fixes #2116
---
 lib/carrierwave/sanitized_file.rb | 46 +++++++++++++++++++------------
 spec/sanitized_file_spec.rb       | 11 +++++++-
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/lib/carrierwave/sanitized_file.rb b/lib/carrierwave/sanitized_file.rb
index c1e62b9e1..f680d8f87 100644
--- a/lib/carrierwave/sanitized_file.rb
+++ b/lib/carrierwave/sanitized_file.rb
@@ -35,7 +35,7 @@ def initialize(file)
     end
 
     ##
-    # Returns the filename as is, without sanitizing it.
+    # Returns the filename (not the path) as is, without sanitizing it.
     #
     # === Returns
     #
@@ -43,11 +43,10 @@ def initialize(file)
     #
     def original_filename
       return @original_filename if @original_filename
-      if @file and @file.respond_to?(:original_filename)
-        @file.original_filename
-      elsif path
-        File.basename(path)
-      end
+      return @file.original_filename if @file && @file.respond_to?(:original_filename)
+      return unless path
+      uri = try_uri(path)
+      File.basename(uri && uri.hostname ? uri.path : path)
     end
 
     ##
@@ -113,13 +112,11 @@ def size
     # [String, nil] the path where the file is located.
     #
     def path
-      unless @file.blank?
-        if is_path?
-          File.expand_path(@file)
-        elsif @file.respond_to?(:path) and not @file.path.blank?
-          File.expand_path(@file.path)
-        end
-      end
+      return if @file.blank?
+      return File.expand_path(@file.path) if @file.respond_to?(:path) && !@file.path.blank?
+      return unless is_path?
+      uri = try_uri(@file)
+      uri && uri.hostname ? uri.to_s : File.expand_path(@file)
     end
 
     ##
@@ -130,6 +127,7 @@ def path
     def is_path?
       !!((@file.is_a?(String) || @file.is_a?(Pathname)) && !@file.blank?)
     end
+    alias_method :path?, :is_path?
 
     ##
     # === Returns
@@ -180,8 +178,8 @@ def read
     #
     def move_to(new_path, permissions=nil, directory_permissions=nil, keep_filename=false)
       return if self.empty?
-      new_path = File.expand_path(new_path)
-
+      uri = try_uri(new_path)
+      new_path = File.expand_path(new_path) unless uri && uri.hostname
       mkdir!(new_path, directory_permissions)
       move!(new_path)
       chmod!(new_path, permissions)
@@ -218,8 +216,8 @@ def move!(new_path)
     #
     def copy_to(new_path, permissions=nil, directory_permissions=nil)
       return if self.empty?
-      new_path = File.expand_path(new_path)
-
+      uri = try_uri(new_path)
+      new_path = File.expand_path(new_path) unless uri && uri.hostname
       mkdir!(new_path, directory_permissions)
       copy!(new_path)
       chmod!(new_path, permissions)
@@ -322,6 +320,8 @@ def chmod!(path, permissions)
     # Sanitize the filename, to prevent hacking
     def sanitize(name)
       name = name.tr("\\", "/") # work-around for IE
+      uri = try_uri(name)
+      name = uri.path if uri # only assign if parse was successful, otherwise treat as local
       name = File.basename(name)
       name = name.gsub(sanitize_regexp,"_")
       name = "_#{name}" if name =~ /\A\.+\z/
@@ -329,6 +329,18 @@ def sanitize(name)
       return name.mb_chars.to_s
     end
 
+    ##
+    # Returns URI if the String param can be parsed as URI, nil otherwise.  Swallows parse errors.
+    #
+    # === Returns
+    #
+    # [URI, nil] URI object
+    #
+    def try_uri(candidate)
+      URI.parse(candidate)
+    rescue URI::InvalidURIError
+    end
+
     def split_extension(filename)
       # regular expressions to try for identifying extensions
       extension_matchers = [
diff --git a/spec/sanitized_file_spec.rb b/spec/sanitized_file_spec.rb
index 324d914e4..0fa3aa476 100644
--- a/spec/sanitized_file_spec.rb
+++ b/spec/sanitized_file_spec.rb
@@ -103,7 +103,8 @@
   end
 
   describe "#filename" do
-    subject { described_class.new(args).filename }
+    let(:instance) {described_class.new(args) }
+    subject { instance.filename }
 
     describe 'should default to the original filename if it is valid' do
       let(:args) { 'llama.jpg' }
@@ -154,6 +155,14 @@
       let(:args) { 'ТестоВый Ёжик.jpg' }
       it { is_expected.to eq 'ТестоВый_Ёжик.jpg' }
     end
+
+    describe 'should handle ugly S3 URIs with slashes in params' do
+      let(:args) { "https://me.s3.amazonaws.com//var/app/current/tmp/uploads/myapp/uploaded_file/file/31/image.png?X-Amz-Expires=600\u0026X-Amz-Date=20161214T193306Z\u0026X-Amz-Security-Token=FQoDZXdqENT//////////wEaDMj3NLTbn4Z3JgbQtSK3B57LyrdXBHQlP6lM7cT/2N9naRgRSqf4FG/BxCCjMGcEVdt4X5ZsfHdzNiD6L0GODXmrR3quoXNBNZCtUVo3DY5E0P67iz9tYC2Ac%2BILJ%2BBzELNz84XI7C9zg6CCecZ8oeNjCTJXsMZ3xLx2bN099sl%2BY5nduDXAxen2Z63QKw7kiuuEXin/z%2B4ywFSP/Z1Sqbjkq4Qwjs5FUSyyz61wjl1%2Bg8uIJ5u3HTOlb8eZpk7gUCtdmLIE7mK1eZe5azUJC8XBW7Eu7jaRyM2PKMwjVnwepnfgPyEDqJSzKYJt1bGXgnQEN7logEKNOjmOcJqggM5Tc7PD40USAveIQ6E8ny/X0N%2BZ/X1rZTaCiAH1aWwVNqa0M43mlECrBeDv9I9BRMJzp4btvEgHKODrJe2MawDu4L1%2BzVNgOD7TZjrFt9zSEpyQK79dh8oHuyzDL0C%2Bpw3zL2ambsJ5OX6UnMuAmrkBbin1PKh2nHFkL/0xXAb2ZbSV6vKBxzKeQ62HMvv8UqypKbkwOMnstxyGGp00r6m6vL62x%2BTDergiiRfs947NyfJnP5l/rNRNMNesGo6kBmAqpACaBPAo0Z3GwgU%3D\u0026X-Amz-Algorithm=AWS4-HMAC-SHA256\u0026X-Amz-Credential=ASIAIB72YBSAAINUZRPQ/20161214/us-east-1/s3/aws4_request\u0026X-Amz-SignedHeaders=host\u0026X-Amz-Signature=f9bfb2a8d6114bccb6f77e4c0526bf19c5658b588ee368d04b42b18771d5359db" }
+      it 'is_path? is still true' do
+        expect(instance).to be_a_path # really *any* non-empty string gets a true from `is_path?`
+      end
+      it { is_expected.to eq 'image.png' }
+    end
   end
 
   describe "#filename with an overridden sanitize_regexp" do

From 1ad073884225954264b3bfe58297eaabf67c9a63 Mon Sep 17 00:00:00 2001
From: Joe Atzberger <ohiocore@gmail.com>
Date: Wed, 8 Feb 2017 14:47:28 -0800
Subject: [PATCH 3/6] Setter method `file=` is already defined and **private**

So having an initially public attr_writer for it is misleading.
---
 lib/carrierwave/sanitized_file.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/carrierwave/sanitized_file.rb b/lib/carrierwave/sanitized_file.rb
index f680d8f87..448cb6c91 100644
--- a/lib/carrierwave/sanitized_file.rb
+++ b/lib/carrierwave/sanitized_file.rb
@@ -19,8 +19,7 @@ module CarrierWave
   # It's probably needlessly comprehensive and complex. Help is appreciated.
   #
   class SanitizedFile
-
-    attr_accessor :file
+    attr_reader :file
 
     class << self
       attr_writer :sanitize_regexp

From d32528e6539c8f71e00537870897f4d0fdb79073 Mon Sep 17 00:00:00 2001
From: Joe Atzberger <ohiocore@gmail.com>
Date: Thu, 9 Feb 2017 16:31:09 -0800
Subject: [PATCH 4/6] Add many *nix-directory cleansing tests, mark pending

See: #2119
---
 spec/sanitized_file_spec.rb | 41 ++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/spec/sanitized_file_spec.rb b/spec/sanitized_file_spec.rb
index 0fa3aa476..5c8cbe06f 100644
--- a/spec/sanitized_file_spec.rb
+++ b/spec/sanitized_file_spec.rb
@@ -131,9 +131,44 @@
       it { is_expected.to eq 'foo.txt' }
     end
 
-    describe 'should make sure the *nix directory thingies cannot be used as filenames' do
-      let(:args) { '.' }
-      it { is_expected.to eq '_.' }
+    describe '*nix directories cannot be used as filename' do
+      describe 'current directory' do
+        let(:args) { '.' }
+        it 'reality check' do
+          pending 'Feature not fully implemented'
+          expect(instance).to be_a_path # really *any* non-empty string gets a true from `is_path?`
+          expect(instance.original_filename).to eq args
+        end
+        xit { is_expected.to eq '_.' }
+      end
+
+      describe 'redundant directory' do
+        let(:args) { '/path/.' }
+        it 'reality check' do
+          pending 'Feature not fully implemented'
+          expect(instance.path).to eq '/path'
+          expect(instance.original_filename).to eq '.' # or do we mean NOT-the-original filename?
+        end
+        xit { is_expected.to eq '_.' }
+      end
+
+      describe 'parent directory' do
+        let(:args) { '..' }
+        it 'reality check' do
+          pending 'Feature not fully implemented'
+          expect(instance.original_filename).to eq '..'
+        end
+        xit { is_expected.to eq '_..' }
+      end
+
+      describe 'compound directory' do
+        let(:args) { '../../.' }
+        it 'reality check' do
+          pending 'Feature not fully implemented'
+          expect(instance.original_filename).to eq '.'
+        end
+        xit { is_expected.to eq '_.' }
+      end
     end
 
     describe 'should maintain uppercase filenames' do

From 3ba83f9d1d015686ce1def9958555a7233fb1c3c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Janko=20Marohni=C4=87?= <janko.marohnic@gmail.com>
Date: Thu, 5 Apr 2018 13:20:53 +0200
Subject: [PATCH 5/6] Delegate MiniMagick processing to ImageProcessing gem

ImageProcessing gem is a wrapper gem around MiniMagick, which implements
common resizing macros. Instead of us having to maintain those macros,
we can use the ImageProcessing to do the heavy lifting for us.

We still maintain backwards compatibility by leaving the #manipulate!
method around, and yielding the MiniMagick::Image object in processing
methods (even though that should be considered deprecated).
---
 README.md                                 |   8 +-
 carrierwave.gemspec                       |   1 +
 lib/carrierwave/processing/mini_magick.rb | 218 +++++++++++-----------
 spec/processing/mini_magick_spec.rb       |  20 +-
 4 files changed, 122 insertions(+), 125 deletions(-)

diff --git a/README.md b/README.md
index 078c55762..388918ee5 100644
--- a/README.md
+++ b/README.md
@@ -277,7 +277,7 @@ You no longer need to do this manually.
 
 Often you'll want to add different versions of the same file. The classic example is image thumbnails. There is built in support for this*:
 
-*Note:* You must have Imagemagick and MiniMagick installed to do image resizing. MiniMagick is a Ruby interface for Imagemagick which is a C program. This is why MiniMagick fails on 'bundle install' without Imagemagick installed.
+*Note:* You must have Imagemagick installed to do image resizing.
 
 Some documentation refers to RMagick instead of MiniMagick but MiniMagick is recommended.
 
@@ -359,7 +359,7 @@ private
   end
 
   def is_landscape? picture
-    image = MiniMagick::Image.open(picture.path)
+    image = MiniMagick::Image.new(picture.path)
     image[:width] > image[:height]
   end
 
@@ -865,8 +865,8 @@ manipulation methods.
 
 ## Using MiniMagick
 
-MiniMagick is similar to RMagick but performs all the operations using the 'mogrify'
-command which is part of the standard ImageMagick kit. This allows you to have the power
+MiniMagick is similar to RMagick but performs all the operations using the 'convert'
+CLI which is part of the standard ImageMagick kit. This allows you to have the power
 of ImageMagick without having to worry about installing all the RMagick libraries.
 
 See the MiniMagick site for more details:
diff --git a/carrierwave.gemspec b/carrierwave.gemspec
index 09e3c6dc3..048506a9b 100644
--- a/carrierwave.gemspec
+++ b/carrierwave.gemspec
@@ -24,6 +24,7 @@ Gem::Specification.new do |s|
   s.add_dependency "activesupport", ">= 4.0.0"
   s.add_dependency "activemodel", ">= 4.0.0"
   s.add_dependency "mime-types", ">= 1.16"
+  s.add_dependency "image_processing", "~> 1.0"
   if RUBY_ENGINE == 'jruby'
     s.add_development_dependency 'activerecord-jdbcpostgresql-adapter'
   else
diff --git a/lib/carrierwave/processing/mini_magick.rb b/lib/carrierwave/processing/mini_magick.rb
index 8c51cb4e5..9a10528c5 100644
--- a/lib/carrierwave/processing/mini_magick.rb
+++ b/lib/carrierwave/processing/mini_magick.rb
@@ -21,9 +21,11 @@ module CarrierWave
   #       process :resize_to_fit => [200, 200]
   #     end
   #
-  # Or create your own helpers with the powerful manipulate! method. Check
-  # out the ImageMagick docs at http://www.imagemagick.org/script/command-line-options.php for more
-  # info
+  # Or create your own helpers with the powerful minimagick! method, which
+  # yields an ImageProcessing::Builder object. Check out the ImageProcessing
+  # docs at http://github.com/janko-m/image_processing and the list of all
+  # available ImageMagick options at
+  # http://www.imagemagick.org/script/command-line-options.php for more info.
   #
   #     class MyUploader < CarrierWave::Uploader::Base
   #       include CarrierWave::MiniMagick
@@ -31,23 +33,22 @@ module CarrierWave
   #       process :radial_blur => 10
   #
   #       def radial_blur(amount)
-  #         manipulate! do |img|
-  #           img.radial_blur(amount)
-  #           img = yield(img) if block_given?
-  #           img
+  #         minimagick! do |builder|
+  #           builder.radial_blur(amount)
+  #           builder = yield(builder) if block_given?
+  #           builder
   #         end
   #       end
   #     end
   #
   # === Note
   #
-  # MiniMagick is a mini replacement for RMagick that uses the command line
-  # tool "mogrify" for image manipulation.
+  # The ImageProcessing gem uses MiniMagick, a mini replacement for RMagick
+  # that uses ImageMagick command-line tools, to build a "convert" command that
+  # performs the processing.
   #
   # You can find more information here:
   #
-  # http://mini_magick.rubyforge.org/
-  # and
   # https://github.com/minimagick/minimagick/
   #
   #
@@ -55,19 +56,7 @@ module MiniMagick
     extend ActiveSupport::Concern
 
     included do
-      begin
-        require "mini_magick"
-      rescue LoadError => e
-        e.message << " (You may need to install the mini_magick gem)"
-        raise e
-      end
-
-      prepend Module.new {
-        def initialize(*)
-          super
-          @format = nil
-        end
-      }
+      require "image_processing/mini_magick"
     end
 
     module ClassMethods
@@ -109,12 +98,11 @@ def resize_and_pad(width, height, background=:transparent, gravity='Center')
     #
     #     image.convert(:png)
     #
-    def convert(format, page=nil)
-      @format = format
-      @page = page
-      manipulate! do |img|
-        img = yield(img) if block_given?
-        img
+    def convert(format, page=nil, &block)
+      minimagick!(block) do |builder|
+        builder = builder.convert(format)
+        builder = builder.loader(page: page) if page
+        builder
       end
     end
 
@@ -128,21 +116,17 @@ def convert(format, page=nil)
     #
     # [width (Integer)] the width to scale the image to
     # [height (Integer)] the height to scale the image to
+    # [combine_options (Hash)] additional ImageMagick options to apply before resizing
     #
     # === Yields
     #
     # [MiniMagick::Image] additional manipulations to perform
     #
-    def resize_to_limit(width, height, combine_options: {})
-      width = dimension_from width
-      height = dimension_from height
-      manipulate! do |img|
-        img.combine_options do |cmd|
-          cmd.resize "#{width}x#{height}>"
-          append_combine_options cmd, combine_options
-        end
-        img = yield(img) if block_given?
-        img
+    def resize_to_limit(width, height, combine_options: {}, &block)
+      width, height = resolve_dimensions(width, height)
+
+      minimagick!(block, combine_options) do |builder|
+        builder.resize_to_limit(width, height)
       end
     end
 
@@ -155,21 +139,17 @@ def resize_to_limit(width, height, combine_options: {})
     #
     # [width (Integer)] the width to scale the image to
     # [height (Integer)] the height to scale the image to
+    # [combine_options (Hash)] additional ImageMagick options to apply before resizing
     #
     # === Yields
     #
     # [MiniMagick::Image] additional manipulations to perform
     #
-    def resize_to_fit(width, height, combine_options: {})
-      width = dimension_from width
-      height = dimension_from height
-      manipulate! do |img|
-        img.combine_options do |cmd|
-          cmd.resize "#{width}x#{height}"
-          append_combine_options cmd, combine_options
-        end
-        img = yield(img) if block_given?
-        img
+    def resize_to_fit(width, height, combine_options: {}, &block)
+      width, height = resolve_dimensions(width, height)
+
+      minimagick!(block, combine_options) do |builder|
+        builder.resize_to_fit(width, height)
       end
     end
 
@@ -183,37 +163,17 @@ def resize_to_fit(width, height, combine_options: {})
     # [width (Integer)] the width to scale the image to
     # [height (Integer)] the height to scale the image to
     # [gravity (String)] the current gravity suggestion (default: 'Center'; options: 'NorthWest', 'North', 'NorthEast', 'West', 'Center', 'East', 'SouthWest', 'South', 'SouthEast')
+    # [combine_options (Hash)] additional ImageMagick options to apply before resizing
     #
     # === Yields
     #
     # [MiniMagick::Image] additional manipulations to perform
     #
-    def resize_to_fill(width, height, gravity = 'Center', combine_options: {})
-      width = dimension_from width
-      height = dimension_from height
-      manipulate! do |img|
-        cols, rows = img[:dimensions]
-        img.combine_options do |cmd|
-          if width != cols || height != rows
-            scale_x = width/cols.to_f
-            scale_y = height/rows.to_f
-            if scale_x >= scale_y
-              cols = (scale_x * (cols + 0.5)).round
-              rows = (scale_x * (rows + 0.5)).round
-              cmd.resize "#{cols}"
-            else
-              cols = (scale_y * (cols + 0.5)).round
-              rows = (scale_y * (rows + 0.5)).round
-              cmd.resize "x#{rows}"
-            end
-          end
-          cmd.gravity gravity
-          cmd.background "rgba(255,255,255,0.0)"
-          cmd.extent "#{width}x#{height}" if cols != width || rows != height
-          append_combine_options cmd, combine_options
-        end
-        img = yield(img) if block_given?
-        img
+    def resize_to_fill(width, height, gravity = 'Center', combine_options: {}, &block)
+      width, height = resolve_dimensions(width, height)
+
+      minimagick!(block, combine_options) do |builder|
+        builder.resize_to_fill(width, height, gravity: gravity)
       end
     end
 
@@ -232,28 +192,17 @@ def resize_to_fill(width, height, gravity = 'Center', combine_options: {})
     # [height (Integer)] the height to scale the image to
     # [background (String, :transparent)] the color of the background as a hexcode, like "#ff45de"
     # [gravity (String)] how to position the image
+    # [combine_options (Hash)] additional ImageMagick options to apply before resizing
     #
     # === Yields
     #
     # [MiniMagick::Image] additional manipulations to perform
     #
-    def resize_and_pad(width, height, background=:transparent, gravity='Center', combine_options: {})
-      width = dimension_from width
-      height = dimension_from height
-      manipulate! do |img|
-        img.combine_options do |cmd|
-          cmd.thumbnail "#{width}x#{height}>"
-          if background == :transparent
-            cmd.background "rgba(255, 255, 255, 0.0)"
-          else
-            cmd.background background
-          end
-          cmd.gravity gravity
-          cmd.extent "#{width}x#{height}"
-          append_combine_options cmd, combine_options
-        end
-        img = yield(img) if block_given?
-        img
+    def resize_and_pad(width, height, background=:transparent, gravity='Center', combine_options: {}, &block)
+      width, height = resolve_dimensions(width, height)
+
+      minimagick!(block, combine_options) do |builder|
+        builder.resize_and_pad(width, height, background: background, gravity: gravity)
       end
     end
 
@@ -284,6 +233,9 @@ def height
     # and then pass each of its frames to the supplied block. It will then
     # save the image to disk.
     #
+    # NOTE: This method exists mostly for backwards compatibility, you should
+    # probably use #minimagick!.
+    #
     # === Gotcha
     #
     # This method assumes that the object responds to +current_path+.
@@ -303,20 +255,56 @@ def manipulate!
       cache_stored_file! if !cached?
       image = ::MiniMagick::Image.open(current_path)
 
-      begin
-        image.format(@format.to_s.downcase, @page) if @format
-        image = yield(image)
-        image.write(current_path)
+      image = yield(image)
+      FileUtils.mv image.path, current_path
 
-        if @format
-          move_to = current_path.chomp(File.extname(current_path)) + ".#{@format}"
-          file.content_type = ::MIME::Types.type_for(move_to).first.to_s
-          file.move_to(move_to, permissions, directory_permissions)
-        end
+      image.run_command("identify", current_path)
+    rescue ::MiniMagick::Error, ::MiniMagick::Invalid => e
+      message = I18n.translate(:"errors.messages.mini_magick_processing_error", :e => e)
+      raise CarrierWave::ProcessingError, message
+    ensure
+      image.destroy! if image
+    end
 
-        image.run_command("identify", current_path)
-      ensure
-        image.destroy!
+    # Process the image with MiniMagick, using the ImageProcessing gem. This
+    # method will build a "convert" ImageMagick command and execute it on the
+    # current image.
+    #
+    # === Gotcha
+    #
+    # This method assumes that the object responds to +current_path+.
+    # Any class that this module is mixed into must have a +current_path+ method.
+    # CarrierWave::Uploader does, so you won't need to worry about this in
+    # most cases.
+    #
+    # === Yields
+    #
+    # [ImageProcessing::Builder] use it to define processing to be performed
+    #
+    # === Raises
+    #
+    # [CarrierWave::ProcessingError] if processing failed.
+    def minimagick!(block = nil, **combine_options)
+      builder = ImageProcessing::MiniMagick.source(current_path)
+      builder = yield(builder)
+      builder = append_combine_options(builder, combine_options)
+
+      result = builder.call
+      result.close
+
+      # backwards compatibility (we want to eventually move away from MiniMagick::Image)
+      if block
+        image  = MiniMagick::Image.new(result.path, result)
+        image  = block.call(image)
+        result = image.instance_variable_get(:@tempfile)
+      end
+
+      FileUtils.mv result.path, current_path
+
+      if File.extname(result.path) != File.extname(current_path)
+        move_to = current_path.chomp(File.extname(current_path)) + File.extname(result.path)
+        file.content_type = ::MIME::Types.type_for(move_to).first.to_s
+        file.move_to(move_to, permissions, directory_permissions)
       end
     rescue ::MiniMagick::Error, ::MiniMagick::Invalid => e
       message = I18n.translate(:"errors.messages.mini_magick_processing_error", :e => e)
@@ -325,26 +313,28 @@ def manipulate!
 
     private
 
-      def append_combine_options(cmd, combine_options)
-        combine_options.each do |method, options|
-          if options.nil?
-            cmd.send(method)
+      def append_combine_options(builder, combine_options)
+        combine_options.inject(builder) do |builder, (option, value)|
+          if value.nil?
+            builder.send(option)
           else
-            cmd.send(method, options)
+            builder.send(option, *value)
           end
         end
       end
 
-      def dimension_from(value)
-        return value unless value.instance_of?(Proc)
-        value.arity >= 1 ? value.call(self) : value.call
+      def resolve_dimensions(*dimensions)
+        dimensions.map do |value|
+          next value unless value.instance_of?(Proc)
+          value.arity >= 1 ? value.call(self) : value.call
+        end
       end
 
       def mini_magick_image
         if url
           ::MiniMagick::Image.open(url)
         else
-          ::MiniMagick::Image.open(current_path)
+          ::MiniMagick::Image.new(current_path)
         end
       end
 
diff --git a/spec/processing/mini_magick_spec.rb b/spec/processing/mini_magick_spec.rb
index 87486bbea..d4a994217 100644
--- a/spec/processing/mini_magick_spec.rb
+++ b/spec/processing/mini_magick_spec.rb
@@ -24,14 +24,20 @@
       expect(instance.file.content_type).to eq('image/png')
     end
 
-    it "converts all pages when no page number is specified" do
-      expect_any_instance_of(::MiniMagick::Image).to receive(:format).with('png', nil).once
-      instance.convert('png')
-    end
+    it "respects the page parameter" do
+      # create a multi-layer image
+      tiff = Tempfile.new(["file", ".tiff"])
+      MiniMagick::Tool::Convert.new do |convert|
+        convert.merge! [landscape_file_path, landscape_file_path, landscape_file_path]
+        convert << tiff.path
+      end
 
-    it "converts specific page" do
-      expect_any_instance_of(::MiniMagick::Image).to receive(:format).with('png', 1).once
-      instance.convert('png', 1)
+      allow(instance).to receive(:file).and_return(CarrierWave::SanitizedFile.new(tiff.path))
+
+      instance.convert('png', 0)
+      expect(instance.file.extension).to eq('png')
+      expect(instance).to be_format('png')
+      expect(instance.file.size).not_to eq(0)
     end
   end
 

From 4a64df3417f7c58c669e8734bfb031282279cd73 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Janko=20Marohni=C4=87?= <janko.marohnic@gmail.com>
Date: Thu, 5 Apr 2018 19:23:57 +0200
Subject: [PATCH 6/6] Use ImageProcessing::Builder#apply

---
 carrierwave.gemspec                       |  2 +-
 lib/carrierwave/processing/mini_magick.rb | 25 ++++++++---------------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/carrierwave.gemspec b/carrierwave.gemspec
index 048506a9b..99aee9bd7 100644
--- a/carrierwave.gemspec
+++ b/carrierwave.gemspec
@@ -24,7 +24,7 @@ Gem::Specification.new do |s|
   s.add_dependency "activesupport", ">= 4.0.0"
   s.add_dependency "activemodel", ">= 4.0.0"
   s.add_dependency "mime-types", ">= 1.16"
-  s.add_dependency "image_processing", "~> 1.0"
+  s.add_dependency "image_processing", "~> 1.1"
   if RUBY_ENGINE == 'jruby'
     s.add_development_dependency 'activerecord-jdbcpostgresql-adapter'
   else
diff --git a/lib/carrierwave/processing/mini_magick.rb b/lib/carrierwave/processing/mini_magick.rb
index 9a10528c5..16130622f 100644
--- a/lib/carrierwave/processing/mini_magick.rb
+++ b/lib/carrierwave/processing/mini_magick.rb
@@ -125,8 +125,9 @@ def convert(format, page=nil, &block)
     def resize_to_limit(width, height, combine_options: {}, &block)
       width, height = resolve_dimensions(width, height)
 
-      minimagick!(block, combine_options) do |builder|
+      minimagick!(block) do |builder|
         builder.resize_to_limit(width, height)
+          .apply(combine_options)
       end
     end
 
@@ -148,8 +149,9 @@ def resize_to_limit(width, height, combine_options: {}, &block)
     def resize_to_fit(width, height, combine_options: {}, &block)
       width, height = resolve_dimensions(width, height)
 
-      minimagick!(block, combine_options) do |builder|
+      minimagick!(block) do |builder|
         builder.resize_to_fit(width, height)
+          .apply(combine_options)
       end
     end
 
@@ -172,8 +174,9 @@ def resize_to_fit(width, height, combine_options: {}, &block)
     def resize_to_fill(width, height, gravity = 'Center', combine_options: {}, &block)
       width, height = resolve_dimensions(width, height)
 
-      minimagick!(block, combine_options) do |builder|
+      minimagick!(block) do |builder|
         builder.resize_to_fill(width, height, gravity: gravity)
+          .apply(combine_options)
       end
     end
 
@@ -201,8 +204,9 @@ def resize_to_fill(width, height, gravity = 'Center', combine_options: {}, &bloc
     def resize_and_pad(width, height, background=:transparent, gravity='Center', combine_options: {}, &block)
       width, height = resolve_dimensions(width, height)
 
-      minimagick!(block, combine_options) do |builder|
+      minimagick!(block) do |builder|
         builder.resize_and_pad(width, height, background: background, gravity: gravity)
+          .apply(combine_options)
       end
     end
 
@@ -284,10 +288,9 @@ def manipulate!
     # === Raises
     #
     # [CarrierWave::ProcessingError] if processing failed.
-    def minimagick!(block = nil, **combine_options)
+    def minimagick!(block = nil)
       builder = ImageProcessing::MiniMagick.source(current_path)
       builder = yield(builder)
-      builder = append_combine_options(builder, combine_options)
 
       result = builder.call
       result.close
@@ -313,16 +316,6 @@ def minimagick!(block = nil, **combine_options)
 
     private
 
-      def append_combine_options(builder, combine_options)
-        combine_options.inject(builder) do |builder, (option, value)|
-          if value.nil?
-            builder.send(option)
-          else
-            builder.send(option, *value)
-          end
-        end
-      end
-
       def resolve_dimensions(*dimensions)
         dimensions.map do |value|
           next value unless value.instance_of?(Proc)