url_for not working when passing locale symbols
BSDer opened this issue · 13 comments
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>
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"
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?
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.
This line:
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:
ddba35a#diff-6500de086a3ee34b17d3ed33ff3af908R32
@enriclluelles do you recall if there is any reason for the conversion of the locale from sym to string?
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
endThe 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. 😉
@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?
Do you know any other gem that requires locale parameter as a string?
Yes, indeed 🙈
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. 😉
@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'
endI 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
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.
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,@flarecriteria thanks!
PR with suggested test is very welcomed