Skip to content

Conversation

tscni
Copy link
Contributor

@tscni tscni commented Aug 23, 2024

I got annoyed by the failing e2e/php8 test (https://github.com/phpstan/phpstan/blob/1.12.x/e2e/php8/test.php#L11, https://phpstan.org/r/32a5c25f-c266-4c04-9bf1-2ec0562397d8), so I wanted to resolve it by restoring the expected issue there. (But only now do I notice that this doesn't help that at all - I completely misunderstood the issue there :P)

Following, phpstan/phpstan#1274, phpstan/phpstan#1465, phpstan/phpstan#1484 I looked into curl_init() a bit more.

Here's my analysis: (with verified behavior across all latest PHP minor versions and libcurl 7.10.5 + 8.9.1)
One important point to start with, if we presume that curl_init() without arguments never returns false, we generally ignore initialization errors or the case where curl fails to allocate more memory.

@tscni tscni force-pushed the feature/improve-curl-init-return-type branch from 909c3ac to 3326f37 Compare August 23, 2024 17:04
Comment on lines 56 to 61
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit cumbersome to do, but I couldn't find a simpler way to get the constant value of a string.
Maybe a Type::getConstantStringValue(): ?string would be useful. But then that might lead to Type exploding with helper functions eventually.

Copy link
Contributor

@staabm staabm Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Type::getConstantStrings work for you?
(And you then check all strings, not just the first)

Testcase:

$url = 'example.org'; If (rand(0,1)) $url = 'example.com'; curl_init($url); 
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I missed that method. But for multiple constant values it turned out to not be helpful. I overlooked that possibility completely.
Thanks a lot!

@tscni tscni force-pushed the feature/improve-curl-init-return-type branch 5 times, most recently from 51eecd8 to 1491b5d Compare August 24, 2024 15:13
@tscni tscni force-pushed the feature/improve-curl-init-return-type branch from 1491b5d to 2d47da6 Compare August 24, 2024 15:28
// https://github.com/php/php-src/blob/php-8.0.0/ext/curl/interface.c#L104-L107
return new NeverType();
}
if ($this->phpVersion->getVersionId() < 80000) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a new PHPVersion method

Copy link
Contributor Author

@tscni tscni Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (isCurloptUrlCheckingFileSchemeWithOpenBasedir())

@tscni tscni force-pushed the feature/improve-curl-init-return-type branch from f1498e5 to 498f2c0 Compare August 24, 2024 16:52
@ondrejmirtes
Copy link
Member

Hi, thank you very much! This is way too much research effort for such a simple function, but I'm grateful for it. Typically I'd solve this with a benevolent union type (decreases number of false positives, does not increase the number of reported errors). (More info here: https://phpstan.org/config-reference#checkbenevolentuniontypes)

@ondrejmirtes ondrejmirtes merged commit 347ceff into phpstan:1.11.x Aug 25, 2024
@tscni tscni deleted the feature/improve-curl-init-return-type branch August 25, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants