Skip to content

Make sure AuthenticationUtils::getLastUsername() returns a string #48480

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

Closed
wants to merge 1 commit into from
Closed

Make sure AuthenticationUtils::getLastUsername() returns a string #48480

wants to merge 1 commit into from

Conversation

gjuric
Copy link
Contributor

@gjuric gjuric commented Dec 5, 2022

Q A
Branch? 6.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

When calling AuthenticationUtils::getLastUsername() there is a return type hint that this method return a string, but $request->getSession()->get(SecurityRequestAttributes::LAST_USERNAME, '') can also return null which throws a TypeError.

This PR makes sure that the method return an empty string in such cases.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@gjuric gjuric changed the base branch from 6.3 to 6.0 December 5, 2022 11:46
@gjuric gjuric changed the base branch from 6.0 to 6.3 December 5, 2022 11:53
@OskarStark OskarStark changed the title Make sure AuthenticationUtils::getLastUsername() returns a string Make sure AuthenticationUtils::getLastUsername() returns a string Dec 5, 2022
@gjuric gjuric changed the base branch from 6.3 to 6.0 December 5, 2022 12:10
@chalasr
Copy link
Member

chalasr commented Dec 5, 2022

Thanks for the PR.
We decided to not do this change in #47829. The same reasoning applies here, so unless you can provide a legit use case where that request attribute ends up being null, I'm 👎 here. Sorry

@gjuric
Copy link
Contributor Author

gjuric commented Dec 5, 2022

Thank you, the issue was with the session value, not the attribute one, but the comments had me take another look at out codebase and I found the issue on our end.

@gjuric gjuric closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants