imagecolorsforindex(): Argument #2 ($color) is out of range

[adrianbj]

Short description of the issue

PHP 8 has changed the behavior of the shutup operator - it no longer silences fatal errors. This means that some old code in ImageSizerEngineGD.php (https://github.com/processwire/processwire/blob/d8945198f4a6a60dab23bd0462e8a6285369dcb9/wire/core/ImageSizerEngineGD.php#L865) now fails with the above error.

Expected behavior

We shouldn't be relying on the shutup operator in the first place :)

Actual behavior

You can test it with this image:

Optional: Suggestion for a possible fix

Replace the above line (865) with this:

$transparentColor = $transparentIndex != -1 ? imagecolorsforindex($image, ($transparentIndex < imagecolorstotal($image) ? $transparentIndex : $transparentIndex - 1)) : 0;

which ensures the $transparentIndex is not greater or equal to the total number of colors in the image.

You can read more about it here: https://stackoverflow.com/questions/3874533/what-could-cause-a-color-index-out-of-range-error-for-imagecolorsforindex

Steps to reproduce the issue

1. Use the above Google logo
2. Attempt to resize it with one of the PW image sizing tools via GD

Setup/Environment

Server Details

Software	Version
ProcessWire	3.0.170
PHP	8.0.0
Webserver	Apache/2.4.46 (Unix)
MySQL Server	5.5.5-10.5.8-MariaDB
MySQL Client	mysqlnd 8.0.0

Server Settings

Parameter	Value
allow_url_fopen	1
max_execution_time	30 (changeable)
max_input_nesting_level	64
max_input_time	60
max_input_vars	1000
memory_limit	128M
post_max_size	50M
upload_max_filesize	2M
xdebug
xdebug.max_nesting_level
mod_rewrite	1
mod_security	*confirmed off
EXIF Support	1
FreeType	1

GD Settings

Parameter	Value
Version	2.3.0
GIF	1
JPG	1
PNG	1
WebP	1

Module Details

Module ClassName	Version
AdminOnSteroids	2.0.21
BatchChildEditor	1.8.23
FieldtypeCombo	0.0.2
FieldtypeTable	0.2.0
FieldtypeTextareas	0.0.8
FileValidatorSvgSanitizer	0.0.3
FormBuilder	0.4.6
InputfieldCombo	0.0.2
InputfieldFormBuilderFile	0.0.2
InputfieldTable	0.2.0
InputfieldTextareas	0.0.7
ModuleReleaseNotes	0.11.1
ModuleSettingsImportExport	0.2.9
PageProtector	2.0.7
ProcessAdminActions	0.8.5
ProcessChildrenCsvExport	1.8.23
ProcessFormBuilder	0.4.6
ProcessPageListerPro	1.1.3
ProcessTableCsvExport	2.0.13
ProcessTracyAdminer	1.1.0
ProcessWireUpgrade	0.0.7
ProcessWireUpgradeCheck	0.0.7
TableCsvImportExport	2.0.13
TracyDebugger	4.21.35

[ryancramerdesign]

@adrianbj That block of code was submitted by someone else (mrx?) and I'm not really sure how it works. Thanks for the suggested fix. @horst-n does this look okay to you?

[horst-n]

@ryancramerdesign Yes, looks good to me!

[teppokoivula]

@adrianbj @ryancramerdesign, I'm still seeing this issue — do you think this should be reopened, or should I open a new issue instead?

One GIF that I can reproduce this with is the first one from https://processwire.com/modules/repeater-easy-sort/. Trying to add this to an image field results in imagecolorsforindex(): Argument #2 ($color) is out of range. Offending line is 866 in /wire/core/ImageSizerEngineGD.php:

865:                // $transparentColor = $transparentIndex != -1 ? @imagecolorsforindex($image, $transparentIndex) : 0;
866:                $transparentColor = $transparentIndex != -1 ? imagecolorsforindex($image, ($transparentIndex < imagecolorstotal($image) ? $transparentIndex : $transparentIndex - 1)) : 0;
867:                if(!empty($transparentColor)) {

Edit: in this case the original value for $transparentIndex is 255, and the ternary used here turns it to 254. When I check the return value of imagecolorstotal($image) it's also 254. Everything seems to work if I use $transparentIndex - 2 instead of $transparentIndex - 1, but I'm not at all sure if that's the correct fix.

Edit 2: quick glance at the issue linked above from wintercms seems to suggest that this indeed requires a more involved fix. Or, alternatively, we should use something along the lines of imagecolorstotal($image) - 1, if that makes any sense... :)

[adrianbj]

The weird thing with that image is that $color is set to 254

Sorry, I just saw your edit where you noted it was converted to 254. Anyway, I did some reading and it seems like the solution is to make use of imagecolorstotal() - see the patch they implemented for animated gifs in Drupal:
https://www.drupal.org/files/375062-47-color-index-out-of-range.patch

[adrianbj]

More info here: https://stackoverflow.com/a/3898007/1524576

[adrianbj]

Ok, I should read before I respond :) - we are already using imagecolorstotal()

