Problem/Motivation

While investigating why my patch for issue #1480428: GD image toolkit does not correctly handle gamma when resizing images. did not pass, I stumbled upon a few bad tests in image.test

The tests usually check if all four corners of a test image are of the expected colours. However, only the last result is actually used in the assert call, which means that for every test, only the bottom-left corner is tested. And this corner is transparent, which means it does not even have the same problems as the other corners.

After correcting this several tests fail, either because of genuine bugs which were not tested or because of problems in these tests which were not visible until now.

Proposed resolution

The proposed patch moves the assert call inside the foreach loop that checks every corner, corrects three tests that were expecting wrong values: fuschia instead of transparent for rotation, black instead of light grey for yellow desaturation, and white instead of light grey for yellow desaturation in GIF.

The problem with GIF desaturation was due to a bug in the GD toolkit, where GIFs were desaturated without being reindexed. This bug was attributed to a problem with transparency because only the last corner was checked, while it was in fact also present on other colours (which was made visible by fixing the tests).

Therefore my patch also fixes the image_gd_desaturate() function in core/modules/system/image.gd.inc.

Files: 
CommentFileSizeAuthor
#30 drupal8.image-system.1579810-30.patch5.11 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 58,973 pass(es).
[ View ]
#26 string-format-1579810-26.patch5.63 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 59,087 pass(es).
[ View ]
#22 string-format-1579810-22.patch5.62 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,779 pass(es).
[ View ]
#18 drupal8.image-system.1579810-18.patch753 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]
#15 gd_tests_do_not_test_check_every_case-1579810.patch2.88 KBSeeSchloss
PASSED: [[SimpleTest]]: [MySQL] 42,818 pass(es).
[ View ]
#8 gd_tests_dont_work-1579810-8.patch5.77 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,140 pass(es).
[ View ]
#5 drupal-imagegd-fixes.diff5.76 KBSeeSchloss
PASSED: [[SimpleTest]]: [MySQL] 36,798 pass(es).
[ View ]
#3 drupal-imagegd-fixes.diff5.05 KBSeeSchloss
PASSED: [[SimpleTest]]: [MySQL] 36,726 pass(es).
[ View ]
drupal-imagegd-fixes.diff5.05 KBSeeSchloss
PASSED: [[SimpleTest]]: [MySQL] 36,679 pass(es).
[ View ]

Comments

Well, any comment on this?

Status:Needs review» Needs work

+++ b/core/modules/system/image.gd.incundefined
@@ -206,6 +206,12 @@ function image_gd_desaturate(stdClass $image) {
+  // GIF images have to be reindexed
+  if ($image->info['extension'] == 'gif') {

Comments need to end with a ".", see http://drupal.org/node/1354#inline.

Status:Needs work» Needs review
StatusFileSize
new5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 36,726 pass(es).
[ View ]

OK thank you, here is a revised version.

Status:Needs review» Needs work

Nice job fixing the tests!

The fix in image_gd_desaturate(), on the other hand, does look like a one-off hack. Applying filters (resize, color filters, etc.) to an indexed image doesn't really make sense, so we should just convert those images to truecolor in image_gd_load().

There is an example code on how to do that on the imageistruecolor() documentation:

<?php
if (!imageistruecolor($image->resource)) {
 
// Convert indexed images to true color, so that filters work
  // correctly and don't result in unnecessary dither.
 
$new_image = imagecreatetruecolor(imagesx($image->resource), imagesy($image->resource));
 
imagecopy($new_image, $im, 0, 0, 0, 0, imagesx($image->resource), imagesy($image->resource));
 
imagedestroy($image->resource);
 
$image->resource = $new_image;
}
?>

Status:Needs work» Needs review
StatusFileSize
new5.76 KB
PASSED: [[SimpleTest]]: [MySQL] 36,798 pass(es).
[ View ]

Thanks Damien, here is a new version doing the conversion at load time.

(rather than imagecreatetruecolor() I'm using image_gd_create_tmp() which takes care of transparency)

Status:Needs review» Reviewed & tested by the community

Really nice. I'm buying that.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:+needs backport to D7

Looks good to me too. Committed/pushed to 8.x.

StatusFileSize
new5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 39,140 pass(es).
[ View ]

Missing the GD library on this system, so can't test patch.

Status:Patch (to be ported)» Needs review

Thanks for the backport (and sorry for the delay), I just tested it and it works just fine for me.

No worries. If you feel like it's RTBC, don't forget to change the status :)

Status:Needs review» Reviewed & tested by the community

Yes, yes !

Status:Reviewed & tested by the community» Needs work

Assert messages don't use t() function, use nothing or format_string() if it's needed.

+ $this->assertTrue($correct_colors, t('Image %file object after %action action has the correct color placement at corner %corner.', array('%file' => $file, '%action' => $op, '%corner' => $key)));

+ $this->assertTrue($image_truecolor, t('Image %file after load is a truecolor image.', array('%file' => $file)));

...

This should be:

+ $this->assertTrue($correct_colors, format_string('Image %file object after %action action has the correct color placement at corner %corner.', array('%file' => $file, '%action' => $op, '%corner' => $key)));

+ $this->assertTrue($image_truecolor, format_string('Image %file after load is a truecolor image.', array('%file' => $file)));

...

Version:7.x-dev» 8.x-dev

This was a direct port from what was committed in 8.x. So if what you're saying is the case, we would need to fix this first in 8.x.

StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 42,818 pass(es).
[ View ]

Woops, I had not seen this was still going on.

Here's a patch that replaces all the t() calls.

Status:Needs work» Needs review

Updating status

Status:Needs review» Needs work
Issue tags:+Needs reroll

The patch from #15 doesn't apply anymore.

$ curl https://drupal.org/files/gd_tests_do_not_test_check_every_case-1579810.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2950  100  2950    0     0   1697      0  0:00:01  0:00:01 --:--:--  2111
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.php:206
error: core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.php: patch does not apply

Can you reroll?

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new753 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]

All but one of these items were resolved in #1797410: Remove t() from asserts for G-L includes system tests

Status:Needs review» Reviewed & tested by the community

True. Thanks for the patch.

Title:GD tests do not test check every caseRemove t() from ToolkitGdTest

Status:Reviewed & tested by the community» Needs work

In drupal 8 use the new OO aproach:

In the top of the file:

use Drupal\Component\Utility\String;

In code:

String::format('Could not load image %file.', array('%file' => $file));

Title:Remove t() from ToolkitGdTestRemove t() and format_string() from toolkit tests
Assigned:Unassigned» claudiu.cristea
Priority:Normal» Minor
Status:Needs work» Needs review
StatusFileSize
new5.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,779 pass(es).
[ View ]

You're right. Here's a patch for all toolkit tests. Setting also "minor" as priority.

Category:bug» task

Setting also the category to "task".

Title:Remove t() and format_string() from toolkit testsGD tests do not test check every case and code cleanup
Category:task» bug
Status:Needs review» Reviewed & tested by the community

I have restored the original title for the bug traceability, and this is a followup code cleanup.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new5.63 KB
PASSED: [[SimpleTest]]: [MySQL] 59,087 pass(es).
[ View ]

Rerolled.

@oriol_e9g, I rerolled the patch, nothing changed. Can you re-RTBC? Thanks!

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new5.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,973 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community

Go again.

Issue tags:-Needs reroll

.

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

Committed 3992f9b with the message Issue followup #1579810 by SeeSchloss, Albert Volkman, claudiu.cristea: Remove t() and format_string() from toolkit tests. and pushed to 8.x. Thanks!

Back to backporting the original issue as was done in #8!

Issue summary:View changes

small precision added