image_dimensions_scale() and image_scale_effect() are ungrokable and buggy

Confirmed, and the patch seems working very fine.

What about like this?

From looking at the code it seems to be a total coincidence that image_dimensions_scale() and image_scale_effect() ever produce the right values. It's actually quite difficult to follow what is supposed to be happening.

Under certain circumstances it generates the wrong values. Returned values can be different depending on whether it is for an actual image transformation, or just the dimensions. This is because image_scale_effect() is stuffing missing dimensions with PHP_INT_MAX.

The patch in #1 makes things worse by adding even more code. I am unable to determine if that patch really fixes the problem in all circumstances.

My patch in #4 refactors these functions. image_dimensions_scale() is now very simple and it is very easy to determine how it works.

I have attached the D7 version of the patch also.

Thanks BTMash!

The testing system ignored the patch in #6 because this issue is currently for Drupal 8 and the filename of the patch ends in _d7.patch

My D7 patch in #5 was just a re-roll of my D8 patch in #4, so I think this latest re-rolling was unnecessary.

There was a duplicate issue: #1324076: "Allow upscaling" on image style results in incorrect image dimensions

We have steps to reproduce, so we have at least one test case to add, and hopefully more. :)

Also, it would be good to have some more detailed inline comments.

Sorry, wrong status before.

The source dimensions can be:

• wide
• tall
• square

The target dimensions can be:

• both - wide
• both - tall
• both - square
• just width
• just height

Scale up can be:

• on
• off

So there are 30 combinations that need to be tested.

Yeah - I agree.

I think it is better just to test control flow branches individually rather than every possible unique route through the function. There's really only about 6 branches. I'll post a patch soon.

Okay I converted it to a unit test because it is a pure function that we are testing.

There are really 8 branch conditions to test:

• no height
• no width
• keep width
• keep height
• don't scale up, don't need to scale up
• don't scale up, need to scale up
• scale up, don't need to scale up
• scale up, need to scale up

but I was able to test all of these with 5 input / output datasets.

I also improved the comments.

Updated patch. The following 8 branch conditions are tested in 5 input / output datasets:

• no height
• no width
• source aspect ratio greater than target
• target aspect ratio greater than source
• upscale, don't need to upscale
• don't upscale, don't need to upscale
• upscale, need to upscale
• don't upscale, need to upscale

The first dataset also tests the exact conditions that trigger the bug originally reported in this issue:

• height missing
• upscale
• source height greater than source width

Handy tip I learned recently: When you upload your tests-only and combined patches in a single comment, attach the tests-only patch first and the combined patch second. That way testbot leaves it at Needs Review. :)

Great!

I re-rolled it for D7.

This is looking good, and it now has tests. Yay! The new inline comments are also very helpful. Here's a review for the latest tests:

1. +++ b/core/modules/image/image.testundefined
   @@ -1129,3 +1129,133 @@ class ImageDimensionsUnitTest extends DrupalWebTestCase {
   +  /**
   +   * Tests all control flow branches in image_dimensions_scale().
   +   */
   +  function testImageDimensionsScale() {
   

   Let's initialize the $tests array at the top of the function and add a comment that explains what these are. Something like:

   function testImageDimensionsScale() {
   // Define several test scenarios for each possible (something-or-other?).
   $tests = array();
   

2. +++ b/core/modules/image/image.testundefined
   @@ -1129,3 +1129,133 @@ class ImageDimensionsUnitTest extends DrupalWebTestCase {
   +    // no height
   +    // upscale, don't need to upscale
   ...
   +    // no width
   +    // don't upscale, don't need to upscale
   ...
   +    // source aspect ratio greater than target
   +    // upscale, need to upscale
   ...
   +    // target aspect ratio greater than source
   ...
   +    // don't upscale, need to upscale
   

   Let's flesh these comments out a little more to be a bit more clear out of context. Also, they should be capitalized and have periods. E.g., maybe something like:

   // Test an image with:
   // - Height specified.
   // - Upscale enabled.
   // - Upscale not required.
   

   Or something along those lines. What do you think?

3. +++ b/core/modules/image/image.testundefined
   @@ -1129,3 +1129,133 @@ class ImageDimensionsUnitTest extends DrupalWebTestCase {
   +    foreach ($tests as $test) {
   +      $return_value = image_dimensions_scale($test['input']['dimensions'], $test['input']['width'], $test['input']['height'], $test['input']['upscale']);
   +
   +      $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'])));
   +      $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'])));
   +      $this->assertEqual($test['output']['return_value'], $return_value, t('Correct return value.'));
   +    }
   +  }
   +}
   

   This block of code is a bit hard to scan. Let's add some inline comments? E.g., something like:

   // Process the test case.   
   $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.'));
   

Codewise, I think these tests provide sufficient test coverage. Awesome work, @BTMash and @jbrown.

Alright, this looks ready to me.

#30 worked for me in D7!

Sorry - fixed typos in the comments in the test: "branches" => "branch".

Ah, yep. I re-read the fixed patch and it's still good to go.

+++ b/core/includes/image.inc
@@ -186,14 +186,14 @@ function image_scale_and_crop(stdClass $image, $width, $height) {
- *   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
  *   based only on the height value.
  * @param $height
- *   The target height, in pixels. This value is omitted then the scaling will
+ *   The target height, in pixels. If this value is NULL then the scaling will
  *   based only on the width value.

Missing 2 "be"s here.

+++ b/core/includes/image.inc
@@ -203,31 +203,25 @@ function image_scale_and_crop(stdClass $image, $width, $height) {
+  // calculated to be bigger than it's target.

it's -> its

Re-rolling with changes :)

I'm starting to look a little foolish. :) Perhaps someone else should RTBC this.

Looks good!

Alright. :)

Looks great, nice set of tests as well.

Committed/pushed to 8.x, will need a re-roll for 7.x.

Thanks catch!

Backport looks fine as well.

Patch in #42 fixes the issue for me thanks.

I'm confirm that the patch #42 works as expected

Awesome, committed and pushed to 7.x. Thanks!

After updating to latest 7.x dev the images I've previously uploaded and that were displayed fine do not show up any more. Not in their nodes or the views that use these fields (still, their thumbnails show just fine though). Could this be related to this commit here or what... ???

change proposed solution

...their img tags are rendered just fine, but when I try to load their src URL, it says the image cannot be displayed because it contains errors (firebug shows "Failed to load the given URL").

...never mind me. Browser cache glitch :/