Fix 10431: Add false return type in exec method

[VincentLanglet]

Closes phpstan/phpstan#10431

[staabm]

can exec return false on php8+?

[VincentLanglet]

can exec return false on php8+?

Looking at the issue https://phpstan.org/r/3fe47f5e-f27b-4b4e-9594-5e7e0e6e8313, currently
Phpstan thinks exec return false on php 8+ et not on php 8-.

Looking at https://www.php.net/manual/en/function.exec.php there is

If command is empty or contains null bytes, exec() now throws a ValueError. Previously it emitted an E_WARNING and returned false.

So I think it doesn't return false on php8.

But the fix seems to be different, since we rely on phpstan/php-8 stubs
https://github.com/phpstan/php-8-stubs/blob/1224976cd9276e6810865ae7df9c591dcc871a90/stubs/ext/standard/exec.php#L9
which, I think, is generated from php-src
https://github.com/php/php-src/blob/47b6eabe37fcb0cd0dd8608648e1fc075bdb7c73/ext/standard/basic_functions.stub.php#L2718

[staabm]

it can return false, see php/php-src#13739

[VincentLanglet]

it can return false, see php/php-src#13739

So, there is nothing to change on PHP8.

But we still need to add |false for PHP7. @ondrejmirtes I don't understand why you closed this ?
The PR is not in contradiction with @staabm intervention. Or am I missing something ?

[staabm]

Right. The PR seems valid to adjust php7 types

[ondrejmirtes]

Sorry, thanks 😊

[thg2k]

Just to make sure we are on the same page, we are talking about the case where fork(2) would fail, not when you exec() a non-existing file or it gives a non-zero exit status. So checking for false is a bit pointless, as it mostly can occur on in out-of-memory situation (so the php process would terminate anyway) or in setups where the kernel denies the fork, which are still quite unlikely.

Shouldn't this be a benevolent union type? I.e. it does not error if you do check for false, but it does not complain in case you do not?

[staabm]

@thg2k I think your reasoning might be true for php8+ (which did not change with this PR), but on php7 - which this PR is changing the return type for - false is returned for all error situations

[thg2k]

What do you mean with "all error situations"? AFAIK, the only difference for exec between PHP7 and PHP8 is the TypeError exception, which is true for every PHP builtin function, i.e. passing array() where string (command name) is expected. But then most of PHP builtins can return false, so this argument does not apply in any special way to exec().

Anyway, I was just wondering.. after all the offiicial prototype includes false so I guess it's OK. I've just always found the false return from exec() extremely misleading as I think most newbies will expect it to be related with the successfull execution of the program, which is very wrong. But this is offtopic and should be brought with php-src.

[staabm]

the php.net manual says

PHP8+: If command is empty or contains null bytes, exec() now throws a ValueError. Previously it emitted an E_WARNING and returned false.

so I think e.g. exec("") would return false on php7