Invalid empty() function handling

[ethernidee]

Bug report

$timeLimit = ini_get('max_execution_time');
empty($timeLimit) && $timeLimit !== '0' and $timeLimit = 30;

ini_get returns string|false
empty checks for '0', '' and false.
empty($timeLimit) && $timeLimit !== '0' is the same as $timeLimit === '' || $timeLimit === false

Got irrelevant errors:

Result of && is always false.
Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your configuration file.
booleanAnd.alwaysFalse

Strict comparison using !== between '0' and '0' will always evaluate to false.
notIdentical.alwaysFalse

Code snippet that reproduces the problem

https://phpstan.org/r/b8833c6e-bfe6-4cdd-98e5-6db9d8fba38e

Expected output

No error

Did PHPStan help you today? Did it make you happy in any way?

Hundreds of false positives

[rvanvelzen]

ini_get('max_execution_time') will always return a numeric-string, so inside empty($timeLimit) it must be '0'. Either empty($timeLimit) or $timeLimit !== '0' is enough.

[ondrejmirtes]

Thanks

[ethernidee]

Sorry, but that's simply not true. It will return any string you use in ini file or call to ini_set.

php > ini_set('max_execution_time', 'G');
php > var_dump(ini_get('max_execution_time'));
string(1) "G"

php > var_dump(ini_get('removed_option'));
bool(false)

The signature of a function is ini_get(string $option): string|false and it should be honored.

[rvanvelzen]

Thanks - for some reason I thought that ini_set() also normalized the value. Of course, if you only modify this value via set_time_limit() it will be a numeric string, but that's not a guarantee.

@ondrejmirtes @staabm the IniGetReturnTypeExtension will probably need to be modified to only return string for keys that are guaranteed to exist. Or perhaps a new configuration setting that allows returning numeric-string in these cases and a rule that forbids ini_set() with an invalid value?

[ondrejmirtes]

I think the extension already behaves like that. Some keys will never return false and the extension knows that.

[rvanvelzen]

That's not the point I was making - it should not return numeric-string by default because as shown above that is not true: https://3v4l.org/NkUSb

[staabm]

hmm I wonder whether we should trust the developer to use proper php.ini settings.
sure this can be set with invalid values, but this might lead to more severe problems then phpstan errors.

I think we should additionally validate the setter call.
currently we are missing a error when ini_set('max_execution_time', 'G'); is used with a invalid string.
https://phpstan.org/r/ce2a950f-1c90-4c4e-ac44-b3c61cb659f2

[staabm]

The signature of a function is ini_get(string $option): string|false and it should be honored.

we narrow to numeric-string only for a very few special keys.
https://github.com/phpstan/phpstan-src/blob/b74b6d984be33a9af9cddd4a505a51094f8757a5/src/Type/Php/IniGetReturnTypeExtension.php#L42-L45

all other keys use string|false.