Support for transparent GIFs broken

Have you looked through the 7.36 or 7.37 release notes to try and identify the issue that might have caused this problem? I looked through them, but didn't really see anything. Sorry, I'm just not familiar with this functionality.

#375062: imagecolorsforindex() Color index nnn out of range in GDToolkit does seem likely, yes.

I'll leave a comment there cross-linking it to this issue; it's a pretty esoteric topic but maybe someone there will be able to look at the image and know right away what the problem is.

Otherwise, any way to debug what's going on as Drupal generates the image and see where the code is breaking the transparency?

A bit shooting in the dark... however this is what could have happened:

1. the GIF images used as overlays (I assume that BOTH the text watermark AND the rounded corners are GIF images, i.e. you are not using the 'rounded corners' effect from imagecache_actions) have their transparent color set out of the palette
2. in that case, image_gd_create_tmp creates a temp truecolor canvass with white background
3. then image_gd_load copies the image from the file onto the canvass - but here transparency is preserved (?) and you end up with white where you expect transparent
4. finally the watermark image is overlayed onto the original image, with full white background

Can you try the patch attached just to test this hypothesis - here we are changing the full white background to full transparency as in fact we are running imagefill on a truecolor image where we have full alpha channel available, and while in memory we do not need to set GIF transparent color.

Can't say if there would be side effects or find a way to develop an automated test ATM, but let's just see if there's any difference.

Please leave to 7.x-dev while we have a clear case as per OP. It should be fixed in 8.0.x-dev first I know, but we do not have a real life example to test there, so let's use the 7.x report to see if we make progress.

@c470ip re:#7

I made those changes to the issue bacause they are needed to make sure
a) that the automated test framework picks the patch and test it (from: 'active' to 'needs review')
b) that the test is executed against the proper development branch (from: '7.37' to '7.x-dev')

and also to flag to the community that a patch is there to be reviewed and tested, to which community members can feedback setting status to 'needs work' or 'RTBC'. Welcome to Drupal's patch review process! See details here if you wish ;)

BTW - @c470ip it would help if you could upload here one of the GIF images you are using as overlays, not just the final result. We could use that to build an automated test that checks colors at different stages of the image object lifecycle. Thanks

@c470ip thanks for sharing the image file. It turns out I was on the right track, but incomplete.

The entire issue: it's not enough to set the GIF transparent color to ensure transparency while images are getting processed in memory by GD.
Since we are creating a truecolor image in image_gd_create_tmp, we need to set alpha channel (to 127) for the color indicated as transparent, plus set the GIF transparent color separately.
If the transparent color is out of the color palette (that was the fix in #375062: imagecolorsforindex() Color index nnn out of range in GDToolkit), still we may 'expect' that the image being loaded would be (partially) transparent while in memory, and only if the image is saved to disk then the missing color index would lead to the transparent parts be replaced by white.

This worked for me locally, please check.

If this proves ok, then we will need to move this issue to the Drupal 8 queue, since bug fixes have to be addressed in the current developing version first, then backported.

EDIT - changed status to major since it's a regression

Please forget about the failing test for the moment. Can you test the patch anyway manually. The test will have to be fixed too in Drupal 8 first.

Thanks for feedback @c470ip, good news :)

We need to fix this in Drupal 8 first, as the problem is there as well. I will work on a patch and tests.

Test-only patch, incorporating a test for the indexed GIF file provided by @c470ip in #10. Should have 2 fails.

The additional test image file broke the sequence of the array returned by drupalGetTestFiles('image'), and ImageFieldDisplayTest. Fixed here, still test-only patch due to have 2 fails.

Full patch. Interdiff from #23: basically the fix is changes to 2 lines of code in CreateNew. The fix in the test is needed because with the change in CreateNew the transparent GIF used in the desaturate test is still transparent (in memory) after the desaturate operation.

Reroll.

One small nit, and a question about a method rename:

1. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
   @@ -271,11 +278,9 @@ function testManipulations() {
   +          // Cannot expect transparency for a GIF that has not.
   

   I don't understand this code comment, and it could probably be rephrased for clarity.

2. +++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
   @@ -436,9 +441,42 @@ public function testResourceDestruction() {
   -   * Tests loading an image whose transparent color index is out of range.
   

   Doesn't this method still also test the out of range bit?

Thanks for review, @jhedstrom!

#33.1 - merged that bit with some checks done a few lines below, and clarified what's going on.

#33.2 - yes it does, that line removed from the method's docblock has been added verbatim a few lines below, where the out-of-range test code is. The method is renamed because now it does more than it was doing before, so I prefer to do that not to confuse. If that's not OK, then I would suggest to have a separate test method here, but I was doing that on the basis to keep the number of test methods down.

This looks good to me. Marking RTBC.

+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -343,6 +343,8 @@ function testImageFieldDefaultImage() {
+    // $images[2] = image-test.gif

We shouldn't add commented out code for this.

Also are all the test changes here necessary to fix the test coverage? Or could some be split to a clean- up issue?

Discussed with @alexpott, @xjm, @effulgentsia and agreed this should be rc target.

#37: adjusted that, reverted non-essential test changes, and removed usage of SafeMarkup::format from the patch.

One comment regarding the changes to ImageFieldDisplayTest. The changes are needed not for functional changes, but only because we introduce a new image file in the simpletest/misc directory for testing purposes. IMHO we have a general issue with how we manage test image files. drupalGetTestFiles('image') returns an array of files sorted by size (if we need to take a specific file for testing, we would need to find it by its size(!)).
The problem appears in any test (like the one here) that is built based on an expected sequence of files: if we introduce a file in within the sequence (because its size is between the sizes of two existing ones), the test breaks as the expected file is not showing up when expected. This is not so visible here, but is strikingly evident in #2377747: Incorrect node create validation error when an invalid image is attached to a field - there we introduce, for testing, an invalid image file of 0 bytes size: that breaks almost all the tests that use drupalGetTestFiles('image') and then get the file with current() or array_shift(), since all of a sudden it will result in fetching a file object for an invalid image. More info in that issue.

Ok, ok.

I think the issues from #37 have been addressed here.

IMHO we have a general issue with how we manage test image files. drupalGetTestFiles('image') returns an array of files sorted by size (if we need to take a specific file for testing, we would need to find it by its size(!)).

Perhaps a follow-up to fix how these tests work?

@jhedstrom

Perhaps a follow-up to fix how these tests work?

there is a proposal for such a fix in #2377747: Incorrect node create validation error when an invalid image is attached to a field

Based on @xjm's post release triage document, committed 12db22c and pushed to 8.0.x and 8.1.x. Thanks!

Working on a D7 patch

Let's see this. It takes a bit more changes than the D8 patch because image_gd_create_tmp was not dealing with image with no transparency set.

testImageFieldDefaultImage() requires the same changes due to sequence of test files as in its D8 equivalent. Also a small change to testGifTransparentImages() to ensure that the resource generated by image_gd_create_tmp() is the one saved to disk.

Missed to add --binary option in git diff for the new test file.

#56 is green.

Reroll after #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in.

#59 works fine for me in the latest D7.