-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
feat: [#615] i18n support added #627
Conversation
@aeneasr The errors with the conformity tests appear to be unrelated to this PR. As per our discussion on Slack, please see this revised PR that provides a method to support ID-less hints for backward compatibility and to control changes. |
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.
Thank you! This looks much better already than the previous PR :) Maybe you could add a bit of docs to the README to show how one would define translation messages? This would help me also better understand the code!
Looks like conformity tests are failing with a build error, could you take a look please? https://github.com/ory/fosite/runs/3913037380?check_suite_focus=true |
Thanks for your review @aeneasr. The failure appears to be
I don't think it is related to my changes as such. Edit: It might have been fixed on the master branch on Hydra. I took a look and the file has changed. Edit Edit: Bingo! The changes you made did it. All green now |
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.
Almost there :)
i18n_helper.go
Outdated
if e, ok := err.(*RFC6749Error); ok { | ||
return e.WithLocalizer(catalog, lang) | ||
} else if e, ok := errorsx.Cause(err).(*RFC6749Error); ok { |
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.
Use errors.As
instead!
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.
Done
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.
Done
@aeneasr I have made the changes and also added a i18n_helper_test that tests the usage using an error object. |
Awesome, great job! |
The changes are proposed in #615 .
The changes involved:
MessageCatalog
interface exposes two funcs that can be used to choose an implementation of choice. TheDefaultMessageCatalog
provides a simple implementation.MessageCatalog
is nil and the implementation falls back to current behavior (tested by running the unit tests)WithHint
andWithHintf
record the original formatted message and arguments for legacy translation. Here the catalog contains a message ID that matches the formatted message.WithHintIDOrDefaultf
allows a message ID to be specified along with a default message. If a localizer is provided, the message ID is used to find the message and the vaargs are applied (if present). Otherwise the default formatted message is used.G11NContext
interface that declares aGetLang
func.There are no Breaking Changes.
NOTE: Some gaps exist in this implementation with the introspection handler because of a missing requester object. This will be addressed in a future PR.
Related issue(s)
Linked to PR.
Checklist
and signed the CLA.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
appropriate).
Further comments
The actual translation of messages is left to the point when
GetDescription
is called. Thus, the localizer only needs to be known to the error object at this point. This allows us to limit the changes to when error responses are built. For example, whenWriteAuthorizeError
is invoked.In both the authorize and token flows, the requester object is passed into the Write*Error func making it simple to extend the requester object to implement the
G11NContext
interface that declares aGetLang
function. With this, all that a consumer needs to do is add aMessageCatalog
instance to the compose.Config and the changes here take care of the rest.However,
WriteIntrospectionError
does not accept the requester and, so, a helper function -AddLocalizerToErrWithLang
- is provided to augment the error object with the localizer details (catalog and lang).For example:
This is somewhat cumbersome and should probably be enhanced to pass in the requester to the
WriteIntrospectError
. In the interest of ensuring backward compatibility, I have chosen not to make these changes but an overload funcWriteIntrospectErrorWithRequester
can be considered in a future update.