Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in

Apparently, imagerotate fails and returns false but does not trigger a warning. This situation is not handled by the core image_gd_rotate function and therefore subsequent functions that operate on the resource will fail with the message as you get.

- If the error would have appeared earlier on and imagerotate() was not already passed in a resource, I expect you would have had a similar message for line 155 as well.
- If it is a lack of memory, you would get a fatal.

- Can you post the image that leads to this failure or does any image (that has a non-0 orientation) lead to the same failures?
- To be sure, can you post the image style (use the admin sub module to export the style).
- Can you post the phpinfo output (you can get that via the status report).

It must be a problem with your php installation. probably the gd component. EXIF and this module work fine, othewise image_gd_rotate() would not even be called. Can you output all php info?

Can you try the imagemagick toolkit and see if that works fine? I hope this is not a compatibility problem with PHP5.5.

You could debug to confirm that it is indeed the call to imagerotate() in image_gd_rotate that returns false (while a proper resource is passed in). Perhaps the value of $php_errormsg after the call to imagerotate can shed some light. After that it becomes difficult, a bit of searching for situations where imagerotate might return false does not give any usable results. Delving into the code of PHP?

Running into the same issue on a php 5.5 server. It seems the exif info fetched in the autorotate module is from an already processed image (despite having the resize action as the first filter) and no longer contains any useful info. This might come from the maximum resolution setting on the upload field which scales the image back to 800x600 if larger, perhaps before the rotate module can spring into action

Switching to imagemagick toolkit solved the problem too.

Exif and PHP5.5:
- https://bugs.php.net/bug.php?id=66443
- https://bugs.php.net/bug.php?id=66519

What I don't understand is how this would corrupt GD resources. The GD resource is created at the beginning of an image style, so before exif_read_data is called which operates on the file (unless of course there is some code sharing and optimization in PHP).

To further pinpoint:
- Can you replace the auto-rotate with a rotate image effect with a fixed number of degrees and have it operate on a camera image that fails with auto-rotate?
- Can you test the auto-rotate with an image from a different camera (try portrait-painting.jpg provided by the auto-rotate module)?

On working on code D8 issue #2073759: Convert toolkit operations to plugins I found the probable cause of this problem. Therefore I'm moving this issue to the D8 core issue queue. I updated the summary accordingly.

Feel free to review, but it is based on top of #2073759: Convert toolkit operations to plugins, so postponing om that issue.

New patch, the previous one was a reversed patch.

