Issue

Steps to reproduce bug (this was tested on a 7.x site - the code looks to be the same for 8.x but will need to be tested out there as well):

  1. Create image preset where the images scale to a given width (in my case, the width was set to 260 pixels) and allow for upscaling.
  2. Have an image field associated with a bundle.
  3. Under display management for the bundle, select to view the image field with the given preset)
  4. Create content for bundle.
  5. Upload an image where the image is taller than the width (my image was 650 pixels wide and 864 pixels tall)

In my scenario, even though the backend image was actually 260 pixels wide and 345 pixels tall, the frontend output was:
<img width="147" height="195.601851852" title="This is a test" alt="Read more about the image at http://redcat.localhost:8080/visit/lounge-redcat#caption-image-2" src="http://redcat.localhost:8080/sites/redcat.org/files/styles/first-sidebar/public/redcat_sheet/associated-images/2011-11/K3F7674_02.jpg" longdesc="/visit/lounge-redcat#caption-image-2" typeof="foaf:Image">
Where you can see the image width does not match (I have uploaded part of the output from firebug so you can see what is happening).

Proposed Resolution

Refactor the functions.

Files: 
CommentFileSizeAuthor
#42 refactor-image_dimensions_scale.patch8.63 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 37,325 pass(es). View
#37 refactor-image_dimensions_scale-37.patch8.47 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 34,104 pass(es). View
#37 interdiff-35-37.txt746 bytesTor Arne Thune
#35 refactor-image_dimensions_scale.patch8.66 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 34,111 pass(es). View
#32 refactor-image_dimensions_scale.patch8.67 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 34,053 pass(es). View
#30 refactor-image_dimensions_scale-d7.patch8.21 KBjbrown
#29 1338428_testonly.patch4.07 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 34,018 pass(es), 2 fail(s), and 0 exception(es). View
#29 1338428.patch8.04 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,035 pass(es). View
#25 refactor-image_dimensions_scale.patch7.91 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 34,016 pass(es). View
#25 refactor-image_dimensions_scale-tests-only.patch3.83 KBjbrown
FAILED: [[SimpleTest]]: [MySQL] 33,987 pass(es), 2 fail(s), and 0 exception(es). View
#23 refactor-image-dimensions_scale.patch7.39 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 34,017 pass(es). View
#23 refactor-image_dimensions_scale-tests-only.patch3.78 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 34,017 pass(es). View
#18 1338428_18.patch5.03 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 34,004 pass(es). View
#17 1338428_16_testonly.patch2.38 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 33,998 pass(es), 4 fail(s), and 0 exception(es). View
#17 1338428_16.patch5.03 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,991 pass(es). View
#9 1338428_9.patch2.65 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,985 pass(es). View
#8 1338428_8.patch2.66 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,982 pass(es). View
#5 refactor_image_dimensions_scale_d7.patch2.77 KBjbrown
#4 refactor_image_dimensions_scale.patch2.82 KBjbrown
PASSED: [[SimpleTest]]: [MySQL] 33,974 pass(es). View
#2 image-dimension-after-patch.png435.58 KBBTMash
#1 1338428_1.patch854 bytesBTMash
PASSED: [[SimpleTest]]: [MySQL] 33,972 pass(es). View
image-dimension-inconsistency.png392.77 KBBTMash

Comments

BTMash’s picture

FileSize
854 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,972 pass(es). View

Attaching patch for review.

BTMash’s picture

Just wanted to add an after-patch picture that shows the patch seems to resolve this issue.

good_man’s picture

Confirmed, and the patch seems working very fine.

jbrown’s picture

FileSize
2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 33,974 pass(es). View

What about like this?

jbrown’s picture

Title: The image style attributes for width / height do not match with the altered image attributes » image_dimensions_scale() and image_scale_effect() are ungrokable and buggy
FileSize
2.77 KB

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.

BTMash’s picture

I should be able to do a real test of the patch you've written (I agree that what I have was probably far from ideal :() but from a first glance through the code, it looks good. Not sure why your patch didn't run through the tests, though.

jbrown’s picture

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

BTMash’s picture

FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 33,982 pass(es). View

Still not tested, but rerolled it for 8.x.

BTMash’s picture

FileSize
2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 33,985 pass(es). View

Ack...double comment.

jbrown’s picture

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.

jbrown’s picture

BTMash’s picture

Ack...not sure why I hadn't looked at #4. Anyways, I finally had a chance to test the patch with various images and it tests out properly for me. I'm going to change the status to RTBC (and hopefully a few others can chime in with tests/whatnot as well :))

BTMash’s picture

Status: Needs review » Reviewed & tested by the community

argh...I hadn't changed it to rtbc.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

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.

xjm’s picture

Status: Needs review » Needs work

Sorry, wrong status before.

BTMash’s picture

Status: Needs review » Needs work

Double post...

BTMash’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 33,991 pass(es). View
2.38 KB
FAILED: [[SimpleTest]]: [MySQL] 33,998 pass(es), 4 fail(s), and 0 exception(es). View

Since I am not entirely familiar with creating tests for this, I created a new test function that directly tests against image_dimensions_scale() which is a large cause of the issue. Attaching two patches. One that is only a test, and one that is the patch from #9/#4 plus the patch.

BTMash’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 34,004 pass(es). View

Removing whitespace.

BTMash’s picture

Status: Needs review » Needs work

Thinking it all through, here are all possible scenarios that I see for getting the scaled dimensions of an image. An image should fall under the following scenarios:

  • Large Landscape (so the image width/height is larger than scaled width/height)
  • Large Portrait (so the image width/height is larger than scaled width/height)
  • Large square (so the image width/height is larger than scaled width/height)
  • Small Landscape (so the image width/height is smaller than scaled width/height)
  • Small Portrait (so the image width/height is larger than scaled width/height)
  • Small square (so the image width/height is larger than scaled width/height)

And this image can fall under the following set of scaling presets:

  1. Set width, unset height, upscale ON
  2. Set width, unset height, upscale OFF
  3. Set height, unset width, upscale ON
  4. Set height, unset width, upscale OFF
  5. Set width >= set height, upscale ON
  6. Set width >= set height, upscale OFF
  7. Set width < set height, upscale ON
  8. Set width < set height, upscale OFF

Of these possibilities, I have written out most of the first one (and I am unsure on what other tests are covered in the dimensions tests). We should write out these various possibilities to ensure they all get covered.

jbrown’s picture

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.

BTMash’s picture

The images can also be larger or smaller than the target dimensions, bringing the tally to 60.

jbrown’s picture

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.

jbrown’s picture

Status: Needs review » Needs work
FileSize
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 34,017 pass(es). View
7.39 KB
PASSED: [[SimpleTest]]: [MySQL] 34,017 pass(es). View

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.

jbrown’s picture

Status: Needs work » Needs review
jbrown’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
FAILED: [[SimpleTest]]: [MySQL] 33,987 pass(es), 2 fail(s), and 0 exception(es). View
7.91 KB
PASSED: [[SimpleTest]]: [MySQL] 34,016 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, refactor-image_dimensions_scale-tests-only.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

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. :)

BTMash’s picture

Would it be worthwhile to to have an 'expected output, actual output' as part of the search result? Since it goes through a loop, it would be hard to track down which test case is failing.

BTMash’s picture

FileSize
8.04 KB
PASSED: [[SimpleTest]]: [MySQL] 34,035 pass(es). View
4.07 KB
FAILED: [[SimpleTest]]: [MySQL] 34,018 pass(es), 2 fail(s), and 0 exception(es). View

I've added in the expected output part of things. I find it helps but more review helps.

jbrown’s picture

Great!

I re-rolled it for D7.

xjm’s picture

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.

jbrown’s picture

FileSize
8.67 KB
PASSED: [[SimpleTest]]: [MySQL] 34,053 pass(es). View
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, this looks ready to me.

brunorios1’s picture

#30 worked for me in D7!

jbrown’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.66 KB
PASSED: [[SimpleTest]]: [MySQL] 34,111 pass(es). View

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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

Tor Arne Thune’s picture

FileSize
746 bytes
8.47 KB
PASSED: [[SimpleTest]]: [MySQL] 34,104 pass(es). View
+++ 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 :)

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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

jbrown’s picture

Looks good!

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright. :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks great, nice set of tests as well.

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

jbrown’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -needs backport to D7
FileSize
8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 37,325 pass(es). View

Thanks catch!

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Backport looks fine as well.

kehan’s picture

Patch in #42 fixes the issue for me thanks.

alive2000’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

klonos’s picture

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... ???

klonos’s picture

Issue summary: View changes

change proposed solution

klonos’s picture

...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 :/