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

Translate message errors #973

Merged
merged 1 commit into from
Jan 27, 2022
Merged

Translate message errors #973

merged 1 commit into from
Jan 27, 2022

Conversation

flohw
Copy link
Contributor

@flohw flohw commented Dec 29, 2021

Hi @chalasr,

The exception messages are not translated even if the locale was set.
I largely copied inspired my code from this PR: symfony/symfony#38037 to translate the message.

Let me know if any changes need to be done.

@flohw
Copy link
Contributor Author

flohw commented Dec 29, 2021

I just noted that on the original PR translations are made in the authenticator. It may be a better choice as the listener can be changed by the developer.

Let me know if I missed something and should update to translate in the authenticators too or only in them.

Thank you

[EDIT] also, not sure why the tests fail...

Copy link
Collaborator

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR. Here are some suggestions :)

@chalasr
Copy link
Collaborator

chalasr commented Dec 29, 2021

Don't worry about the failing tests, it's unrelated. I need to look into them, sorry about that.

@flohw
Copy link
Contributor Author

flohw commented Dec 29, 2021

Thank you for this quick review! I updated the code accordingly. (sorry for the multiple push force, I struggled with my gpg key...)

Also note about this comment above:

I just noted that on the original PR translations are made in the authenticator. It may be a better choice as the listener can be changed by the developer.

Let me know if I missed something and should update to translate in the authenticators too or only in them.

Should I update the other place where strtr is used?

@chalasr
Copy link
Collaborator

chalasr commented Dec 29, 2021

Sorry I was unsure to understand your question, got it now.
If you could do the same in the jwt authenticator that would be awesome (both are good, so what you already did is valid!)

@flohw
Copy link
Contributor Author

flohw commented Dec 30, 2021

Hi @chalasr,

Thanks I just added translator in both authenticators (JWT and JWTToken).

@flohw
Copy link
Contributor Author

flohw commented Jan 5, 2022

Hi @chalasr
I just saw the new release and rebase my branch.
The failing tests have changed. I am not sure if it's on my side to repair especially for the PHP8/SF5.3 couple.

Thanks

@chalasr
Copy link
Collaborator

chalasr commented Jan 5, 2022

No worries, it's due to a Symfony bug revealed by your changes. See symfony/symfony#44918 for the fix.

@chalasr
Copy link
Collaborator

chalasr commented Jan 27, 2022

Thanks @flohw for working on this feature, this is much appreciated.

@chalasr chalasr merged commit 6a196f3 into lexik:2.x Jan 27, 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.

2 participants