-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
Psalm issue after update to 2.14.1
#944
Comments
Thanks for the report. This is due to a compatibility layer implying some ugly hacks, I don't think psalm should care. |
@chalasr awesome! And really big thanks for quick replies on issues and PR's to fix those! |
And just noticed that after updating PHPStan to version 1.6.0 it also complains at this.
So currently I'm using following to "fix" this: public function onAuthenticationFailure(Request $request, AuthenticationException $exception): Response
{
/**
* @psalm-suppress MissingDependency, InvalidArgument
*/
$event = new AuthenticationFailureEvent(
$exception,
new JWTAuthenticationFailureResponse( // @phpstan-ignore-line
$this->translator->trans('Invalid credentials.', [], 'security')
)
);
$this->dispatcher->dispatch($event);
return $event->getResponse();
} |
I wouldn't have expected that, but our hack-ish BC layers may be handled by PHPStan phpstan/phpstan#7157. Let's see how it goes and eventually propose the same for psalm |
The problem isn't in |
Oh yeah, separate JWTCompatAuthenticationFailureResponse from JWTAuthenticationFailureResponse resolve the problem on phpstan, thanks ! But it doesn't seem to solve it on psalm unfortunately. |
After some tests, it seems that psalm does not appreciate the eval(), is it really necessary? |
I have an oddity, psalm passes without any problem when I remove one of the two conditions of the "if" or the eval(). I tried to put
But just Also if i modify the |
Ok, I don't have this error before psalm 4.8, it seems that in this version or a minor version of 4.7 that introduced the problem. Update: Ok it's the minor version 4.7.1 who introduced the problem, gonna open an issue. What to do in the meantime, change the |
I managed to make PHPStan discover |
…se (GErpeldinger) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Fix #944: Separate CompatFailureResponse from FailureResponse Hello, Following ondrejmirtes' feedback on issue #944, here is a little fix that might do the trick. It seems that just separating the classes is not enough for psalm, and that you have to blow up the eval(), which doesn't seem to me to bring any particular difficulty? update: Oh my bad, it makes the tests fail in 7.1 & 7.4 Commits ------- de9f7d0 Fix #944: Separate CompatFailureResponse from FailureResponse
Noticed that Psalm complains following;
Related code;
Although everything works as expected - Should I raise an issue to Psalm repository about this?
The text was updated successfully, but these errors were encountered: