I18n.locale not available to middlewares when request completes

[frahugo]

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 a before_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.

class ApplicationController < ActionController::Base
  # HACK for route_translators
  skip_around_filter :set_locale_from_url
  before_filter :set_my_locale_from_url

  def set_my_locale_from_url
    tmp_default_locale = RouteTranslator::Host.locale_from_host(request.host)
    if tmp_default_locale
      current_default_locale = I18n.default_locale
      I18n.default_locale    = tmp_default_locale
    end

    tmp_locale = params[RouteTranslator.locale_param_key] || tmp_default_locale
    if tmp_locale
      current_locale = I18n.locale
      I18n.locale    = tmp_locale
    end
  end
end

[tagliala]

I was wondering if the around_filter is really necessary

It is absolutely necessary: #44 (comment)

[frahugo]

Alright. I'll find another solution and post it here, in case someone else would experience the same problem. Thx

[tagliala]

if you are able to find another solution, let me know

[tagliala]

@frahugo a failing spec would also be appreciated

[frahugo]

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.

[jaimeiniesta]

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

[prem-prakash]

I tested the solution proposed by @frahugo and it works fine.
Maybe it could be on the README or wiki

[kpitn]

I confirm the @frahugo fix works

[tagliala]

Please always pay attention to thread safety when setting the locale with a before_action

[plribeiro3000]

@tagliala What do you mean by thread safety?

Is there something wrong with @frahugo solution?

[tagliala]

Is there something wrong with @frahugo solution?

yes: apparently, setting I18n.locale in a before_filter is not a good idea.

Ref:

• Issues with rails admin (Thread related) #44
• https://stackoverflow.com/questions/9263036/why-locale-setting-in-rails-acts-as-global-when-using-thin/13931928#13931928

edit: maybe things have changed in the last 4 years

[plribeiro3000]

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?

[tagliala]

If we do force the value in every new request, i believe we are good, no?

How would you force that?

[plribeiro3000]

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.

[tagliala]

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

[plribeiro3000]

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)

[tagliala]

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

[plribeiro3000]

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 !

[rkh]

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.

Improving @frahugo's solution

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

Moving things to a Middleware

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.

Why does Devise use 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.

[tagliala]

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: #44 (comment)

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

[rkh]

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.

[tagliala]

Thanks, really appreciate

Please attempt the simplest solution possible you mentioned about route_name_for

[rkh]

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.

[tagliala]

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

[rkh]

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.

[tagliala]

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 use locale_from_request(request), as you can access params via request.params.

Thanks

[lapser]

Any news about solving this translation-related issue the "normal" way? I can't translate "Invalid email or password." (related to issue heartcombo/devise#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)