[adrianbj]

Not well tested, but this (basically copying the logic from the Drupal patch) works for me:

		} else if($this->imageType == IMAGETYPE_GIF) {
			// @mrx GIF transparency
			$transparentIndex = imagecolortransparent($image);
			// $transparentColor = $transparentIndex != -1 ? @imagecolorsforindex($image, $transparentIndex) : 0;
			//$transparentColor = $transparentIndex != -1 ? imagecolorsforindex($image, ($transparentIndex < imagecolorstotal($image) ? $transparentIndex : $transparentIndex - 1)) : 0;
			if($transparentIndex >= 0 && $transparentIndex < imagecolorstotal($image)) {
				$transparentNew = imagecolorallocate($im, $transparentColor['red'], $transparentColor['green'], $transparentColor['blue']);
				$transparentNewIndex = imagecolortransparent($im, $transparentNew);
				imagefill($im, 0, 0, $transparentNewIndex);
			}

		} else {

[adrianbj]

Ok, I did a little more testing and I think this is actually what we need:

$transparentIndex = imagecolortransparent($image);
if($transparentIndex >= 0 && $transparentIndex < imagecolorstotal($image)) {
    $transparentColor = $transparentIndex != -1 ? imagecolorsforindex($image, ($transparentIndex < imagecolorstotal($image) ? $transparentIndex : $transparentIndex - 1)) : 0;
    if(!empty($transparentColor)) {
        $transparentNew = imagecolorallocate($im, $transparentColor['red'], $transparentColor['green'], $transparentColor['blue']);
        $transparentNewIndex = imagecolortransparent($im, $transparentNew);
        imagefill($im, 0, 0, $transparentNewIndex);
    }
}

This works with the animated gif in @teppokoivula's example and it also works with gifs where a transparent color has been set. The only thing I am confused about is why the thumbnail version of the image isn't actually transparent - is this a bug, or intended behavior? In other words, why does that function proceed to do an imagefill() with the transparent color?

[teppokoivula]

@ryancramerdesign — sorry for the ping, but would be great if this issue could get some attention. I've not tested this properly, but the fix provided by @adrianbj above at least allows me to upload GIFs on my sites.

[ryancramerdesign]

@teppokoivula @adrianbj I've pushed a fix for this based on Adrian's suggestion. It seems to be working here with any GIFs I test with (also including the one mentioned earlier). But I am not very up-to-speed on the details of GD, Horst is really the expert so I'm CCing him here @horst-n . Please let me know if you find any issues. Thanks.

[matjazpotocnik]

@horst-n @adrianbj any comments on Ryan's fix? Is it ok to close this issue?

[adrianbj]

@matjazpotocnik - I haven't tested, but it looks like Ryan's change matches my fix (slightly improved) so should be fine. I guess my only remaining question might be around the lack of transparency of the thumbnail that I noted in my last post above. But I suppose that is different to this issue anyway so we can probably close this.

[matjazpotocnik]

Ok, then I'll close this one. You can reopen as needed.