Skip to content

Fix PHP 8.x & imagedestroy #196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manusfreedom wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −5
Open

Fix PHP 8.x & imagedestroy #196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions Adapter/GD.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ public function fillBackground($background = 0xffffff)
$n = imagecreatetruecolor($w, $h);
imagefill($n, 0, 0, ImageColor::gdAllocate($this->resource, $background));
imagecopyresampled($n, $this->resource, 0, 0, 0, 0, $w, $h, $w, $h);
imagedestroy($this->resource);
if (PHP_VERSION_ID < 80000) {
imagedestroy($this->resource);
}
Comment on lines +64 to +66
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PHP version gate around imagedestroy() is likely the wrong criterion here: in PHP 8+ imagedestroy() still exists and accepts GdImage, so skipping it based solely on PHP_VERSION_ID changes cleanup behavior without guaranteeing the underlying issue is addressed. If the goal is to avoid type errors / invalid-handle destroys, guard on the actual value (e.g., only call when the variable is a GD image resource / GdImage), and consider centralizing this into a small helper to avoid duplicating the same version check in multiple methods.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@manusfreedom manusfreedom Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the doc: https://www.php.net/manual/en/function.imagedestroy.php

$this->resource = $n;

return $this;
Expand Down Expand Up @@ -101,7 +103,9 @@ protected function doResize($bg, int $target_width, int $target_height, int $new
$height
);

imagedestroy($this->resource);
if (PHP_VERSION_ID < 80000) {
imagedestroy($this->resource);
}
Comment on lines +106 to +108
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above: using PHP_VERSION_ID < 80000 to decide whether to call imagedestroy() is brittle and duplicates logic. Prefer a single helper that safely destroys only when the value is a valid GD image (resource on PHP<8, GdImage on PHP>=8), rather than keying off the runtime version in each call site.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@manusfreedom manusfreedom Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the doc: https://www.php.net/manual/en/function.imagedestroy.php


$this->resource = $n;

Expand All @@ -117,7 +121,9 @@ public function crop($x, $y, $width, $height)
imagealphablending($destination, false);
imagesavealpha($destination, true);
imagecopy($destination, $this->resource, 0, 0, (int) $x, (int) $y, $this->width(), $this->height());
imagedestroy($this->resource);
if (PHP_VERSION_ID < 80000) {
imagedestroy($this->resource);
}
Comment on lines +124 to +126
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional cleanup is duplicated across several methods and is based on PHP version rather than the actual type/value being destroyed. Consider replacing this with a single safe-destroy helper (destroy if resource or GdImage) so behavior is consistent across PHP versions and the intent is clearer.

Copilot uses AI. Check for mistakes.
$this->resource = $destination;

return $this;
Expand Down Expand Up @@ -268,7 +274,9 @@ public function gaussianBlur($blurFactor = 1)
imagefilter($this->resource, IMG_FILTER_GAUSSIAN_BLUR);

// clean up
imagedestroy($prevImage);
if (PHP_VERSION_ID < 80000) {
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PHP 8+, this now skips destroying the temporary image held in $prevImage. If the intent is to avoid errors, it would be safer to destroy based on whether $prevImage is actually a GD image (resource/GdImage) rather than PHP version, and keep cleanup consistent across runtimes (especially for long-running CLI processes where prompt freeing can matter).

Suggested change
if (PHP_VERSION_ID < 80000) {
if (
$prevImage !== $this->resource
&& (
is_resource($prevImage)
|| (class_exists('\\GdImage') && $prevImage instanceof \GdImage)
)
) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@basteyy basteyy Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using destroy in PHP 8.5 and above?

Copy link
Copy Markdown
Author

@manusfreedom manusfreedom Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy ? We could perhaps use unset(), but I don't think it's necessary: https://stackoverflow.com/questions/79522994/in-php-8-how-to-clear-image-data-from-script-previously-imagedestroy

imagedestroy($prevImage);
}

return $this;
}
Expand Down Expand Up @@ -480,7 +488,9 @@ public function flip($flipVertical, $flipHorizontal)
imagesavealpha($imgdest, true);

if (imagecopyresampled($imgdest, $this->resource, 0, 0, $src_x, $src_y, $width, $height, $src_width, $src_height)) {
imagedestroy($this->resource);
if (PHP_VERSION_ID < 80000) {
imagedestroy($this->resource);
}
Comment on lines +491 to +493
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces another PHP-version-based special case for resource cleanup. To reduce duplication and avoid tying behavior to a hardcoded version threshold, prefer a shared helper that safely calls imagedestroy() only when the variable is a valid GD image (resource/GdImage).

Copilot uses AI. Check for mistakes.
$this->resource = $imgdest;
}
}
Expand Down
Loading