Skip to content
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

fix(token): fix getUsername calls only if there is no getUserIdentifi… #71

Conversation

noodle69
Copy link
Contributor

@noodle69 noodle69 commented Aug 5, 2022

…er method available

  • Update checks for token user getUsername call only if there is no getUserIdentifier method available.
  • fix deprecation feedbacks

@noodle69 noodle69 requested a review from a team as a code owner August 5, 2022 19:35
Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

EDIT: 👌

@noodle69
Copy link
Contributor Author

noodle69 commented Aug 5, 2022

shrug

@Oliboy50
from our proect we migrate to SF5.4, all our unit tests throw a deprecated feedback
we are invited to replace somewhere getUsername with GetUserIdentifier called from AbstractToken from symfony/security-core
from log-bridge-bundle tag 10.0.0 and using SF5.4, getUsername is present with geUserIdentifier in AbstractToken class.
So if we check only if method_exists for getUsername, we will throw a deprecated feedback

@Oliboy50
Copy link
Member

Oliboy50 commented Aug 6, 2022

thanks for the full explanation ❤️ this would have fit perfectly in the PR description 😄

@nalscher
Copy link

nalscher commented Aug 8, 2022

@noodle69 Can you had a test maybe ? 🤷‍♂️

@noodle69
Copy link
Contributor Author

noodle69 commented Aug 8, 2022

@noodle69 Can you had a test maybe ? man_shrugging

there is not already tests about those parts?
i don't know if it's possible to test depends of php version and symfony/security-core version used

@shavounet shavounet merged commit c406a5b into BedrockStreaming:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants