8000 Add UsernameNotFoundException declaration to refreshUser(). by umulmrum · Pull Request #26467 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Add UsernameNotFoundException declaration to refreshUser(). #26467

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Add UsernameNotFoundException declaration to refreshUser(). #26467

wants to merge 2 commits into from

Conversation

umulmrum
Copy link
Contributor
@umulmrum umulmrum commented Mar 9, 2018
Q A
Branch? master
Bug fix? maybe
New feature? maybe
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Symfony\Component\Security\Core\User\UserProviderInterface::refreshUser() does not declare that implementations may throw a UsernameNotFoundException, although a) it makes sense, as the user could have been deleted since the last load, and b) the ContextListener already handles the UsernameNotFoundException. So it looks like someone thought of this, but simply forgot the annotation.

Unsure if this is a bugfix or a feature, but as it doesn't change executed code, master should be soon enough.

@linaori
Copy link
Contributor
linaori commented Mar 9, 2018 •
edited
Loading

👍 as internally the loadUserByUsername is often called, which should bubble up the throwable exceptions

@xabbuh
Copy link
Member
xabbuh commented Mar 9, 2018

Our own user providers throw this exception. So this change actually just reflects the existing implementations.

@xabbuh xabbuh added this to the 2.7 milestone Mar 9, 2018
@xabbuh xabbuh added the Security label Mar 9, 2018
@fabpot
Copy link
Member
fabpot commented Mar 10, 2018

Thank you @umulmrum.

@fabpot fabpot closed this in e0f79f6 Mar 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
Security Status: Reviewed
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
5 participants
@umulmrum @linaori @xabbuh @fabpot @carsonbot
0