-
-
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
Test instanceof Passport instead of more restrictive SelfValidatingPassport #951
Test instanceof Passport instead of more restrictive SelfValidatingPassport #951
Conversation
3a3fa9a
to
db6b7db
Compare
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.
Seems nice to me.
@@ -240,7 +240,7 @@ protected function loadUser(array $payload, string $identity): UserInterface | |||
|
|||
public function createAuthenticatedToken(PassportInterface $passport, string $firewallName): TokenInterface | |||
{ | |||
if (!$passport instanceof SelfValidatingPassport) { | |||
if (!$passport instanceof Passport) { | |||
throw new \LogicException(sprintf('Expected "%s" but got "%s".', SelfValidatingPassport::class, get_debug_type($passport))); |
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.
You forgot to change this to Passport::class
.
Edit:
Already updated before i write this.
db6b7db
to
147c1fc
Compare
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.
Excellent 👍
@@ -240,8 +240,8 @@ protected function loadUser(array $payload, string $identity): UserInterface | |||
|
|||
public function createAuthenticatedToken(PassportInterface $passport, string $firewallName): TokenInterface |
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.
(*PassportInterface are deprecated in Symfony 5.4)
What about this argument type here?
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 think @chalasr was talking about UserPassportInterface
here, and not the generic PassportInterface
(the * before makes me say this)
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.
Oh.. i didn't thought about it. But I think it is PassportInterface? You can check from here https://github.com/symfony/symfony/blob/5.4/CHANGELOG-5.4.md
Let's wait him for his opinion about this method argument type change.
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.
Of what I've seen the whole createAuthenticatedToken
is going to be deprecated in favor of the simple createToken
method in any case :)
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.
This method must remain the same for compatibility. Both PassportInterface
and createAuthenticatedToken()
are deprecated and removed in Symfony 6.0.
Note that the reasoning for deprecating PassportInterface
is that passports are not an extension point, one should use custom badges instead.
Thanks Tristan. |
…assport