Problem/Motivation

Today the PHP 7.0.x-dev and 7.1.x-dev environments began failing on Drupal\KernelTests\Core\Image\ToolkitGdTest with:

 fail: [Other] Line 217 of core/modules/image/src/Tests/ImageDimensionsTest.php:
Value 40 is equal to value 41.
 

https://www.drupal.org/pift-ci-job/795103

@Mixologic tracked down this change: https://github.com/php/php-src/commit/22c487616f132ee7ebfa838bce9d14c924...

Proposed resolution

It looks like our test might actually be asserting an off-by-one error that's the bugged behavior fixed by the recent PHP commit. If so, we'll need to do something tricky with the test.

Remaining tasks

TBD

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

xjm created an issue. See original summary.

xjm credited Mixologic.

xjm’s picture

xjm’s picture

Hm, looking at this:

    $this->assertEqual($this->getImageTag($variables), '<img src="' . $url . '" width="41" height="41" alt="" class="image-style-test" />');
    $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
    $this->drupalGet($this->getAbsoluteUrl($url));
    $this->assertResponse(200, 'Image was generated at the URL.');
    $this->assertTrue(file_exists($generated_uri), 'Generated file does exist after we accessed it.');
    $image_file = $image_factory->get($generated_uri);
    $this->assertEqual($image_file->getWidth(), 41);
    $this->assertEqual($image_file->getHeight(), 41);

That actually looks like the latest PHP dev is what's off-by-one.

dawehner’s picture

Its a bit non ideal to have test coverage depending on the PHP version. I'm wondering whether we could change the assertion to be +-1 or so. Let's phase it, in reality having an image being 1px bigger or smaller is not that much of a deal.

mondrake’s picture

The rotated image dimensions after a rotate 5 degrees operation were 42x24 up to PHP 5.4, and 41x23 as of PHP 5.5. D7 tests are still PHP version dependent.

So it looks like this changed again as effect of the upstream bugfix.

In #1551686: Image rotate dimensions callback can calculate new dimensions for every angle there was some discussion about adjusting the dimensions of the rotated image to the results of the Rectangle class calculation (so 'abstracting' dimensions from the GD/PHP calculation hiccups). But then we dropped that given that Drupal 8 raised PHP reqs to 5.5. See comments #23 to #27 there.

mondrake’s picture

Just uploading a patch taking some pieces from #1551686-23: Image rotate dimensions callback can calculate new dimensions for every angle, to see if resizing the image to dimensions calculated through Rectangle let the tests pass.

This is not the right fix though, ideally we should go for what #1551686-24: Image rotate dimensions callback can calculate new dimensions for every angle suggests.

The logic here is to 'force' image dimensions to the buggy PHP 5.5+ values. Even if not appropriate, that's what most of Drupal8 sites will be producing, anyway. This gives us breath to discuss whether we want to adjust Rectangle class calculations to the PHP logic just implemented upstream. The effect of that change would be that at one point, also PHP 5.5+ sites will size rotated images to the latest PHP 7.0.x values. But - a separate issue.

mondrake’s picture

A more complete patch, that instead of just resizing the rotated image if it's different from expected dimensions, will crop or set a canvas to the rotated image (respectively if the image is bigger or smaller than expected).

The set canvas part of the code is largely taken from Image Effect's 'setcanvas' operation.

mondrake’s picture

FileSize
37.04 KB
36.94 KB
38.55 KB

Some manual testing, applying the image style below to the sample image.

Image style:
- scale width 200
- rotate 5 degrees, background color #FF00FF

1) Run under PHP 7.1.8 (prior to PHP bugfix), output dimensions 212x166

2) Run under PHP 7.0.25 (after PHP bugfix), without the patch in #8 applied, output dimensions 214x168

3) Run under PHP 7.0.25 (after PHP bugfix), with the patch in #8 applied, output dimensions 212x166

1 and 3 have the same dimensions.

mondrake’s picture

  • xjm committed 7ea6068 on 8.5.x
    Issue #2918570 by xjm: Hotfix for ToolkitGdTest
    

  • xjm committed 5d451cd on 8.4.x
    Issue #2918570 by xjm: Hotfix for ToolkitGdTest
    
    (cherry picked from...
xjm’s picture

Priority: Critical » Major
Status: Needs review » Needs work

I had to hotfix this because in about half an hour PHP 7.0.26 is going to finish its deployment to the primary test environment and so all patch tests would stop because of this. #8 and #9 are great and deserve thorough reviews. We'll now want to remove the @todo and version check from my hotfix in this issue's patch:

diff --git a/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
index d283d9b4ba..78ca513306 100644
--- a/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -214,7 +214,9 @@ public function testManipulations() {
     ];
 
     // Systems using non-bundled GD2 don't have imagerotate. Test if available.
-    if (function_exists('imagerotate')) {
+    // @todo Remove the version check once
+    //   https://www.drupal.org/project/drupal/issues/2670966 is resolved.
+    if (function_exists('imagerotate') && (version_compare(phpversion(), '7.0.26') < 0)) {
       $operations += [
         'rotate_5' => [
           'function' => 'rotate',

Downgrading to major since the test failure will be resolved but this is still an actual bug.

  • xjm committed a303c45 on 8.5.x
    Issue #2918570 by xjm: Hotfix for ToolkitGdTest take 2
    

  • xjm committed 7fb8c07 on 8.5.x
    Issue #2918570 by xjm: Hotfix for ToolkitGdTest take 3
    

  • xjm committed de59b9a on 8.4.x
    Issue #2918570 by xjm: Hotfix for ToolkitGdTest take 3
    
    (cherry picked...
xjm’s picture

There were two tests that had this very special off-by-one error, so my first hotfix wasn't sufficient. So this patch should also uncomment these:

diff --git a/core/modules/image/src/Tests/ImageDimensionsTest.php b/core/modules/image/src/Tests/ImageDimensionsTest.php
index d81c04dc90..136b0f1f15 100644
--- a/core/modules/image/src/Tests/ImageDimensionsTest.php
+++ b/core/modules/image/src/Tests/ImageDimensionsTest.php
@@ -207,14 +207,18 @@ public function testImageDimensions() {
 
     $effect_id = $style->addImageEffect($effect);
     $style->save();
-    $this->assertEqual($this->getImageTag($variables), '<img src="' . $url . '" width="41" height="41" alt="" class="image-style-test" />');
+    // @todo Uncomment this once
+    //   https://www.drupal.org/project/drupal/issues/2670966 is resolved.
+    // $this->assertEqual($this->getImageTag($variables), '<img src="' . $url . '" width="41" height="41" alt="" class="image-style-test" />');
     $this->assertFalse(file_exists($generated_uri), 'Generated file does not exist.');
     $this->drupalGet($this->getAbsoluteUrl($url));
     $this->assertResponse(200, 'Image was generated at the URL.');
     $this->assertTrue(file_exists($generated_uri), 'Generated file does exist after we accessed it.');
     $image_file = $image_factory->get($generated_uri);
-    $this->assertEqual($image_file->getWidth(), 41);
-    $this->assertEqual($image_file->getHeight(), 41);
+    // @todo Uncomment this once
+    //   https://www.drupal.org/project/drupal/issues/2670966 is resolved.
+    // $this->assertEqual($image_file->getWidth(), 41);
+    // $this->assertEqual($image_file->getHeight(), 41);
David_Rothstein’s picture

We will need a hotfix (and maybe a real fix) for this in Drupal 7 too. I'm going to commit the attached patch as a hotfix if it passes tests. For Drupal 7, there was actually already code and a @todo (added in #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in) which predicted this exact scenario and explained what to do about it, so my hotfix looks different than the Drupal 8 one... and maybe is even good enough to qualify as a "real" fix? We'll see if it works.

Side note: The @todos in the Drupal 8 hotfix commits actually link to the wrong issue (they should probably link here, not 2670966).

David_Rothstein’s picture

Well, so much for that plan. Maybe the post-bugfix PHP behavior is different than the PHP < 5.5 behavior somehow.

Here's a different Drupal 7 hotfix that looks more like the Drupal 8 one.

mondrake’s picture

@David_Rothstein

+++ b/modules/simpletest/tests/image.test
@@ -353,13 +353,13 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
+      // https://bugs.php.net/bug.php?id=65148). This bug was then fixed in PHP
+      // 7.0.26 and 7.1.12, thereby restoring the earlier behavior. For the
+      // 40x20 test images, the dimensions resulting from rotation will
+      // therefore be 1 pixel smaller in both width and height for the affected
+      // PHP versions.

actually this is not correct - the dimensions in latest PHP 7 versions for this rotation is 43x25, different again from any earlier version. See #2921123: [PP-1] Adjust Rectangle class to calculate rotated image dimensions according to PHP bugfix #65148 for a fully restated list of test dimensions.

mondrake’s picture

Summarizing:

Rotation of a 40x20 image by 5 degrees:

PHP < 5.5                => 42x24
PHP >= 5.5               => 41x23
PHP >= 7.0.25 | 7.1.11   => 43x25
David_Rothstein’s picture

The hotfix in #22 passes tests, so I committed that to 7.x as a stopgap.

@mondrake, thanks - that probably explains it. So it would be an option to do an intermediate fix here that just checks the expected dimensions for each version. I'm pretty sure it's 7.0.26 and 7.1.12 though (not 7.0.25 and 7.1.11)? (At least, that's what the PHP release notes say, and there's currently also a testbot running PHP 7.1.11 which did not have these test failures.)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathanshaw’s picture

Priority: Major » Critical

This causes tests to fail in HEAD, such as https://www.drupal.org/pift-ci-job/868994 I just hit.

Mixologic’s picture

Priority: Critical » Major

That particular fail was due to configuring the patch to be tested against 8.3.x. Im not really sure how that was even an option given that it was just two days ago. 8.3.x was closed a while ago right?

jonathanshaw’s picture

Sorry about that, I just queued the old patch on that issue up for retesting yesterday and didn't notice it was still set at 8.3

Mixologic’s picture

I missed that the patch was from last year, and that it was just the files being hidden that was from a couple of days ago. We've logged a couple of testing stories to make it not possible to retest old patches with closed branches so that somebody else doesnt fall into that trap.