Respect locale set by controller in the failure app #5567
Conversation
|
Please commit/merge this asap if it works. |
|
@lapser thanks. I plan to get it merged soon, just leaving it sit here for a bit longer in case someone from the issues I pinged previously wanted to give it a try first. If you do test it out, let me know how it goes. |
carlosantoniodasilva
force-pushed
the
ca-fix-i18n-locale-failure-app
branch
from
0e8534c
to
4d08b2e
Compare
carlosantoniodasilva
force-pushed
the
ca-fix-i18n-locale-failure-app
branch
from
4d08b2e
to
ec708c9
Compare
|
I was having the same issue as #5246 and tried out this branch and confirmed that it fixes the issue for me |
|
This would be wildly helpful!! ❤️ 🚢 |
❤️
|
Any updates on this PR? I had the same problem and the PR fixes the issue for me 🚀 |
|
This looks good and works for me too. If it isn't ready to be merged right away, can it be rebased against the latest release (4.9.2) so anyone using this branch is getting the latest devise plus this improvement? |
carlosantoniodasilva
force-pushed
the
ca-fix-i18n-locale-failure-app
branch
from
ec708c9
to
8bd19e8
Compare
|
@mark-young-atg this is now rebased against v4.9.2 (not main -- I have to review a couple of things that got in there recently that I haven't had the chance to yet) Everyone else who commented: I'll get this merged and hopefully released soon, appreciate the help confirming it works as expected for you all, thanks for the patience. |
|
I have removed my previous comments. This branch does work as expected. The problem I saw is elsewhere. |
|
Having the same issue and this PR works for me. |
|
Hey just checking on this again, hopping to see it get in soon! Any progress updates would be appreciated! 😃 |
|
Sorry folks, I know it's been taking some time, but I had to step away from things for personal reasons in the past couple of months. I'm slowly catching up now, and have this as one of the main things on my radar to look at. 🔜 |
|
Hi @carlosantoniodasilva |
|
@mark-young-atg 👋 thanks. v4.9.3 already landed, but I'll get this merged and released as part of the next version. |
carlosantoniodasilva
force-pushed
the
ca-fix-i18n-locale-failure-app
branch
3 times, most recently
from
f6f367a
to
68d048c
Compare
A common usage of I18n with different locales is to create some around callback in the application controller that sets the locale for the entire action, via params/url/user/etc., which ensure the locale is respected for the duration of that action, and resets at the end. Devise was not respecting the locale when the authenticate failed and triggered the failure app, because that happens in a warden middleware right up in the change, by that time the controller around callback had already reset the locale back to its default, and the failure app would just translate flash messages using the default locale. Now we are passing the current locale down to the failure app via warden options, and wrapping it with an around callback, which makes the failure app respect the set I18n locale by the controller at the time the authentication failure is triggered, working as expected. (much more like a normal controller would.) I chose to introduce a callback in the failure app so we could wrap the whole `respond` action processing rather than adding individual `locale` options to the `I18n.t` calls, because that should ensure other possible `I18n.t` calls from overridden failure apps would respect the set locale as well, and makes it more like one would implement in a controller. I don't recommend people using callbacks in their own failure apps though, as this is not going to be documented as a "feature" of failures apps, it's considered "internal" and could be refactored at any point. It is possible to override the locale with the new `i18n_locale` method, which simply defaults to the passed locale from the controller. Closes #5247 Closes #5246 Related to: #3052, #4823, and possible others already closed. Related to warden: (may be closed there afterwards) wardencommunity/warden#180 wardencommunity/warden#170
carlosantoniodasilva
force-pushed
the
ca-fix-i18n-locale-failure-app
branch
from
68d048c
to
50af639
Compare
A common usage of I18n with different locales is to create some around callback in the application controller that sets the locale for the entire action, via params/url/user/etc., which ensure the locale is respected for the duration of that action, and resets at the end. Devise was not respecting the locale when the authenticate failed and triggered the failure app, because that happens in a warden middleware right up in the change, by that time the controller around callback had already reset the locale back to its default, and the failure app would just translate flash messages using the default locale. Now we are passing the current locale down to the failure app via warden options, and wrapping it with an around callback, which makes the failure app respect the set I18n locale by the controller at the time the authentication failure is triggered, working as expected. (much more like a normal controller would.) I chose to introduce a callback in the failure app so we could wrap the whole `respond` action processing rather than adding individual `locale` options to the `I18n.t` calls, because that should ensure other possible `I18n.t` calls from overridden failure apps would respect the set locale as well, and makes it more like one would implement in a controller. I don't recommend people using callbacks in their own failure apps though, as this is not going to be documented as a "feature" of failures apps, it's considered "internal" and could be refactored at any point. It is possible to override the locale with the new `i18n_locale` method, which simply defaults to the passed locale from the controller. Closes #5247 Closes #5246 Related to: #3052, #4823, and possible others already closed. Related to warden: (may be closed there afterwards) wardencommunity/warden#180 wardencommunity/warden#170
|
This is in 4-stable and main now. |
|
Thank you for all your work on this project!! 💖 💖 💖 |
|
Thank you @carlosantoniodasilva, much appreciated. |
|
@carlosantoniodasilva |
|
Now this work has been merged into |
|
Maybe it's worth noting, if current_user is called anywhere in before action then this fix is not working, sign_in goes straight to warden and sessions_controller#create action along with auth_options are not called. Combining with this fix: #5602 (comment) - everything works as expected |
|
@RavWar the ideal solution is to make sure you're always setting the locale first, in a prepended before action / around action (this is true not only for Devise, but as a general rule, if your app depends on I18n locale to be set). That other change you pointed out can be used, but it has more side-effects that are worth being aware of. @mark-young-atg I'll look into cutting a new release, however from 4-stable (since main contains other unrelated / breaking changes), thanks. |
|
For folks waiting on a release: I just released v4.9.4 including this fix. |
A common usage of I18n with different locales is to create some around callback in the application controller that sets the locale for the entire action, via params/url/user/etc., which ensure the locale is respected for the duration of that action, and resets at the end.
Devise was not respecting the locale when the authenticate failed and triggered the failure app, because that happens in a warden middleware right up in the change, by that time the controller around callback had already reset the locale back to its default, and the failure app would just translate flash messages using the default locale.
Now we are passing the current locale down to the failure app via warden options, and wrapping it with an around callback, which makes the failure app respect the set I18n locale by the controller at the time the authentication failure is triggered, working as expected. (much more like a normal controller would.)
I chose to introduce a callback in the failure app so we could wrap the whole
respondaction processing rather than adding individuallocaleoptions to theI18n.tcalls, because that should ensure other possibleI18n.tcalls from overridden failure apps would respect the set locale as well, and makes it more like one would implement in a controller. I don't recommend people using callbacks in their own failure apps though, as this is not going to be documented as a "feature" of failures apps, it's considered "internal" and could be refactored at any point.It is possible to override the locale with the new
i18n_localemethod, which simply defaults to the passed locale from the controller.Closes #5247
Closes #5246
Related to: #3052, #4823, and possible others already closed. Related to warden: (may be closed there afterwards) wardencommunity/warden#180 wardencommunity/warden#170