Skip to content
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

Closed
tarlepp opened this issue Nov 2, 2021 · 10 comments
Closed

Psalm issue after update to 2.14.1 #944

tarlepp opened this issue Nov 2, 2021 · 10 comments

Comments

@tarlepp
Copy link

tarlepp commented Nov 2, 2021

Noticed that Psalm complains following;

psalm: MissingDependency: Lexik\Bundle\JWTAuthenticationBundle\Response\JWTAuthenticationFailureResponse depends on class or interface lexik\bundle\jwtauthenticationbundle\response\jwtcompatauthenticationfailureresponse that does not exist

Related code;

public function onAuthenticationFailure(Request $request, AuthenticationException $exception): Response
{
    $event = new AuthenticationFailureEvent(
        $exception,
        new JWTAuthenticationFailureResponse(
            $this->translator->trans('Invalid credentials.', [], 'security')
        )
    );

    $this->dispatcher->dispatch($event);

    return $event->getResponse();
}

Although everything works as expected - Should I raise an issue to Psalm repository about this?

@chalasr
Copy link
Collaborator

chalasr commented Nov 2, 2021

Thanks for the report. This is due to a compatibility layer implying some ugly hacks, I don't think psalm should care.
I'll try to fix that here.

@tarlepp
Copy link
Author

tarlepp commented Nov 2, 2021

@chalasr awesome! And really big thanks for quick replies on issues and PR's to fix those!

@tarlepp
Copy link
Author

tarlepp commented Apr 26, 2022

And just noticed that after updating PHPStan to version 1.6.0 it also complains at this.

phpstan: Parameter #2 $response of class Lexik\Bundle\JWTAuthenticationBundle\Event\AuthenticationFailureEvent constructor expects Symfony\Component\HttpFoundation\Response, Lexik\Bundle\JWTAuthenticationBundle\Response\JWTAuthenticationFailureResponse given.

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();
}

@chalasr
Copy link
Collaborator

chalasr commented Apr 30, 2022

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

@ondrejmirtes
Copy link

The problem isn't in eval - the problem is that Lexik\Bundle\JWTAuthenticationBundle\Response\JWTCompatAuthenticationFailureResponse isn't defined acording to PSR-4 rules configured in that package's composer.json. If you put it to a separate file with the right filename, it's going to work.

GErpeldinger added a commit to GErpeldinger/LexikJWTAuthenticationBundle that referenced this issue May 3, 2022
@GErpeldinger
Copy link
Contributor

Oh yeah, separate JWTCompatAuthenticationFailureResponse from JWTAuthenticationFailureResponse resolve the problem on phpstan, thanks ! But it doesn't seem to solve it on psalm unfortunately.

@GErpeldinger
Copy link
Contributor

After some tests, it seems that psalm does not appreciate the eval(), is it really necessary?

GErpeldinger added a commit to GErpeldinger/LexikJWTAuthenticationBundle that referenced this issue May 3, 2022
GErpeldinger added a commit to GErpeldinger/LexikJWTAuthenticationBundle that referenced this issue May 3, 2022
@GErpeldinger
Copy link
Contributor

GErpeldinger commented May 3, 2022

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 if(true && true"), it returns me the error:

ERROR: MissingDependency - Lexik\Bundle\JWTAuthenticationBundle\Response\JWTAuthenticationFailureResponse depends on class or interface lexik\bundle\jwtauthenticationbundle\response\jwtcompatauthenticationfailureresponse that does not exist (see https://psalm.dev/157)

But just if(true) is good...

Also if i modify the && to AND, it works...

@GErpeldinger
Copy link
Contributor

GErpeldinger commented May 3, 2022

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 && to AND?

@ondrejmirtes
Copy link

I managed to make PHPStan discover Lexik\Bundle\JWTAuthenticationBundle\Response\JWTCompatAuthenticationFailureResponse even with the current release of lexik/jwt-authentication-bundle: phpstan/phpstan-src@e40474b

GErpeldinger added a commit to GErpeldinger/LexikJWTAuthenticationBundle that referenced this issue May 5, 2022
chalasr added a commit that referenced this issue May 23, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants