Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] DeprecatePassportInterface#42198
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
PassportInterfacePassportInterfacePassportInterfacePassportInterfaceUh oh!
There was an error while loading.Please reload this page.
PassportInterfacePassportInterface| * @return Passport | ||
| */ | ||
| publicfunctionauthenticate(Request$request):PassportInterface; | ||
| publicfunctionauthenticate(Request$request);/*: 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.
Well, maybe we should keep the existing return type while documenting the new one, instead of widening the explicit return type now (the new one is only documented anyway).
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.
👍 if theDebugClassLoader still correctly reports the deprecation.
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.
Keeping the return type would forbid upgrading userland authenticators on PHP versions prior to 7.4https://3v4l.org/LcLRN#veol
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 don't see much issue in requiring a user to be on PHP 7.4 to fix a deprecation (i.e. it's not breaking support for older versions, it's just triggering a deprecation if you stay on: PassportInterface). They would need PHP 8 anyways if they want to upgrade to Symfony 6 and even PHP 7.4 will be eom when we release Symfony 5.4.
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.
Pushing our users to upgrade their PHP versions has never been our take, at least not by making symfony upgrades harder. Moreover, that would break our CI on 7.2 given that core authenticators have been upgraded.
e2dfd5f todb572e3ComparePassportInterfacePassportInterfacechalasr commentedJul 24, 2021
Renamed PR is ready. |
src/Symfony/Component/Security/Http/Authenticator/AuthenticatorInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedAug 6, 2021
Thank you@chalasr. |
Uh oh!
There was an error while loading.Please reload this page.
As explained in#42181, the right extension point is badges, not passports.
Also renames
AuthenticatorInterface::createAuthenticatedToken()tocreateToken()because of the signature change and the recent abandon of theauthenticatedstate for tokens.