url_for not working when passing locale symbols

[BSDer]

Hi, I see url_for not working when routes are localized:

#config/routes.rb 
Rails.application.routes.draw do
  root 'pages#index'
  localized do
    resources :agencies
  end
end

Routes are OK:

% rake routes
                   Prefix Verb   URI Pattern                                                                              Controller#Action
                     root GET    /                                                                                        pages#index
              agencies_en GET    /agencies(.:format)                                                                      agencies#index {:locale=>"en"}
                          POST   /agencies(.:format)                                                                      agencies#create {:locale=>"en"}
            new_agency_en GET    /agencies/new(.:format)                                                                  agencies#new {:locale=>"en"}
           edit_agency_en GET    /agencies/:id/edit(.:format)                                                             agencies#edit {:locale=>"en"}
                agency_en GET    /agencies/:id(.:format)                                                                  agencies#show {:locale=>"en"}
                          PATCH  /agencies/:id(.:format)                                                                  agencies#update {:locale=>"en"}
                          PUT    /agencies/:id(.:format)                                                                  agencies#update {:locale=>"en"}
                          DELETE /agencies/:id(.:format)                                                                  agencies#destroy {:locale=>"en"}
       rails_service_blob GET    /rails/active_storage/blobs/:signed_id/*filename(.:format)                               active_storage/blobs#show
rails_blob_representation GET    /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
       rails_disk_service GET    /rails/active_storage/disk/:encoded_key/*filename(.:format)                              active_storage/disk#show
update_rails_disk_service PUT    /rails/active_storage/disk/:encoded_token(.:format)                                      active_storage/disk#update
     rails_direct_uploads POST   /rails/active_storage/direct_uploads(.:format)                                           active_storage/direct_uploads#create

But when I call url_for:

% rails c
Running via Spring preloader in process 74129
Loading development environment (Rails 5.2.2.1)
irb(main):001:0> include Rails.application.routes.url_helpers
=> Object
irb(main):002:0> url_for :action=>"index", :controller=>"pages",  only_path: true
=> "/"
irb(main):003:0> url_for :action=>"index", :controller=>"agencies", :locale => :en,  :only_path => true
ActionController::UrlGenerationError: No route matches {:action=>"index", :controller=>"agencies", :locale=>:en}
        from (irb):5
irb(main):004:0> 

[tagliala]

Hi,

does https://github.com/enriclluelles/route_translator/wiki/Generating-translated-URLs help?

[BSDer]

Hi, locale has to be a string, not as a symbol, nevertheless by following the instructions on the docs, I18n.locale has to be set as a symbol. So is this an url_for bug (that should check and convert locale to string) or is this a bug in router translator documentation?

irb(main):004:0> url_for :action=>"index", :controller=>"agencies", :locale => :en,  :only_path => true
ActionController::UrlGenerationError: No route matches {:action=>"index", :controller=>"agencies", :locale=>:en}
        from (irb):4
irb(main):005:0> url_for :action=>"index", :controller=>"agencies", :locale => 'en',  :only_path => true
=> "/agencies"

[tagliala]

So is this an url_for bug (that should check and convert locale to string) or is this a bug in router translator documentation?

Not a bug, it has to be a string 😅. PR to support both are welcomed

Is there any place of the documentation of Route Translator which states that it should be a symbol?

[BSDer]

Hi, ok I really think there is a bug and it's not in Rails. I am short of knowledge to track it down, but the bug is there:

irb(main):007:0> url_for action: :index, controller: :pages, locale: :es, only_path: true
=> "/?locale=es"
irb(main):008:0> url_for action: :index, controller: :agencies, locale: :es, only_path: true
ActionController::UrlGenerationError: No route matches {:action=>"index", :controller=>"agencies", :locale=>:es}
        from (irb):8

As you can see in 007 it works with locale as a symbol for a route outside the localized block and it does not work for a localized route, as shown in 008.

As a workaround you can specify locale as a string, in which case url_for works also for a localized route, see 009.

irb(main):009:0> url_for action: :index, controller: :agencies, locale: 'es', only_path: true
=> "/agencias"

I tested {model}_path and it works with locale as a symbol, so it's an issue that is triggered by url_for and locale as a symbol in translated routes.

irb(main):010:0> agencies_path locale: :es
=> "/agencias"

Please note that I18n.locale are supposed to be symbols, not strings, as reported in Rails literature and common practice, see for example https://guides.rubyonrails.org/i18n.html

Can you please help me track the bug down?

Thank you.

[tagliala]

This line:

https://github.com/enriclluelles/route_translator/blob/master/lib/route_translator/locale_sanitizer.rb#L8

of course, adding .to_sym will break all tests and backward compatibility.

Also, I'm not the original author of this gem so I don't know if there is a deeper reason for using strings instead of symbols. The first appearance of .to_s is this one: https://github.com/enriclluelles/route_translator/commit/ddba35ab3daf1a635ad8dd856d956dc0ca438c7f#diff-6500de086a3ee34b17d3ed33ff3af908R32

[tagliala]

@enriclluelles do you recall if there is any reason for the conversion of the locale from sym to string?

[alexanderadam]

It's not necessary to change the sanitizer. So maybe there are not so many breaking tests if you just change ActionDispatch::Routing::RouteSet#translate_mapping to

def translate_mapping(locale, route_set, translated_options, translated_path_ast, scope, controller, default_action, to, formatted, via, translated_options_constraints, anchor)
  scope_params = {
    blocks:      (scope[:blocks] || []).dup,
    constraints: scope[:constraints] || {},
    defaults:    scope[:defaults] || {},
    module:      scope[:module],
    options:     scope[:options] ? scope[:options].merge(translated_options) : translated_options
  }

  if RouteTranslator.config.verify_host_path_consistency
    scope_params[:blocks].push RouteTranslator::HostPathConsistencyLambdas.for_locale(locale)
  end

  translated_options[:locale] = translated_options[:locale].to_sym if translated_options[:locale].is_a?(String)
  ::ActionDispatch::Routing::Mapper::Mapping.build scope_params, route_set, translated_path_ast, controller, default_action, to, via, formatted, translated_options_constraints, anchor, translated_options
end

The important bit is translated_options[:locale] = translated_options[:locale].to_sym if translated_options[:locale].is_a?(String) but the symbol is just required for the method Mapping#build.

But I had to set params[:locale] = params[:locale].to_s if params[:locale].is_a?(Symbol) in a before filter because outerwise the locale param is a Symbol (which can break other gems that rely on Strings).

So this could fix the issue if you can solve the params thingy without a before filter. :wink:

[tagliala]

@alexanderadam thanks for the detailed feedback

which can break other gems that rely on Strings

Do you know any other gem that requires locale parameter as a string?

[alexanderadam]

Do you know any other gem that requires locale parameter as a string?

Yes, indeed :see_no_evil:

I just ran into this when investigated into this issue and made this fix mentioned before.

I had this issue for example in Alchemy, a CMS I use (I can really recommend it btw.):

If params[:locale] is given Alchemy CMS tries to find a proper localized page for it and (indirectly) uses params[:locale].split('-') (for getting en if en-GB was given).

But it is no bug in Alchemy CMS and Alchemy also is definitely no exception in this regard.

In general it totally make sense: given HTTP parameters can't be typed if there's no additional format on top. Thus all parameters have to be handled as a string.

So instead of adding .to_s to all other gems we should rather fix it in the place were the issue comes from. :wink:

[tagliala]

@alexanderadam thanks again

Premise: at the moment I'm quite busy and I cannot allocate too much time on route_translator

An example of gem using symbols instead?

I've tried with your suggested fix but I'm now having 39 failures

Finished in 1.539876s, 61.0439 runs/s, 105.8527 assertions/s.
94 runs, 163 assertions, 39 failures, 0 errors, 0 skips

And that's expected, now we should use symbols instead of strings to match the route. But, even if I do so, I have 2 other failures.

This one about side effects makes me think

Failure:
TranslateRoutesTest#test_no_side_effects [/Users/geremia/dev/route_translator/test/routing_test.rb:614]:
The recognized options <{"controller"=>"products", "action"=>"show", "locale"=>"es", "id"=>"path/to/a/product"}> did not match <{"controller"=>"products", "action"=>"show", "locale"=>:es, "id"=>"path/to/a/product"}>, difference:.
--- expected
+++ actual
@@ -1 +1 @@
-{"controller"=>"products", "action"=>"show", "locale"=>:es, "id"=>"path/to/a/product"}
+{"controller"=>"products", "action"=>"show", "locale"=>"es", "id"=>"path/to/a/product"}

  # See https://github.com/enriclluelles/route_translator/issues/69
  def test_no_side_effects
    draw_routes do
      localized do
        resources :people
      end

      scope '(:locale)', locale: /(en|es)/ do
        get '*id' => 'products#show', as: 'product'
      end
    end

    assert_routing '/es/gente', controller: 'people', action: 'index', locale: :es
    assert_routing '/people', controller: 'people', action: 'index', locale: :en

    assert_routing '/es/path/to/a/product', controller: 'products', action: 'show', locale: :es, id: 'path/to/a/product'
    assert_routing '/path/to/another/product', controller: 'products', action: 'show', id: 'path/to/another/product'
  end

I think this works because of the regexp and the locale is going to be checked after, but I cannot invest more time, sorry.

Anyway... A PR is very welcomed, but I will be pedantic on this change.

edit: I guess that the only problem is that url_for needs to accept both string and symbol format for the locale, so I would take a look at how that work and try to reproduce a similar behaviour

[alexanderadam]

The failure in TranslateRoutesTest#test_no_side_effects is exactly the issue I had, isn't it? I have no clue about the internals of route_translator so it's not very likely that I will be able to provide a PR.

[flarecriteria]

Ran into this this morning. You can just monkey patch generate. Kinda don't like doing that but it's a decent quick and dirty work around. Patch makes symbols work and non of the existing tests fail. Also pretty sure you want to test with assert_generates not assert_routing

assert_routing is bidirectional, so its not specifically testing your use case

diff --git a/lib/route_translator/extensions/route_set.rb b/lib/route_translator/extensions/route_set.rb
index 2e863d6..84e84ad 100644
--- a/lib/route_translator/extensions/route_set.rb
+++ b/lib/route_translator/extensions/route_set.rb
@@ -24,6 +24,13 @@ module ActionDispatch

       private

+      def generate(route_key, options, recall = {})
+        if options.key?(:locale)
+          options[:locale] = options[:locale].to_s
+        end
+        Generator.new(route_key, options, recall, self).generate
+      end
+
       def translate_mapping(locale, route_set, translated_options, translated_path_ast, scope, controller, default_action, to, formatted, via, translated_options_constraints, anchor)
         scope_params = {
           blocks:      (scope[:blocks] || []).dup,

[tagliala]

@flarecriteria thanks!

PR with suggested test is very welcomed