Phpstan fixes #204
Phpstan fixes #204
Conversation
PreviousUrlHelper says it returns a string, so return '' if there was no session.
| @@ -131,6 +132,7 @@ public function fetchUserFromToken(AccessToken $accessToken) | |||
| */ | |||
| public function fetchUser() | |||
| { | |||
| /** @var AccessToken $token */ | |||
There was a problem hiding this comment.
Why is this needed if the method has @return on it?
There was a problem hiding this comment.
Even with the |AccessToken, the AbstractProvider::getAccessToken that OAuth2Client::getAccessToken calls still only returns an AccessTokenInterface. However, OAuth2Client::fetchUserFromToken as well as the AbstractProvider::getResourceOwner that it calls, take the hard classes.
In 7.4 the types can be wider/narrower where it makes sense, but in the meantime, we have to suggest to PHPStan that it's it's also the hard class that fetchUserFromToken expects.
There was a problem hiding this comment.
Yea, that's a mess. I guess checking if it's not an instance of AccessToken and throwing an exception is more honest?
There was a problem hiding this comment.
It could only be one of those two, depending on the version and so if the interface was available (and described only in the docblock return types). The tree of code it runs from that point should only be methods described by the Interface, which is perfectly handled by the AccessToken class anyway.
So, the comment does for PHPStan, what PHP 7.4 would be able to do for itself - widening the acceptable type.
Rordering the condition allows for falling through to throw the exception on error.
`fetchUserFromToken()` wants the hard class, not the interface. Leave those changes for later, by using a docblock @var notation to tweak the interface down to the hard-class it describes
| 'guard.finish_registration.user_information', | ||
| $e->getUserInformation() | ||
| ); | ||
| } |
There was a problem hiding this comment.
What if not? Probably better to throw an exception else? And the same in a few other places. Wdyt?
There was a problem hiding this comment.
indeed - OAuth2Client::getSession() will throw a \LogicException if there is no session - and it's 'stateless''. Should a session be required everywhere - and how much stateless-ness is needed?
There was a problem hiding this comment.
Hmm, in this case, because the user would be calling this method directly (and the method is called saveUserInfoToSession()), if we're unable to actually save to the session because there is none, I think an exception is a good idea
There was a problem hiding this comment.
Taking a look in the Request class, Symfony 4.1 deprecated calling getSession() if there one didn't exist - it's been triggering a notice since then, and 5.0 will throw a BadMethodCallException. Till 4.4 or so though, it was however, still marked as potentially returning null.
I'll make a friendlier error message instead for all the checks with a if (!$request->hasSession()) { throw .... }. ThehasSession() method was in for our minimum-supported version of 3.4.
Check with `hasSession()` (since 3.4), to provide a friendlier message. Until Symfony v4.4, `getSession()` is also marked as @return ...|null, so confirm with an additional check of `instanceof SessionInterface`.
|
Thank you @alister! :) |
A group of individual commits each fixing one distinct issue.
They are: