-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[RFC] Deprecate imagedestroy() #19454
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.
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
Conversation
To avoid merge conflicts, NEWS/UPGRADING will be sent once this is approved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
@@ -629,6 +629,7 @@ function imagegd2(GdImage $image, ?string $file = null, int $chunk_size = 128, i | |||
/** @param resource|string|null $file */ | |||
function imagebmp(GdImage $image, $file = null, bool $compressed = true): bool {} | |||
|
|||
#[\Deprecated(since: '8.5', message: "as it has no effect since PHP 8.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't notice in the RFC, but the consistent wording with the existing functions would be:
#[\Deprecated(since: '8.5', message: "as it has no effect since PHP 8.0")] | |
#[\Deprecated(since: '8.5', message: "as GdImage objects are freed automatically")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we proposed the wording on the RFC, I'd prefer to start with using that message and then we can tweak it in a separate patch if desired; same for the other patches for the no-op deprecations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay with me (that's why I opted just to comment rather than “Request Changes”).
@@ -13,7 +13,7 @@ imagecolortransparent($im, $trans_color); | |||
imagefilledrectangle($im, 0,0, 192,36, $trans_color); | |||
$c = imagecolorat($im, 191,35); | |||
imagegif($im, $dest); | |||
imagedestroy($im); | |||
$im = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$im = null; | |
unset($im); |
I have a feeling that unset()
would capture the intent a little better, but no strong opinion.
RFC: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_imagedestroy