Fix PHP 8.x & imagedestroy#196
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the GD adapter to avoid calling imagedestroy() when running on PHP 8.x, presumably to address PHP 8 compatibility issues related to GD image destruction.
Changes:
- Added
PHP_VERSION_ID < 80000guards around severalimagedestroy()calls in the GD adapter. - Affected cleanup paths in background fill, resize, crop, gaussian blur, and flip operations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (PHP_VERSION_ID < 80000) { | ||
| imagedestroy($this->resource); | ||
| } |
There was a problem hiding this comment.
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.
| if (PHP_VERSION_ID < 80000) { | ||
| imagedestroy($this->resource); | ||
| } |
There was a problem hiding this comment.
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.
| if (PHP_VERSION_ID < 80000) { | ||
| imagedestroy($this->resource); | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| // clean up | ||
| imagedestroy($prevImage); | ||
| if (PHP_VERSION_ID < 80000) { |
There was a problem hiding this comment.
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).
| if (PHP_VERSION_ID < 80000) { | |
| if ( | |
| $prevImage !== $this->resource | |
| && ( | |
| is_resource($prevImage) | |
| || (class_exists('\\GdImage') && $prevImage instanceof \GdImage) | |
| ) | |
| ) { |
There was a problem hiding this comment.
What about using destroy in PHP 8.5 and above?
| if (PHP_VERSION_ID < 80000) { | ||
| imagedestroy($this->resource); | ||
| } |
There was a problem hiding this comment.
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).
No description provided.