ext/gd: changing signatures for functions returning true

Am 27.05.2025 um 10:00 schrieb Claude Pache claude.pache@gmail.com:

Le 27 mai 2025 à 00:51, David CARLIER devnexen@gmail.com a écrit :
Working through this https://github.com/php/php-src/pull/18651/files, do not mind waiting for PHP 9 if needs be. Let me know what you think.

Those functions are documented to return false on failure (e.g. https://www.php.net/manual/en/function.imagesetthickness.php). For that reason, I typically check for failure (e.g., if (! imagesetthickness(...)) { return false; }).

If some or all functions of the gd library do not or no longer return false, please amend the documentation, so that I (and everyone else, see e.g. https://phpstan.org/r/a69b0ad4-b4bd-4487-a6d1-a436ce142dc2 ) know that those checks are useless without needing to read the php source code.

But don’t just change the return type from bool to void: you are needlessly breaking existing code. Thanks!

I agree. If you want to change this then it should go through a warning phase whenever the return value is not discarded. The engine has means to determine this, similar but opposite of #[\NoDiscard].

OTOH I'm not sure the change is even worth it, maybe just adapt the documentation saying that it always returns true and move on.

The current behavior also allows for constructs like
file_exists($fn) && ($img = imagecreatefrompng($fn)) && imagesetthickness($img, 10) && ...

Not that I'm advocating this usage but it can be short form of chaining things.

Regards,

• Chris

Le 27 mai 2025 à 00:51, David CARLIER devnexen@gmail.com a écrit :

Hi,

Working through this https://github.com/php/php-src/pull/18651/files, do not mind waiting for PHP 9 if needs be. Let me know what you think.

Cheers.

Hi,

Those functions are documented to return false on failure (e.g. https://www.php.net/manual/en/function.imagesetthickness.php). For that reason, I typically check for failure (e.g., if (! imagesetthickness(...)) { return false; }).

If some or all functions of the gd library do not or no longer return false, please amend the documentation, so that I (and everyone else, see e.g. https://phpstan.org/r/a69b0ad4-b4bd-4487-a6d1-a436ce142dc2 ) know that those checks are useless without needing to read the php source code.

But don’t just change the return type from bool to void: you are needlessly breaking existing code. Thanks!

—Claude

Nowadays, it only returns true, which is the initial motivation for that PR.
And yes the documentation is wrong then, which I'm not surprised as it used to be the case it could return false.
I need to go back to working on one of my doc tools that can automatically find and flag those cases.

Note: This was one of the reasons I pushed for adding the true type so that we could find these cases, and have static analysis tools flag them.

However, I definitely think this is a PHP 9 only change, as it changes semantics in surprising ways.

Best regards,
Gina P. Banyard