Issue #1338428 by jbrown, BTMash, Tor Arne Thune: Fixed image_dimensions_scale() and image_scale_effect() are ungrokable and buggy.

3 files changed, 161 insertions, 28 deletions

diff --git a/includes/image.inc b/includes/image.inc
index 8dc36b995..f6ae7f19b 100644
--- a/includes/image.inc
+++ b/includes/image.inc
@@ -186,14 +186,14 @@ function image_scale_and_crop(stdClass $image, $width, $height) {
  *   Dimensions to be modified - an array with components width and height, in
  *   pixels.
  * @param $width
- *   The target width, in pixels. This value is omitted then the scaling will
+ *   The target width, in pixels. If this value is NULL then the scaling will be
  *   based only on the height value.
  * @param $height
- *   The target height, in pixels. This value is omitted then the scaling will
- *   based only on the width value.
+ *   The target height, in pixels. If this value is NULL then the scaling will
+ *   be based only on the width value.
  * @param $upscale
- *   Boolean indicating that files smaller than the dimensions will be scaled
- *   up. This generally results in a low quality image.
+ *   Boolean indicating that images smaller than the target dimensions will be
+ *   scaled up. This generally results in a low quality image.
  *
  * @return
  *   TRUE if $dimensions was modified, FALSE otherwise.
@@ -203,31 +203,25 @@ function image_scale_and_crop(stdClass $image, $width, $height) {
 function image_dimensions_scale(array &$dimensions, $width = NULL, $height = NULL, $upscale = FALSE) {
   $aspect = $dimensions['height'] / $dimensions['width'];
 
-  if ($upscale) {
-    // Set width/height according to aspect ratio if either is empty.
-    $width = !empty($width) ? $width : $height / $aspect;
-    $height = !empty($height) ? $height : $width / $aspect;
+  // Calculate one of the dimensions from the other target dimension,
+  // ensuring the same aspect ratio as the source dimensions. If one of the
+  // target dimensions is missing, that is the one that is calculated. If both
+  // are specified then the dimension calculated is the one that would not be
+  // calculated to be bigger than its target.
+  if (($width && !$height) || ($width && $height && $aspect < $height / $width)) {
+    $height = (int) round($width * $aspect);
   }
   else {
-    // Set impossibly large values if the width and height aren't set.
-    $width = !empty($width) ? $width : 9999999;
-    $height = !empty($height) ? $height : 9999999;
-
-    // Don't scale up.
-    if (round($width) >= $dimensions['width'] && round($height) >= $dimensions['height']) {
-      return FALSE;
-    }
+    $width = (int) round($height / $aspect);
   }
 
-  if ($aspect < $height / $width) {
-    $dimensions['width'] = $width;
-    $dimensions['height'] = (int) round($width * $aspect);
-  }
-  else {
-    $dimensions['width'] = (int) round($height / $aspect);
-    $dimensions['height'] = $height;
+  // Don't upscale if the option isn't enabled.
+  if (!$upscale && ($width >= $dimensions['width'] || $height >= $dimensions['height'])) {
+    return FALSE;
   }
 
+  $dimensions['width'] = $width;
+  $dimensions['height'] = $height;
   return TRUE;
 }
 
diff --git a/modules/image/image.effects.inc b/modules/image/image.effects.inc
index ea898f91f..35a6a74c7 100644
--- a/modules/image/image.effects.inc
+++ b/modules/image/image.effects.inc
@@ -124,13 +124,11 @@ function image_resize_dimensions(array &$dimensions, array $data) {
 function image_scale_effect(&$image, $data) {
   // Set sane default values.
   $data += array(
+    'width' => NULL,
+    'height' => NULL,
     'upscale' => FALSE,
   );
 
-  // Set impossibly large values if the width and height aren't set.
-  $data['width'] = empty($data['width']) ? PHP_INT_MAX : $data['width'];
-  $data['height'] = empty($data['height']) ? PHP_INT_MAX : $data['height'];
-
   if (!image_scale($image, $data['width'], $data['height'], $data['upscale'])) {
     watchdog('image', 'Image scale failed using the %toolkit toolkit on %path (%mimetype, %dimensions)', array('%toolkit' => $image->toolkit, '%path' => $image->source, '%mimetype' => $image->info['mime_type'], '%dimensions' => $image->info['width'] . 'x' . $image->info['height']), WATCHDOG_ERROR);
     return FALSE;
diff --git a/modules/image/image.test b/modules/image/image.test
index a29b4f3a1..c9a709033 100644
--- a/modules/image/image.test
+++ b/modules/image/image.test
@@ -1129,3 +1129,144 @@ class ImageDimensionsUnitTest extends DrupalWebTestCase {
     $this->assertEqual($img_tag, '<img typeof="foaf:Image" src="' . $url . '" alt="" />', t('Expected img tag was found.'));
   }
 }
+
+/**
+ * Tests image_dimensions_scale().
+ */
+class ImageDimensionsScaleTestCase extends DrupalUnitTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'image_dimensions_scale()',
+      'description' => 'Tests all control flow branches in image_dimensions_scale().',
+      'group' => 'Image',
+    );
+  }
+
+  /**
+   * Tests all control flow branches in image_dimensions_scale().
+   */
+  function testImageDimensionsScale() {
+    // Define input / output datasets to test different branch conditions.
+    $test = array();
+
+    // Test branch conditions:
+    // - No height.
+    // - Upscale, don't need to upscale.
+    $tests[] = array(
+      'input' => array(
+        'dimensions' => array(
+          'width' => 1000,
+          'height' => 2000,
+        ),
+        'width' => 200,
+        'height' => NULL,
+        'upscale' => TRUE,
+      ),
+      'output' => array(
+        'dimensions' => array(
+          'width' => 200,
+          'height' => 400,
+        ),
+        'return_value' => TRUE,
+      ),
+    );
+
+    // Test branch conditions:
+    // - No width.
+    // - Don't upscale, don't need to upscale.
+    $tests[] = array(
+      'input' => array(
+        'dimensions' => array(
+          'width' => 1000,
+          'height' => 800,
+        ),
+        'width' => NULL,
+        'height' => 140,
+        'upscale' => FALSE,
+      ),
+      'output' => array(
+        'dimensions' => array(
+          'width' => 175,
+          'height' => 140,
+        ),
+        'return_value' => TRUE,
+      ),
+    );
+
+    // Test branch conditions:
+    // - Source aspect ratio greater than target.
+    // - Upscale, need to upscale.
+    $tests[] = array(
+      'input' => array(
+        'dimensions' => array(
+          'width' => 8,
+          'height' => 20,
+        ),
+        'width' => 200,
+        'height' => 140,
+        'upscale' => TRUE,
+      ),
+      'output' => array(
+        'dimensions' => array(
+          'width' => 56,
+          'height' => 140,
+        ),
+        'return_value' => TRUE,
+      ),
+    );
+
+    // Test branch condition: target aspect ratio greater than source.
+    $tests[] = array(
+      'input' => array(
+        'dimensions' => array(
+          'width' => 2000,
+          'height' => 800,
+        ),
+        'width' => 200,
+        'height' => 140,
+        'upscale' => FALSE,
+      ),
+      'output' => array(
+        'dimensions' => array(
+          'width' => 200,
+          'height' => 80,
+        ),
+        'return_value' => TRUE,
+      ),
+    );
+
+    // Test branch condition: don't upscale, need to upscale.
+    $tests[] = array(
+      'input' => array(
+        'dimensions' => array(
+          'width' => 100,
+          'height' => 50,
+        ),
+        'width' => 200,
+        'height' => 140,
+        'upscale' => FALSE,
+      ),
+      'output' => array(
+        'dimensions' => array(
+          'width' => 100,
+          'height' => 50,
+        ),
+        'return_value' => FALSE,
+      ),
+    );
+
+    foreach ($tests as $test) {
+      // Process the test dataset.
+      $return_value = image_dimensions_scale($test['input']['dimensions'], $test['input']['width'], $test['input']['height'], $test['input']['upscale']);
+
+      // Check the width.
+      $this->assertEqual($test['output']['dimensions']['width'], $test['input']['dimensions']['width'], t('Computed width (@computed_width) equals expected width (@expected_width)', array('@computed_width' => $test['output']['dimensions']['width'], '@expected_width' => $test['input']['dimensions']['width'])));
+
+      // Check the height.
+      $this->assertEqual($test['output']['dimensions']['height'], $test['input']['dimensions']['height'], t('Computed height (@computed_height) equals expected height (@expected_height)', array('@computed_height' => $test['output']['dimensions']['height'], '@expected_height' => $test['input']['dimensions']['height'])));
+
+      // Check the return value.
+      $this->assertEqual($test['output']['return_value'], $return_value, t('Correct return value.'));
+    }
+  }
+}