-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHPStan 1.9.1 #5807
PHPStan 1.9.1 #5807
Conversation
@@ -163,7 +163,6 @@ class Connection | |||
* @param Configuration|null $config The configuration, optional. | |||
* @param EventManager|null $eventManager The event manager, optional. | |||
* @psalm-param Params $params | |||
* @phpstan-param array<string,mixed> $params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPStan fully understands our Params
type now, so we can have it analyze the @psalm-param
annotation.
src/DriverManager.php
Outdated
if ($url === null) { | ||
throw new LogicException(preg_last_error_msg(), preg_last_error()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confident that this case never happens because the regular expressions are analyzed by PHPStan already. But apparently, it is desired that we perform an explicit null
check here. See phpstan/phpstan#1901 (comment) for a discussion on this topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even when I manage to get a Process exited with code 137.
, there will be a string as a return type. I don't know if an evil regex could end up in a return null after some runtime, but I don't see one in our use case. I guess we can live with a null check, but if you're sure about not-null, then maybe an assert
is sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's go with an assert.
34d04dc
to
c6767c6
Compare
c6767c6
to
8c9e2d1
Compare
No description provided.