Open frahugo opened 9 years ago
I was wondering if the around_filter is really necessary
It is absolutely necessary: https://github.com/enriclluelles/route_translator/issues/44#issuecomment-37212297
Alright. I'll find another solution and post it here, in case someone else would experience the same problem. Thx
if you are able to find another solution, let me know
@frahugo a failing spec would also be appreciated
This is my current solution: https://gist.github.com/frahugo/705b0854126eb3ae7565 I basically push the "finale" locale (decided by ApplicationController) in the request environment, and pick it up later in the Devise middleware.
I've run into the same issue, on an application with 2 locales, the flash alert for "Invalid email or password" is always taken from I18n.default_locale
. It only affects the flash message, the views are properly translated.
Here's a sample app to reproduce it https://github.com/jaimeiniesta/devisebug
I tested the solution proposed by @frahugo and it works fine. Maybe it could be on the README or wiki
I confirm the @frahugo fix works
Please always pay attention to thread safety when setting the locale with a before_action
@tagliala What do you mean by thread safety?
Is there something wrong with @frahugo solution?
Is there something wrong with @frahugo solution?
yes: apparently, setting I18n.locale in a before_filter
is not a good idea.
Ref:
edit: maybe things have changed in the last 4 years
Gotcha. Thanks for the fast feedback.
Just one more question. The problem lies in a thread lasting longer then expected. If we do force the value in every new request, i believe we are good, no?
If we do force the value in every new request, i believe we are good, no?
How would you force that?
Wouldn't it be forcing the value at the beginning of every request if i don't have urls without the location on them? I'm not sure because i don't fully understand whats happening here but it seems to me that it would set a new value on any request. But again, i'm not sure about it.
Wouldn't it be forcing the value at the beginning of every request if i don't have urls without the location on them?
Exactly how? I mean, a snippet of code that will do that. It should skip I18n.locale at all, because the issue is in how I18n.locale sets locale
Gotcha. Thats what i expected. =)
I'm doing exactly as @frahugo suggested.
Since i did not have enough understanding of how I18n.locale
and route_translator
work i couldn't know for sure.
Just for clarity purposes, does it mean that I18n.locale =
is not a simple setter? (I believe that was my confusion)
I'm doing exactly as @frahugo suggested.
The problem in his approach is the before_filter
. You should avoid that to set locale, because thread related issues are sneaky: i've started #44 because I had issue in translatings strings through rails admin.
Just for clarity purposes, does it mean that I18n.locale = is not a simple setter? (I believe that was my confusion)
It is quite complicated also for me understand how Thread.current works and how threads are managed in multithreaded servers. This is explained in the stackoverflow issue I've linked and also in https://github.com/steveklabnik/request_store#the-problem
Gotcha. So i think i'm ok since all my ApplicationController
has is:
def set_current_locale
request.env['app.current_locale'] = I18n.locale.to_s
end
Since i just use the before_action
to get the actual value, i wont have any issues.
Thank you @tagliala !
I've had a deep dive into this (trying to get Devise and route_translator to play nice).
The above solution works because it does set I18n.default_locale
in addition to I18n.locale
, which is what will be picked up by Devise when deciding which URL helper method to call. If it is left to :en (the default), it will always call new_user_session_en_url'
, which work somewhat ok if you do path or param based language detection, but break for host based links.
The other thing is that the example above is not resetting I18n.default_locale once the controller is done, thus "leaking" it up the middleware chain.
This shouldn't be a threading issue, as default_locale turns out to be thread local (ie, not shared across threeads).
However, it still isn't a great solution, as it will set the locale and default locale for the next request as well (up to the point where the before_action triggers), which means that the locale will actually differ within the Warden middleware depending on whether it has called out to the application or not. This might easily cause a lot of headache in the future.
As a side note, the code snippet given as solution above assigns a few local variables and such that are never actually used and could be shortened to:
# hey there, don't copy-paste this, this isn't a real solution
def set_my_locale_from_url
return unless locale = params[RouteTranslator.locale_param_key] || RouteTranslator::Host.locale_from_host(request.host)
I18n.default_locale = I18n.locale = locale
end
Also note that in my example I'm using main locales (en and de) for routes, but also support more specific locales for page content. To make it pick up the right URL helpers I have to set the default locale to the generic locale (ie de for de-CH/de-DE).
This can be done with the following adjustment:
# hey there, don't copy-paste this, this isn't a real solution
def set_my_locale_from_url
return unless locale = params[RouteTranslator.locale_param_key] || RouteTranslator::Host.locale_from_host(request.host)
I18n.locale = locale
I18n.default_locale = locale[/^[^\-]+/]
end
Part of a proper solution is, in my opinion, to move the logic for setting the locale (and default locale) into a middleware:
# lib/route_translator/middleware
class RouteTranslator::Middleware
def initialize(app)
@app = app
end
def call(env)
return @app.call(env) unless locale = parse_locale(env)
locale_save do
I18n.locale = locale
I18n.default_locale = locale[/^[^\-]+/]
@app.call(env)
end
end
private
def parse_locale(env)
request = ActionDispatch::Request.new(env)
RouteTranslator.locale_from_params(request.params) || RouteTranslator::Host.locale_from_host(request.host)
end
def locale_save
default_locale_was = I18n.default_locale
locale_was = I18n.locale
yield
ensure
I18n.default_locale = default_locale_was if default_locale_was
I18n.locale = locale_was if locale_was
end
end
# config/initializers/i18n.rb
require 'route_translator/middleware'
Rails.application.config.middleware.insert_before(Warden::Manager, RouteTranslator::Middleware)
Note that this differs slightly from the code I'm using, as I pick the specific locale from bothe the host and the Accept-Language header.
I18n.default_locale
?At first I just moved the locale logic into a middleware, without also setting default_locale
(which leads to broken redirects), until I saw the above solution. Which got me wondering why Devise seems to use I18n.default_locale
. It doesn't. Devise ends up calling something like new_user_session_url
(this is happening in lib/devise/failure_app.rb
if anyone is following along), which gives control back to route translator, which in turn is falling back to default_locale.
This could be avoided/improved if route_name_for
would first try I18n.fallbacks
instead of going straight to I18n.default_locale
.
Hi @rkh, thanks for the thorough explanation
I do not recall how to reproduce this issue, is there by any chance the possibility to have a reduced test case with Rails 6.1, Devise, and Route Translator to reproduce it, so I can take a look?
Maybe a refreshed version of https://github.com/jaimeiniesta/devisebug with a step-by-step guide to reproduce in the readme
This shouldn't be a threading issue, as default_locale turns out to be thread local (ie, not shared across threeads).
The problem is that the proposed solution is to use before_filter
, reverting to old route translator's behavior.
The effects of having before_filter
instead of around_filter
are described here: https://github.com/enriclluelles/route_translator/issues/44#issuecomment-37212297
I can see that config
was a thread local variable also in I18n version 0.6.9 when I was experiencing this issue back in 2014
I cannot advise using before_filter
instead of around_filter
, and I'm unlikely to revert to the old approach. I think that if there should be an issue in route_translator, you may prefer a failed authentication message in the default application locale rather than saving user content in the wrong one
This could be avoided/improved if route_name_for would first try I18n.fallbacks instead of going straight to I18n.default_locale.
PR are welcomed
I agree. before_filter is not the way to go. I'll see if I can find some time in the coming days to work on a PR/demo.
Thanks, really appreciate
Please attempt the simplest solution possible you mentioned about route_name_for
Here is an example: https://github.com/rkh/drt-example
There's also a host-based
branch to demonstrate it breaking completely (rather than just displaying the wrong locale) as well as url_for
not working.
There's no easy fix while also maintaining a before action. I am considering to extract the middleware logic I'm using and put it in a separate gem, I think, that will address the issue, as I am not sure how much complexity you want to add to your library.
Hi @rkh thanks for the reproducible test case.
I can confirm the issues, and I confirm that I'm not sure that a middleware would be the best choice
At the moment, Route Translator is opt-in, can we use the same approach with a middleware?
Let's go back to the original issue here
You should be able to translate the message by adding the following to devise.rb
# config/devise.rb
ActiveSupport.on_load(:devise_failure_app) do
def i18n_options(options)
locale_from_url = RouteTranslator.locale_from_params(params) || RouteTranslator::Host.locale_from_host(request.host)
options.merge(locale: locale_from_url)
end
end
I think I can add a convenience method (locale_from_request(params, request.host)
) which calls both
The second problem is the wrong redirect
I think this could be fixed by monkey patching route
method:
# config/devise.rb
ActiveSupport.on_load(:devise_failure_app) do
def route(scope)
:"new_#{scope}_session_#{route_translator_locale.to_s.underscore}_url"
end
def i18n_options(options)
options.merge(locale: route_translator_locale)
end
private
def route_translator_locale
RouteTranslator.locale_from_params(params) || RouteTranslator::Host.locale_from_host(request.host)
end
end
If the above approach works, we can turn this into an extension or a module
Makes sense. Middleware can also be opt-in. I'll be using this approach, as I can reuse the middleware for Sinatra applications. I've been copy-pasting I18n logic around a bit and want to move away from this, hence me considering putting the middleware into a separate library. Having separate domains to choose between German and English, and then picking a specific locale (like de-CH/de-DE) based on Accept-Language header is a common use case for me.
I was also working on I18n itself a bit more and realised that setting I18n.default_locale (as the initial solution does) is not thread-safe at all and should not be done (as all I18n::Config instances share the same value via a class variable).
I currently set up the middleware in the same initializer I use to configure RouteTranslator:
I18n.backed = Localization::Backend.new
I18n.available_locales = %i[ en en-AU en-CA en-CY en-GB en-IE en-IN en-NZ en-US en-ZA de de-AT de-CH de-DE de-LI de-LU]
I18n.load_path += Dir[Rails.root.join('config/locales/**/*.{yml,rb}')]
RouteTranslator.config do |config|
config.locale_param_key = :main_locale
config.available_locales = [:en, :de]
# note: I've already realized that the line above should include all sub-locales, but haven't updated the app yet
if Rails.env.production?
config.host_locales = { … }
elsif File.readable?('/etc/hosts') and File.read('/etc/hosts') =~ /de\.local en\.local/
config.host_locales = { 'de.local' => :de, 'en.local' => :en }
else
config.force_locale = true
end
end
Rails.application.config.middleware.insert_before(Warden::Manager, Localization::Middleware)
Here's the middleware I currently use in the specific app where I ran into the issue (and that fixes the issue for me):
module Localization
class Middleware
def initialize(app) = @app = app
include Helpers
def call(env)
locale_was = I18n.locale
request = ActionDispatch::Request.new(env)
params = request.params
locale_from_url = RouteTranslator.locale_from_params(params) || RouteTranslator::Host.locale_from_host(request.host)
I18n.locale = locale_from_url if locale_from_url
I18n.locale = preferred_locale(request, I18n.locale, params[:locale])
@app.call(env)
ensure
I18n.locale = _locale_was
end
end
end
It would also be possible to add a middleware automatically, but enable/disable it via RouteTranslator.config
:
# in route_translator/middleware.rb
class RouteTranslator::Middleware < I18n::Middleware
ActiveSupport.on_load(:after_initialize) do
# find the last-ish built-in middleware so we can insert our middleware before devise and other tools
last_middleware = Rails.application.middleware.reverse_each.detect { |m| m.name.start_with? 'ActionDispatch::' }
Rails.application.middleware.insert_after(last_middleware, self)
end
def call(env)
if RouteTranslator.config.set_locale_in_middeware
request = ActionDispatch::Request.new(env)
locale_from_url = RouteTranslator.locale_from_params(request.params) || RouteTranslator::Host.locale_from_host(request.host)
I18n.locale = locale_from_url if locale_from_url
end
super
end
end
With such a middleware, all that would be needed to fix the Devise issue should be:
RouteTranslator.config do |config|
config.set_locale_in_middeware = true
end
But I do think just adding a small bit of Devise-specific logic should be fine, as it is fully backwards compatible.
Instead of locale_from_request(params, request.host)
it should also be possible to use locale_from_request(request)
, as you can access params
via request.params
.
A general middleware functionality could be interesting to use route translator with other framework and with rails iself
It would also be possible to add a middleware automatically, but enable/disable it via
RouteTranslator.config
:
Will it be possible to exclude some controllers with the middleware approach? An equivalent of skip_around_filter
Instead of
locale_from_request(params, request.host)
it should also be possible to uselocale_from_request(request)
, as you can accessparams
viarequest.params
.
Thanks
Any news about solving this translation-related issue the "normal" way? I can't translate "Invalid email or password." (related to issue https://github.com/heartcombo/devise/issues/3647).
In my config/locales/devise.it.yml
I have:
it:
devise:
# ...
failure:
# ...
invalid: "%{authentication_keys} o password non validi."
In my /project-app/app/controllers/application_controller.rb
I have:
class ApplicationController < ActionController::Base
around_action :switch_locale
def switch_locale(&action)
locale = current_user.try(:locale) || params[:locale] || I18n.default_locale
I18n.with_locale(locale, &action)
end
def default_url_options
{ locale: I18n.locale }
end
end
Terminal output is:
Started POST "/users/sign_in?locale=it" for 127.0.0.1 at 2023-03-07 21:53:06 +0100
Processing by Users::SessionsController#create as TURBO_STREAM
Parameters: {"authenticity_token"=>"[FILTERED]", "user"=>{"email"=>"email@example.it", "password"=>"[FILTERED]", "remember_me"=>"0"}, "commit"=>"Accedi", "locale"=>"it"}
Completed 401 Unauthorized in 1ms (ActiveRecord: 0.0ms | Allocations: 402)
I'm currently having an issue with the route_translator & Devise combo. When a user accesses a localized route, the gem properly sets the locale (in a
around_filter
method). If the user is not authenticated, Devise will issue a 401 and redirect to the sign-in page and also build a flash message. That logic is performed by a metal app called by the warden middleware.Since route_translator remembers the previous I18n.locale and default_locale, and sets them back after the yield, the Devise middleware ends up with the wrong I18n.locale when the requets completes.
I hacked my application controller to make it work (see below). I was wondering if the
around_filter
is really necessary in route_translator and if it could be replaced by abefore_filter
. I think the locale should not go back to the previous value and the middleware(s) running afterward should have access to that value.If you agree with the change, it will be a pleasure for me to submit a pull request. Thanks.