search

php / php-sdk-binary-tools

Tool kit for building PHP under Windows
BSD 2-Clause "Simplified" License
85 stars 32 forks source link

Added forward compatibility for PHP 8.1+ #13

Open Bilge opened 5 months ago

Bilge commented 5 months ago

When the shell_exec call fails, it returns null. Passing null to trim(), which expects a string, is deprecated since PHP 8.1. I understand we're bundling PHP 8.0 at present, but this is just a forward compatibility change for when it's upgraded later.

As a bonus, removed some trailing whitespace.

cmb69 commented 1 month ago

Good catch, thank you! Indeed we should update the bundled PHP version (and likely some other stuff, such as the MinGW tools), but only after having tagged a new release.

cmb69 commented 1 month ago

Passing null to trim(), which expects a string, is deprecated since PHP 8.1.

Right. I've seen that same trim(shell_exec(…)) call a couple of lines below, and that would need to be fixed as well (to be on the safe side for PHP 9). And then I noticed that Config::guessCurrentBranchName() returns ?string, but the result is passed to Config::setCurrentBranchName() which expects string here:

https://github.com/php/php-sdk-binary-tools/blob/8a4ca6589c47a0a5bc07202075ff08c28e0d91f2/lib/php/libsdk/SDK/Config.php#L250-L251

That's probably even worse, since that fatals, if there is no git.exe in the PATH, and there would be something wrong with php_version.h.

I expect many more of such issues, and I don't think addressing this piecemeal makes much sense. Maybe we should employ some static analysis tool, but which one (PHPStan, Psalm, or something else)? Any preferences?