- Closed #2165411: ImageDimensionsTest & ToolkitGdTest fail on PHP 5.5 as a duplicate.
- The test results it refers to (https://qa.drupal.org/pifr/test/600303) indicate the same errors i encountered here and that no longer appear when I run the (image related) tests locally (PHP5.5 on Windows): Drupal\image\Tests\ImageDimensionsTest and Drupal\system\Tests\Image\ToolkitGdTest.

This patch:
- Solves some errors in handling color indices in the rotate operation.
- Defines a color code as transparent color that is fully transparent on its own. This seems to give better results on image formats that support a transparency channel.
- Corrects a dimensions test because in PHP5.5 rotation may result in smaller images then before.
- Attached test file can be used to do further research in resultng image dimensions for some given rotation angles and to find out which interpolation filter works best (generally).

Could we make sure that we have a bug open against PHP and to notify Pierre Joye? I guess this is related to all the GD refactoring that he has done.

PHP Bug: https://bugs.php.net/bug.php?id=65148, a certain 'pajoye' has reacted on that bug...

This will remove 52 from the current 53 fails and all 131 exceptions on the PHP5.5 testbot (https://qa.drupal.org/pifr/test/600303#tabset-tab-5).

Setting to RTBC to get feedback from a maintainer if we wan to get it in like this.

I just lost hours debugging #2028109: Convert hook_stream_wrappers() to tagged services. where I thought it was a problem related to that but it wasn't. Very annoying :)

Maybe we should at least add a reference to the bug report and a @todo to remove it again or limit to specific versions only or at least the tests will suddenly fail again?

Maybe we should at least add a reference to the bug report and a @todo to remove it again or limit to specific versions only or at least the tests will suddenly fail again?

I'd be OK with the current approach if this is added.

In #1551686-11: Image rotate dimensions callback can calculate new dimensions for every angle I found out that the 'dimensions after rotate' problem is worse than documented here (up to 3 pixels on 1 axis and 5 in total) and that Imagemagick exhibits more or less the same deviations form the math I use over there. I'm currently looking into solving it over there by just following the GD internals.

I edited the issue summary to more clearly state the problems encountered and solved in this patch. I think that even without the correction to the dimensions tests (3rd error), the number of test failures on PHP 5.5 will already be down dramatically. So that part can be:
- accepted as is (up to the core committer)
- removed (leaving a number of PHP 5.5. testbot failures in)
- annotated with a todo and bug reference (as suggested by @berdir) and then accepted
- corrected once and for all: I would like to postpone that to the related issue.

Note: IMO, limiting to specific versions is as bad as well: when newer PHP versions arrive, tests will suddenly fail again and we will need to update the upper version limit with each new PHP version until the bug has been solved. If we do not place an upper limit on the PHP version, the test will fail if and when the bug has been solved. We then need to change the test once and it will be gone.

EDIT: we have an answer form a core committer. I will update the patch.

Note: IMO, limiting to specific versions is as bad as well: when newer PHP versions arrive, tests will suddenly fail again and we will need to update the upper version limit with each new PHP version until the bug has been solved. If we do not place an upper limit on the PHP version, the test will fail if and when the bug has been solved. We then need to change the test once and it will be gone.

What I meant is that we add the specific versions once the bug was fixed, so if it's fixed in 5.5.10 or example, we after that was released add the version check for <= 5.5.9. And for now, we just add a @todo to do that.

#23: Then we think the same about this part.

#21: done

FYI:

• error 1 and 2 (see issue summary, thus the changes within the gd rotate operation) reduce the number of (image related) test failures from 52 fails, and 131 exceptions to 24 fails, and 12 exceptions.
• The new change (using actual image width and height instead of expected width and height; around line 290 of gdtoolkittest) to get the corner pixels further reduces this to 12 fails, and 0 exceptions. This can be seen as an error in the tests: these specific assertions are not testing expected dimensions but image contents so may use actual image dimensions without invalidating the tests.
• The reduction of expected width and height based on PHP version number (around line 260 of gdtoolkittest) reduces the number of fails to 0.

Started a partial d7 backport patch here: https://www.drupal.org/node/2337081

After some offline discussions with @mondrake and further testing by both of us, we found problems with the current implementation when the gif does not have a transparent color set. So I added another test image.

I also discovered that GD 5.5 does not handle the negative rotation angles -90, -180 and -270 as special cases that can be done fast and without any loss. The rotate operation now checks for this.

This patch:
- Adds a gif test image without any transparency to the test set
- Makes use of the color utility. To do so, the background argument of the rotate operation has to be passed as a string, not an int.
- Improves handling all the combinations of gif images with and without transparency set and the background argument adding transparency or not. This is difficult error prone stuff, so I added a lot of comments as well.

It locally runs on PHP 5.5 without image related test errors.

Issue summary updated.

Patch in #26 does not contain the added gif. Let's try again. interdiff remains actual, as I suppose we are not interested in reading binary rubbish.

Added steps to reproduce in issue summary, tested manually the patch against these steps, OK. Code looks good. RTBC

Wow. That's a crazy-good find. And yay for test coverage as well.

Committed and pushed to 8.x. Thanks!

Marking for backport.

Here's a first stab at a D7 port; does this need updated tests for D7 or is the current coverage sufficient?

We were having a problem with photos taken with an iPad. They were getting rotated in a Drupal 7 instance.

#32 fixed it

Thank you

I was testing this patch as I had problem with error message when rotating images.

When applying the patch the error message disappear. But the background didn't get transparent as I wanted. It got black.

With further investigation the function imagecolortransparent is returning -1 and not 0 for no background. Se (http://php.net/manual/en/function.imagecolortransparent.php).

And also the patch for Drupal 8.x has it. (http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Plugin/Im... row 67).

After changing the if case for the imagecolortransparent the transparent works.

Here comes a updated patch.

#35 patch fixed image_rotate error on php 5.5

+1 for 35 working nicely!

Extracting some code into separate functions (_image_validate_hex and _image_hex_to_rgb) does improve readability. The functions could be made a bit more tolerant of faulty input (what if no string is passed in, do we get a warning on ltrim?) but that is not the point here.

Looking a the +1's, we see an increased use of PHP5.5, even with D7. The rest of the patch is a straight backport of the D8 patch and works as expected (#36, #37 (, #32)), so good to go, except if we want better test coverage in D7 as well. But that's up to the core maintainer.

Patch #35 works like a charm...Thank you so much...

Testbot fluke.

The #35 patch causes the background fill to be wrong, for example creating an image style that rotates 45 degrees with #FF00FF as background color defined results with the following images before and after applying the patch, respectively.

setting status to "needs work"

modified patch #35 which assumed it needed to parse image hex value, when actually was already getting decimal value

there is still some transparency issue with gif images. the logic is somehow mistakenly executing imagecolorallocatealpha() without first building the $background rgba array in a certain gif image rotate_transparent_5 action situation

a secondary problem that may need a new issue (I am asking for advise on this) is that these tests are passing because they are being skipped when in fact the tests should be failing. The skipped test message says "Image manipulations for the GD toolkit were skipped because the GD toolkit is not available" which is false, GD is available

The second problem has already been solved in #375062: imagecolorsforindex() Color index nnn out of range in GDToolkit, see comment #375062-112: imagecolorsforindex() Color index nnn out of range in GDToolkit from the D7 maintainer himself, who also found out this glitch as well. So we don't have to worry about that here, though we may want to add it here as well to get test coverage for this issue as well before committing it.

Regarding the 1st problem: I did not look into the problem in any detail, but if that problem is also present in the D8 code base, we have to make it a separate issue and solve it in D8 first.

Then the patch itself:

1. +++ b/modules/image/image.module
   +function _image_validate_hex($hex) {
   

   This function can be removed as it is no longer used,

2. +++ b/modules/image/image.module
   @@ -1399,3 +1399,45 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
   + * @param string $decimal
   + *   The decimal color string to convert.
   + *
   ...
   +function _image_convert_rgba($decimal) {
   

   - type is int
   - Please rename to _image_dec_to_rgba to make it clear what we are expecting as input.

3. +++ b/modules/system/image.gd.inc
   +++ b/modules/system/image.gd.inc
   @@ -116,38 +116,57 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
   

   The phpdoc on image_gd_rotate is thus wrong as well. Let's correct that and make clear that we are expecting an (24 bit) integer, not a hexadecimal integer (what is that anyway?)

4. Though this is on the edge of adding a new feature, but anyway: I never understood why we are not accepting an RGBA value in the effect? We could start with changing _image_dec_to_rgba() to accepting a 32 bit integer containing an alpha value as well and convert that 0 to 255 value to the GD 0 to 127 scale! and use fully opaque (=0) as default.

Can you adapt the patch for the above points (decide for yourself what to do with the 4th point) and clarify the mentioned problem regarding transparency? Try to include an interdiff as well.

Thanks for the comments, really helpful, although some points are over my head, but only by a little.

regarding your points

1. will delete that function now
2. I can rename to _image_dec_to_rgba but what do you mean by -type is int? Should I be adding comment or is there a function parameter to set??? or something?
3. for the image_gd_rotate doc I can change hexadecimal to decimal but I don't really understand all the image manipulation cases to do much more than that, including how to explain the 24 bit vs 32 bit requirements in concise way. 24 bits has enough for the rgb data, but the transparency data requires another word of space, right? I feel like I could actually rewrite it but it might sound horrible, confusing, and awkward if I tried, not to mention wrong, lol.
4. I like your approach here to make this future compatible...and after writing these comments here I think I am beginning to understand most everything involved here except what to do with the "type is int" comment you made about point #2

I will investigate the gif transparency issue more and learn how to make the interdiff

Inserted a TODO on line 137 of image.gd.inc where I think the transparent color should be added to the $background variable. In the case of a gif image, the $background value becomes "4", the transparent identifier returned from the imagecolortransparent() function on line 130. And then the following $background_idx needs to see $background as an rgba array which will not have been set without my TODO addition.

Since I have no idea what a "4" identifier really means, and how to represent that as an rgba, I've inserted the TODO

In the _image_dec_to_rgba() function I've added a little alpha value to the array from the $background using >> 24, and in my test cases it returns 0 as expected from a 24 bit decimal color but I don't know if it will return correct values for the alpha data in larger decimal color values.

please let me know if the interdiff looks correct

resubmitting with correct file extension

so far I've actually been testing and patching for php5.3 and 5.4. Now on 5.5 I have only one additional thing to address...that I am aware of.

The imagerotate() will return different width and height values between 5.4 and 5.5.

testing a 20x40 pixel image, rotating 5 degrees

5.4 => 24x42
5.5 => 23x41

Any suggestions on how to proceed will be appreciated. Is it possible, or even a good idea to use different test values for sites using php5.4 vs php5.5?

Thank you for picking this up. With int I meant that the type of the parameter is not a string but an int(eger). See below.

1. +++ b/modules/image/image.module
   @@ -1399,3 +1399,22 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
   +/**
   + * Converts a decimal color string to rgba.
   + *
   ...
   + *
   + * @return array
   + *   An array containing the values for 'red', 'green', 'blue', 'alpha'.
   + */
   +function _image_dec_to_rgba($decimal) {
   +
   

   I propose to change the comment and parameter name as follows, including setting the order of the array to r g b a:
   /**
   *
   /**
   * Converts a 24 bit RGB or 32 bit ARGB value to an RGBA array.
   *
   * @param int $argb
   * The color code to convert.
   *
   * @return array
   * An array containing the values for 'red', 'green', 'blue', 'alpha'.
   */
   function _image_dec_to_rgba($argb) {
   return array(
   'red' => $argb >> 16 & 0xFF,
   'green' => $argb >> 8 & 0xFF,
   'blue' => $argb & 0xFF,
   'alpha' => $argb >> 24 & 0xFF,
   );
   }

2. +++ b/modules/system/image.gd.inc
   @@ -99,9 +99,9 @@ function image_gd_resize(stdClass $image, $width, $height) {
     * @param $background
   - *   An hexadecimal integer specifying the background color to use for the
   - *   uncovered area of the image after the rotation. E.g. 0x000000 for black,
   - *   0xff00ff for magenta, and 0xffffff for white. For images that support
   ...
     *   transparency, this will default to transparent. Otherwise it will
   

   Plase add the type int(eger) to the $background param. There has been to much confusion about what gets passed in, leading to a.o. the errors we are solving here.
   @param int $background
   An 24 bit RGB or 32 bit ARGB value specifying the background color to use for the uncovered area of the image after the rotation. ... rest of the comment is ok ...

3. +++ b/modules/system/image.gd.inc
   @@ -116,38 +116,62 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
   +    $background = _image_dec_to_rgba($background);
   

   As _image_dec_to_rgba() returns an alpha value between 0 and 255 this must be divided by two for GD that only accepts an alpha range of 0 to 127, by adding the line:
   $background['alpha'] /= 2;

4. Regarding the TODO:
   This part has not correctly been ported from the D8 patch in comment #27. Over there, we
   - first get a rgba array for the background
   - get a color index for that rgba array
   - only then try to handle gif transparency (that large block starting with // GIF does not work with a transparency channel, but can define 1 color who)

   So, the inner if - else statement in the outer else part can be replaced by a single assignment to $background.

   		     // Get the current transparent color.
        $background = imagecolortransparent($image->resource);
    
   -    // If no transparent colors, use white.
   -    if ($background == 0) {
   -      $background = imagecolorallocatealpha($image->resource, 255, 255, 255, 0);
   +    // If no transparent colors, use transparent white.
   +    if ($background === -1) {
   +      $background = array('red' => 255, 'green' => 255, 'blue' => 255, 'alpha' => 127);
   +    }
   +    // Otherwise apply the transparent color
   +    // TODO: correctly apply this instead of just copying the use transparent white from above
   +    else{
   +      $background = array('red' => 255, 'green' => 255, 'blue' => 255, 'alpha' => 127);
        }
   

   Should become:

    $background = array('red' => 255, 'green' => 255, 'blue' => 255, 'alpha' => 127);
   

   See the patch in #27 and add the same line of comment to that.

5. The differences in dimensions in PHP5.5+ have been tackled in the D8 patch in the tests, we should port that part back as well to this patch.

Patch #51 worked for me on D7.

Patch #51 also worked for me

Patch #51 is working great here

RTBC #51

Hmm, but now my ImageCache Actions "transparent canvas behind image" action makes everything white. As if it were putting the canvas on top of the image. Specifying #ffffff in said filter instead of leaving it empty (and thus Transparent) fixes it.

Could this patch have caused it?

Setting to NW as per the review in #53.

I have no idea how this patch could influence the canvas effect, as the only changes are in the image_gd_rotate function, which is not called over there. If you report that to the imagecache_actions issue queue, I (as co-maintainer of that module) will have a look at that.

@fietserwin, I'll do that.

Minor remark:

+++ b/modules/system/image.gd.inc
@@ -116,38 +116,62 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
+  // PHP 5.5 GD bug: https://bugs.php.net/bug.php?id=65148: To prevent buggy
+  // behavior on negative multiples of 90 degrees we convert any negative
+  // angle to a positive one between 0 and 360 degrees.
+  $degrees -= floor($degrees / 360) * 360;

This will convert an angle of -540 to -180, could this form a problem?

floor(-540/360) = -2, floor means "round down", not "round towards 0" ... I will dive into your other new issue later.

Wow, what a mistake on my part. Guess next time I'll use "php -a" before I comment :o

https://www.drupal.org/node/2544678#comment-10396365

I figured out how the int value for $bgd_color in PHP's imagerotate function works and understand how to preserve transparency. I'll see if I can contribute patches to fix related Drupal modules to handle this properly.

-

Ah, I see Lowell already submitted a patch similar to mine at https://www.drupal.org/node/2215369#comment-9752055

I fixed transparent gd rotation issues by modifying modules/system/image.gd.inc and adding to line 139 of image_gd_rotate function:

    else if ($background == -1) {
      $background = imagecolorallocatealpha($image->resource, 0, 0, 0, 127);
    }

Note that imagecolorallocatealpha($image->resource, 0, 0, 0, 127); returns int(2130706432)

I have re-rolled #51 with the changes mentioned in #53.
Currently using this patch in production as it fixes Image EXIF Autorotate.

Note: I have not verified the test changes work locally as I my setup currently prevents me from running all tests.

Attached patch should resolve the remaining test failure.

The result of rotating a transparent gif by 5 degrees does yield an unexpected result though:

I'm not sure if this is due to gif's limited support for transparancy. The outer region of the resulting image is transparant, the original transparent region has become yellowish (which is the background color we defined ourselves.) Colormap & histogram before and after rotate attached.

Tests green after retesting, so back to NR.

@mr.baileys re #69

The result of rotating a transparent gif by 5 degrees does yield an unexpected result

please note that there is a pending issue with GIF transparency, see #2485761: Support for transparent GIFs broken. It was fixed in D8 but waiting for review in D7. Not sure it impacts here, but just to let you know.

This issue had dropped of my radar until I hit it again on 1 of my own sites. This while a working and correct patch has been around for several months now.

- All points of #53 are corrected (since #66),
- #69 indeed resolves the test failure
- #72 correctly points out that the gif problem mentioned in #69 is a separate issue.
- As the non tests code part of the patch has not been changed since #51, the positive feedback of #55 to #58 also holds for the current patch.
- I did a new review and test of the patch in #69.

RTBC

#69 helped me, thank you, mr.baileys.
PHP 5.6.17
imagecache_autorotate 7.x-1.7
image_exif_autorotate 7.x-1.0

Thanks mr.baileys
#69 Works on PHP 7.0
imagecache_autorotate 7.x-1.7
image_exif_autorotate 7.x-1.0

# remove this comment please, I'm an idiot and it was unrelated

The patch is crazy with including so many things at once. However the same has gone into D8, so this looks good to me as it is a straightforward bug fix.

Yeah, all the fixes are a little hard to follow here, but overall it looks good and I think the patch contains what it's supposed to (with the differences with the Drupal 8 code mostly due to other changes elsewhere).

However I found a few things:

1. @@ -116,38 +116,52 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
   ....
   -    $background = imagecolorallocatealpha($image->resource, $rgb[0], $rgb[1], $rgb[2], 0);
   +    $background = _image_dec_to_rgba($background);
   +    $background['alpha'] /= 2;
      }
   ....
   +  // Store the color index for the background as that is what GD uses.
   +  $background_idx = imagecolorallocatealpha($image->resource, $background['red'], $background['green'], $background['blue'], $background['alpha']);
   

   This ability to specify the alpha looks like a new feature which does not appear to be present in Drupal 8? (I'm comparing this to https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Plugin...)

   It is not clear to me why we should be adding that here.

   And if we do, what does that mean for other image toolkits (which don't currently expect an alpha to be specified by the caller)?

   Can someone explain?

2. --- a/modules/image/image.module
   +++ b/modules/image/image.module
   @@ -1413,3 +1413,21 @@ function image_filter_keyword($value, $current_pixels, $new_pixels) {
    function _image_effect_definitions_sort($a, $b) {
      return strcasecmp($a['name'], $b['name']);
    }
   +
   +/**
   + * Converts a 24 bit RGB or 32 bit ARGB value to an RGBA array.
   + *
   + * @param int $argb
   + *   The color code to convert.
   + *
   + * @return array
   + *   An array containing the values for 'red', 'green', 'blue', 'alpha'.
   + */
   +function _image_dec_to_rgba($argb) {
   

   This function is defined in the Image module but used in the System module. That won't work since Image module is optional and as far as I know it's very possible for this to be called with the Image module disabled (image_rotate() is a public API function defined in includes/image.inc, not the Image module).

   So I think it needs to be moved elsewhere, probably to includes/image.inc, and also should be renamed to be public (e.g. image_dec_to_rgba()).

   Also a minor fix on the documentation: should be "and 'alpha'" at the end of the list.

3. +        // PHP 5.5 GD bug: https://bugs.php.net/bug.php?id=65148. PHP 5.5 GD
   +        // rotates differently then it did in PHP 5.4 resulting in different
   +        // dimensions then what math teaches us. For the test images, the
   +        // dimensions will be 1 pixel smaller in both dimensions (though other
   +        // tests have shown a difference of 0 to 3 pixels in both dimensions.
   +        // @todo: if and when the PHP bug gets solved, add an upper limit
   +        //   version check.
   +        // @todo: in #1551686: Image rotate dimensions callback can calculate new dimensions for every angle the dimension calculations for rotation are
   +        //   reworked. That issue should also check if these tests can be made
   +        //   more robust.
   

   There are a few grammatical mistakes and such in the above (I realize this was ported directly from Drupal 8, but it's been removed in Drupal 8 since then, so we can just fix them here).

   I think we can also remove the @todo pointing to #1551686: Image rotate dimensions callback can calculate new dimensions for every angle - based on discussion there it looks unlikely that issue will be backported to Drupal 7, and even if it is the Drupal 8 fix wound up just removing all this code anyway, so that would naturally happen as part of a backport.

4. - * @param $background
   - *   An hexadecimal integer specifying the background color to use for the
   - *   uncovered area of the image after the rotation. E.g. 0x000000 for black,
   - *   0xff00ff for magenta, and 0xffffff for white. For images that support
   + * @param int $background
   + *   An 24 bit or 32 bit ARGB value  specifying the background color to use for
   + *   the uncovered area of the image after the rotation. E.g. 0 for black,
   + *   16711935 for fuchsia, and 16777215 for white. For images that support
     *   transparency, this will default to transparent. Otherwise it will
     *   be white.
   ...
   @@ -116,38 +116,52 @@ function image_gd_rotate(stdClass $image, $degrees, $background = NULL) {
   

   A couple small grammatical issues here too.

   Also, my understanding is that the previous documentation is just wrong, and that we aren't actually changing the meaning of this parameter (except perhaps for the "alpha" issue mentioned above). Given that, don't we need the same fix in the image_rotate() documentation itself?

I'm attaching a reroll for everything except the first point above, since I'm not sure what the answer to that one is.

@David_Rothstein re #79:

1. You are correct, good catch. In D8's Drupal\system\Plugin\ImageToolkit\Operation\gd\Rotate the background can be either fully opaque or fully transparent. It could be probably desirable to specify a semi-transparent background color, but that would be another issue. Corrected here.

3. I am not sure to say that PHP 5.5 has a bug here. It's just a different algorithm that stays also in 5.6 and 7. D8 has not a problem as it requires 5.5+ so we are over the turn there. PHP < 5.4 is out of support so I do not think we can expect any change there. So we have to live probably forever with a check of the PHP version in D7 test. Here I just tried to make it more understandable.

#79.2 maybe based on #80 we do not need that function at all.

Hm. It also looks part of the tests that were committed for D8 in #30 were not backported. Working on that.

With tests from #27 backported.

Note that there is also a related #2485761: Support for transparent GIFs broken issue that deals with GIF transparency issues, fixed in D8 but waiting for review in D7.

Patch in #83 is for review, passes on 5.3. On 5.5 and 7 the failures are related to other PHP issues.

Test-only in #83 shows the failure net of the adjustment to test code required for PHP 5.5 dimensions calculations.

Side note.

I see that in the upstream issue https://bugs.php.net/bug.php?id=65148 there is now a patch for review. In any case, even if that will be eventually committed I think that it won't be on 5.5 which is now in security fix only till July 10, 2016, then it will be EOL. So the version checking in our test code should stay.

Those changes look great to me - thanks.

I confirmed manually that setting $background to 0xffffff (what the current documentation says to do for white) produces the same result as 16777215 (what the previous proposed documentation says to do). So either works, and we don't need to change the documentation after all (since 0xffffff is definitely more intuitive).

The only functional change from the previous patch regards the issue with alpha, tests are passing as expected, and I checked via a manual test (similar to the steps to reproduce in the issue summary) that image rotation on PHP 5.5 works again with this patch applied. So I'm marking it RTBC.

A minor issue which could be fixed on commit:

+      // As of PHP version 5.5, GD uses a different algorithm to rotate images
+      // than version 5.4 and below, resulting in different dimensions.
+      // See https://bugs.php.net/bug.php?id=65148).

(the parentheses at the end should be removed)

Also when committing this needs to be applied with git apply to pick up the binary file.

Aside, FYI: The PHP problem IS a bug and the patch proposed over there does NOT solve our problem.

It is a bug, as the addition of +1 (or +.5 (I don't remember anymore)) is always positive even if the sin/cos returns a negative value and it is only done on one side of the +, not the other side. The proposed patch (for the PHP bug) does not solve that part of the problem (and thus IMO, does not solve that bug at all).

@fietserwin

It is a bug, as the addition of +1 (or +.5 (I don't remember anymore)) is always positive even if the sin/cos returns a negative value and it is only done on one side of the +, not the other side.

Yes you are right, thanks for pointing that out. It's 0.5 and that value we factored into in the Rectangle::rotate method introduced in #1551686: Image rotate dimensions callback can calculate new dimensions for every angle.

+    // For some rotations that are multiple of 30 degrees, we need to correct
+    // an imprecision between GD that uses C floats internally, and PHP that
+    // uses C doubles. Also, for rotations that are not multiple of 90 degrees,
+    // we need to introduce a correction factor of 0.5 to match the GD
+    // algorithm used in PHP 5.5 (and above) to calculate the width and height
+    // of the rotated image.
+    if ((int) $angle == $angle && $angle % 90 == 0) {
+      $imprecision = 0;
+      $correction = 0;
+    }
+    else {
+      $imprecision = -0.00001;
+      $correction = 0.5;
+    }

Looks like we'll have to live with that for long time...

This is good to go, marking for commit.

#83 is soooo much better to read and also easier to understand.

Great work!

Committed and pushed to 7.x! Thanks!

And with that PHP 7 should be green! (hopefully)

--

Fixed on commit:

diff --git a/modules/simpletest/tests/image.test b/modules/simpletest/tests/image.test
index d31602a..7ca1d3a 100644
--- a/modules/simpletest/tests/image.test
+++ b/modules/simpletest/tests/image.test
@@ -354,7 +354,7 @@ class ImageToolkitGdTestCase extends DrupalWebTestCase {
       );
       // As of PHP version 5.5, GD uses a different algorithm to rotate images
       // than version 5.4 and below, resulting in different dimensions.
-      // See https://bugs.php.net/bug.php?id=65148).
+      // See https://bugs.php.net/bug.php?id=65148.
       // For the 40x20 test images, the dimensions resulting from rotation will
       // be 1 pixel smaller in both width and height in PHP 5.5 and above.
       // @todo: If and when the PHP bug gets solved, add an upper limit