From 137e06a316b86a560a1c03adacde609dfdf7340c Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 19 Apr 2022 11:21:24 +0100
Subject: [PATCH] FIX: Ensure 'crop' always returns requested dimensions
 (#16437)

Previously, 'crop' would resize the image to have the requested width, then crop the height to the requested value. This works when cropping images vertically, but not when cropping them horizontally.

For example, trying to crop a 500x500 image to 200x500 was actually resulting in a 200x200 image. Having an OptimizedImage with width/height columns mismatching the actual OptimizedImage width/height causes some unusual issues.

This commit ensures that a call to `OptimizedImage.crop(from, to, width, height)` will always return an image of the requested width/height. The `w x h^` syntax defines minimum width/height, while maintaining aspect ratio.
---
 app/models/optimized_image.rb       |  3 +-
 spec/models/optimized_image_spec.rb | 61 +++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index 041b9604c43..901e762328f 100644
--- a/app/models/optimized_image.rb
+++ b/app/models/optimized_image.rb
@@ -252,7 +252,7 @@ class OptimizedImage < ActiveRecord::Base
       -auto-orient
       -gravity north
       -background transparent
-      -#{thumbnail_or_resize} #{opts[:width]}
+      -#{thumbnail_or_resize} #{dimensions}^
       -crop #{dimensions}+0+0
       -unsharp 2x0.5+0.7+0
       -interlace none
@@ -290,7 +290,6 @@ class OptimizedImage < ActiveRecord::Base
   end
 
   def self.crop(from, to, width, height, opts = {})
-    opts[:width] = width
     optimize("crop", from, to, "#{width}x#{height}", opts)
   end
 
diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb
index 700474ff13f..c008ca6d8a3 100644
--- a/spec/models/optimized_image_spec.rb
+++ b/spec/models/optimized_image_spec.rb
@@ -7,22 +7,23 @@ describe OptimizedImage do
   describe '.crop' do
     it 'should produce cropped images (requires ImageMagick 7)' do
       tmp_path = "/tmp/cropped.png"
+      desired_width = 5
+      desired_height = 5
 
       begin
         OptimizedImage.crop(
           "#{Rails.root}/spec/fixtures/images/logo.png",
           tmp_path,
-          5,
-          5
+          desired_width,
+          desired_height
         )
 
-        # we don't want to deal with something new here every time image magick
-        # is upgraded or pngquant is upgraded, lets just test the basics ...
-        # cropped image should be less than 120 bytes
+        w, h = FastImage.size(tmp_path)
+        expect(w).to eq(desired_width)
+        expect(h).to eq(desired_height)
 
         cropped_size = File.size(tmp_path)
-
-        expect(cropped_size).to be < 120
+        expect(cropped_size).to be < 200
         expect(cropped_size).to be > 50
 
       ensure
@@ -30,6 +31,52 @@ describe OptimizedImage do
       end
     end
 
+    it 'should correctly crop images vertically' do
+      tmp_path = "/tmp/cropped.png"
+      desired_width = 100
+      desired_height = 66
+
+      begin
+        OptimizedImage.crop(
+          "#{Rails.root}/spec/fixtures/images/logo.png", # 244x66px
+          tmp_path,
+          desired_width,
+          desired_height
+        )
+
+        w, h = FastImage.size(tmp_path)
+
+        expect(w).to eq(desired_width)
+        expect(h).to eq(desired_height)
+
+      ensure
+        File.delete(tmp_path) if File.exist?(tmp_path)
+      end
+    end
+
+    it 'should correctly crop images horizontally' do
+      tmp_path = "/tmp/cropped.png"
+      desired_width = 244
+      desired_height = 500
+
+      begin
+        OptimizedImage.crop(
+          "#{Rails.root}/spec/fixtures/images/logo.png", # 244x66px
+          tmp_path,
+          desired_width,
+          desired_height
+        )
+
+        w, h = FastImage.size(tmp_path)
+
+        expect(w).to eq(desired_width)
+        expect(h).to eq(desired_height)
+
+      ensure
+        File.delete(tmp_path) if File.exist?(tmp_path)
+      end
+    end
+
     describe ".resize_instructions" do
       let(:image) { "#{Rails.root}/spec/fixtures/images/logo.png" }