Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable_fallback not working when I18n.fallbacks is configured #241

Closed
dvkch opened this issue Apr 20, 2021 · 7 comments · Fixed by #253
Closed

disable_fallback not working when I18n.fallbacks is configured #241

dvkch opened this issue Apr 20, 2021 · 7 comments · Fixed by #253
Labels
Milestone

Comments

@dvkch
Copy link

dvkch commented Apr 20, 2021

Steps to reproduce*

Create an app with the given configuration and run rails routes

Expected behavior*

We should generate the following routes :

  • support_contact_en
  • support_contact_it
  • newsletter_subscription_en

Actual behavior*

newsletter_subscription_it is defined as well, with the en translation of the route.

If I comment config.i18n.fallbacks = ... then the route gets created too but with the following path /it/translation%20missing:%20it.routes.newsletter_subscription.

System configuration*

Rails version: Rails 6.0.3.4
Ruby version: Ruby 2.6.6
Route Translator version: 10.0.0

I18n configuration*

    config.i18n.available_locales = [:fr, :en, :es, :it]
    config.i18n.default_locale = :en
    config.i18n.fallbacks = [I18n.default_locale, {
      it: :en,
      es: :en,
      fr: :en,
      en: :fr
    }]

Route Translator initializier

Source of: config/initializers/route_translator.rb

RouteTranslator.config do |config|
  config.force_locale = true
  config.disable_fallback = true
end

Source code of routes.rb*

Source of: config/routes.rb

get  'newsletter_subscription', to: 'newsletter_subscriptions#new', as: :newsletter_subscription
post 'newsletter_subscription', to: 'newsletter_subscriptions#create', as: :create_newsletter_subscription
get  'support_contact', to: 'support_contacts#new', as: :support_contact
post 'support_contact', to: 'support_contacts#create', as: :create_support_contact

Locale .yml files*

  • routes.en.yml
en:
  routes:
    newsletter_subscription: 'newsletter'
    support_contact: 'contact'
  • routes.it.yml
it:
  routes:
    support_contact: 'contatti'
@tagliala
Copy link
Collaborator

Hi,

apologies for the late reply

I think that the expected behavior is correct, or I'm getting this report wrong

route_translator generates routes for all the available locales. If the application is available in it and en, all routes inside a localized block will be generated for both locales

@dvkch
Copy link
Author

dvkch commented Oct 30, 2021

@tagliala Well I haven't been much quicker to reply either 😬

If that's the intended behavior on your end then it's not a bug, but maybe it would be nice to have this behavior clearly explained in the README, if you have the time.

An amazing feature would be to allow skipping routes that don't have a translated path available for a given locale, to allow supporting (possibly minor) use-cases like this one.

In any case, thanks for your reply!

@tagliala
Copy link
Collaborator

Hi @dvkch I read this again and I think it is an issue. Will investigate further

tagliala added a commit to diowa/ruby3-rails6-bootstrap-heroku that referenced this issue Nov 14, 2021
tagliala added a commit to diowa/ruby3-rails6-bootstrap-heroku that referenced this issue Nov 14, 2021
@tagliala
Copy link
Collaborator

tagliala commented Nov 14, 2021

Hi, I think I've found the issue but I'm not confident to do the changes because I'm not using route_translator in production since years

We have two points to discuss

1. Raise on translation error

I18n.t str, **opts.merge(fallback_options(str, locale))

The above lines should raise an error, so translate_path can intercept it and behave accordingly

This new behavior may be considered a breaking change, so I should at least bump up the major version number but I'm afraid of side effects

2. Fallback behaviour

Rails may itself fallback on the default translation, so we should decide if it is correct to do that or give priority to RouteTranslator's setting. I would set fallback to false if disable_fallback is enabled in Route Translator, so config.i18n.fallbacks will not have effect on routes

{ scope: :routes, fallback: true }

Also: why is fallback true when the option disable_fallback is active? 🤷🏼‍♂️
I've checked history and blame and it is unclear to me, that violates the PoLA


With the following diff, all route translator tests are green and your use case is fixed

diff --git a/lib/route_translator/translator/path/segment.rb b/lib/route_translator/translator/path/segment.rb
index c9b5df5..9bb6f68 100644
--- a/lib/route_translator/translator/path/segment.rb
+++ b/lib/route_translator/translator/path/segment.rb
@@ -9,7 +9,7 @@ module RouteTranslator
 
           def fallback_options(str, locale)
             if RouteTranslator.config.disable_fallback && locale.to_s != I18n.default_locale.to_s
-              { scope: :routes, fallback: true }
+              { scope: :routes, fallback: false }
             else
               { scope: :routes, default: str }
             end
@@ -25,7 +25,7 @@ module RouteTranslator
             opts    = { locale: locale, scope: scope }
 
             if I18n.t(str, **opts.merge(exception_handler: handler)).is_a?(I18n::MissingTranslation)
-              I18n.t str, **opts.merge(fallback_options(str, locale))
+              I18n.t! str, **opts.merge(fallback_options(str, locale))
             else
               I18n.t str, **opts
             end

but I should at least add a failing spec for this use case

tagliala added a commit that referenced this issue Nov 14, 2021
1. Do not enable fallback if disable_fallback option is enabled
2. Raise if the translation is not found, so that `translate_path` may
  rescue from it

Close #241
@dvkch
Copy link
Author

dvkch commented Nov 23, 2021

@tagliala That seams perfect! I don't see any issue either in your decision making or the code itself.

Unfortunately I am no longer working on the impacted project and no longer have access to it to run tests. If you wish I can rebuild a similar project myself to test that case, but I think adding the test case in Route Translator suite should be enough to validate this reasoning and prevent regressions

tagliala added a commit that referenced this issue Nov 28, 2021
1. Do not enable fallback if disable_fallback option is enabled
2. Raise if the translation is not found, so that `translate_path` may
  rescue from it

Close #241
@tagliala tagliala added this to the 12.0.0 milestone Nov 28, 2021
@tagliala
Copy link
Collaborator

I've provided a failing test case and released version 12.0.0 because this is a potential breaking change

Thanks for the report

@dvkch
Copy link
Author

dvkch commented Nov 28, 2021

Amazing, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants