Drupal\KernelTests\Core\Image\ToolkitGdTest fails on PHP 7.1.x-dev and 7.0.x-dev following a PHP bugfix

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.

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.

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.

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.

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.

Added child issue #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ as a follow-up; see comment in #7.

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

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.

@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: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ for a fully restated list of test dimensions.

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

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

Rerolled #8 to remove the hotfix comments, and adjusted comment to point to PHP releases as indicated by @David_Rothstein.

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

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?

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

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.

#27 needs a reroll.

I think this patch is obsolete for Drupal 9 because Drupal 9 requires PHP 7.3. The issue is still tagged Needs backport to D7 and 8.8.x and 8.9.x are still supported. I changed the Version to 8.9.x. If this ends up committed, then a follow-up issue should be created for Drupal 7.

I think this still applies to PHP 7.3, 7.4 and 8 a unless PHP guys reverted to using PHP 5.5 GD code which I doubt. So this should still be valid for D9.

Maybe a test-only patch reverting @xjm hotfix from #13 would give us clarity.

I think we need to fix \Drupal\Component\Utility\Rectangle to have the same fix as whatever is in PHP. Atm in HEAD we're following the buggy PHP 5.5 algo. That doesn't really make much sense when the minimum version of Drupal 9 is PHP 7.3 - and we're not even running the test on any Drupal 8 test runs either since we do if (function_exists('imagerotate') && (version_compare(phpversion(), '7.0.26') < 0)) {... and PHP 7.0 version on Drupal CI is PHP 7.0.33...

@alexpott that’s what #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ is meant for.

@mondrake regardless the current situation in HEAD has meant we have no test coverage of something because we've preserved a bug from the days of PHP 5. I'm not sure we made the correct decisions there.

And this, I think, then "fixes" image rotate by breaking it - ie re-introducing a bug PHP fixed upstream.

I moved part of the patch here to latest patches in #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+, and would suggest to close this issue as a duplicate in favor of that one.

@mondrake I agree let's mark this a duplicate