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] Deprecate returning stringish objects from Security::getUser#27943
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
ro0NL commentedJul 13, 2018
Im also wondering if it's better to throw, rather then silently ignoring "non users" by returning null. To clarify the intend here. |
xabbuh left a comment
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.
a test would be nice
stof commentedJul 13, 2018
well, throwing for anonymous token would be a big BC break, and would make this API much less developer-friendly |
ro0NL commentedJul 13, 2018
You're right. On 2nd thought i think the null default is fine indeed. There's no UserInterface available which is what we asked for, but it doesn't mean you're "logged out" per se in case of null. |
weaverryan commentedJul 13, 2018
I’m following the logic. But, was this to solve some specific issue? It’s looks like a BC break to me, unfortunately. |
ro0NL commentedJul 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We had some discussion on slack about the inconsistency between the documented But no real issue :) it will just crash if this class is (ever) updated to have type declarations (#27944). To me that smells a violation. And yes, we can do an upgrade path with deprecations but i also think it qualifies a bugfix (otherwise the bugfix would be to update to |
weaverryan commentedJul 14, 2018
Bug fix: that’s a good argument for the behavior change. I’d still prefer a deprecation and a merge into 4.2... after all, the bug isn’t currently hurting any functionality. So, waiting until 4.2 does not cause any issues. |
chalasr commentedJul 14, 2018
+1 for a BC layer |
ro0NL commentedJul 15, 2018
Done. |
| } | ||
| if (!$userinstanceof UserInterface) { | ||
| @trigger_error(sprintf('Accessing the user object "%s" that is not an instance of "%s" from "%s()" is deprecated since Symfony 4.2. Use "getToken()->getUser()" instead.',get_class($user), UserInterface::class,__METHOD__),E_USER_DEPRECATED); |
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.
...is deprecated since Symfony 4.2, use...
weaverryan left a comment
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.
Changelog entries could use a period at the end, but just that nit pick :)
| } | ||
| if (!$userinstanceof UserInterface) { | ||
| @trigger_error(sprintf('Accessing the user object "%s" that is not an instance of "%s" from "%s()" is deprecated since Symfony 4.2, use "getToken()->getUser()" instead.',get_class($user), UserInterface::class,__METHOD__),E_USER_DEPRECATED); |
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.
$user may not be an object here:is_object($user) ? get_class($user) : $user
Ah no, we check that just before.
lyrixx commentedJul 16, 2018
I like the idea 👍 |
fabpot commentedJul 18, 2018
Thank you@ro0NL. |
…Security::getUser (ro0NL)This PR was squashed before being merged into the 4.2-dev branch (closes#27943).Discussion----------[Security] Deprecate returning stringish objects from Security::getUser| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes-ish| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->`$user` can also be an object implementing `__ toString`. Here we want only true user objects...Commits-------8c410da [Security] Deprecate returning stringish objects from Security::getUser
Uh oh!
There was an error while loading.Please reload this page.
$usercan also be an object implementing__ toString. Here we want only true user objects...