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

Rails config for raise on missing translations #13832

Merged
merged 1 commit into from Jan 27, 2014
Merged

Rails config for raise on missing translations #13832

merged 1 commit into from Jan 27, 2014

Conversation

kassio
Copy link
Contributor

@kassio kassio commented Jan 24, 2014

Add a config to setup whether raise exception for missing translation or
not.

Fixes #13196

@@ -153,6 +153,10 @@ class Base
# Specify default_formats that can be rendered.
cattr_accessor :default_formats

# Specify whether raise error for missing translations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about something like:

Specify whether an error should be raised for missing translations

@robin850
Copy link
Member

Hello @kassio, thanks for your contribution. This looks like a quite tiny patch that could be pretty useful! However, I'm not a merger so let's wait for some feedback before addressing the comments.

Also, what do you think about changing the changelog entry to something like:

Added `config.action_view.raise_on_missing_translations` to define whether an
error should be raised for missing translations.

@kassio
Copy link
Contributor Author

kassio commented Jan 25, 2014

@robin850 thank's for the comments, I did the changes.

@@ -38,7 +38,8 @@ def translate(key, options = {})

# If the user has specified rescue_format then pass it all through, otherwise use
# raise and do the work ourselves
if options.key?(:raise) || options.key?(:rescue_format)
options[:raise] ||= ActionView::Base.raise_on_missing_translations
if options[:raise] || options.key?(:rescue_format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic can be improved to something like:

options[:raise] ||= ActionView::Base.raise_on_missing_translations

if options.key?(:rescue_format)
  raise_error = options[:rescue_format]
else
  raise_error = options[:raise]
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT:

options[:raise] ||= ActionView::Base.raise_on_missing_translations

raise_error = options[:raise] || options.key?(:rescue_format)
unless raise_error
  options[:raise] = true
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good

Add a config to setup whether raise exception for missing translation or
not.
rafaelfranca added a commit that referenced this pull request Jan 27, 2014
…tions

Rails config for raise on missing translations
@rafaelfranca rafaelfranca merged commit d57ce23 into rails:master Jan 27, 2014
@derekprior
Copy link
Contributor

Thanks @kassio! Can we back port this to 4.0.x and 3.2.x since it was a regression?

@rafaelfranca
Copy link
Member

I consider this is a new feature so I'd not backport. But if we would backport we would only do to 4.0

@kassio kassio deleted the setup-for-raise-missing-translations branch January 27, 2014 19:54
@fguillen
Copy link

In which version of Rails is this commit merged?

In Rails 4.0.2 I have the error:

NoMethodError: undefined method `raise_on_missing_translations=' for ActionView::Base:Class

@carlosantoniodasilva
Copy link
Member

@fguillen it's been merged to master only.

@firedev
Copy link
Contributor

firedev commented Feb 10, 2014

What about disregarding missing translations for default language? #13429

@fguillen
Copy link

Sorry @firedev but I don't know what you mean :/

@firedev
Copy link
Contributor

firedev commented Feb 10, 2014

Well, it is all described in the ticket, since rails 4.0.2 there seem to be no way to work around missing translations.

@fguillen
Copy link

@firedev I'm using your workaround:

# application_helper.rb
if(Rails.env.development? || Rails.env.test?)
  def t(key, opts = {})
    opts = opts.merge(:raise => true)
    I18n.t(key, opts)
  end
end

I know you can't intercept the exception but at least I have an exception :)

@firedev
Copy link
Contributor

firedev commented Feb 10, 2014

I just want <span class="missing_translation" title="Missing translation for en.Welcome"> to not show up for English locale.

@fguillen
Copy link

I see. Looks like you are trying hard: http://stackoverflow.com/questions/12558716/disable-translation-missing-for-english-only-in-rails luck with this!

@firedev
Copy link
Contributor

firedev commented Apr 16, 2014

And by the way I tried to add

config.action_view.raise_on_missing_translations = false

To application.rb, nothing changed.

@edwinv
Copy link
Contributor

edwinv commented Apr 16, 2014

I'm not completely sure what is going on, but I too have issues with a custom exception handler for I18n. We are using an exception handler that stores missing translations in a separate file. Before Rails 4.1, we needed to override the Rails helper to always use raise: false. With the new configuration this can be configured, which is way nicer than a monkey patch.

Unfortunately, our previous setup only works when supplying the rescue_format: true key to the translation helper. Only then the exception handler is called and we right missing translation missing is returned. It seems cumbersome to always include this key in each call to the Rails helper and it completely eliminate all benefits from the configuration setting.

Before Rails 4.1: t(".foobar", raise: false) calls the exception handler in the right way

After upgrading to 4.1: t(".foobar", rescue_format: true) calls the exception handler when config.action_view.raise_on_missing_translations=false is set.

edwinv added a commit to moneybird/i18n-workflow that referenced this pull request Apr 16, 2014
@firedev
Copy link
Contributor

firedev commented Apr 16, 2014

Just to reiterate, before 4.0.2 was out I intercepted missing translations for english and I didn't have to bother about en.yml. Now I do have config.action_view.raise_on_missing_translations=false in my application.rb but the whole page is covered in missing_translation spans.

@edwinv
Copy link
Contributor

edwinv commented Apr 17, 2014

@firedev I think everyone gets your point now... Maybe you can add something valuable to the ticket to help @kassio to debug this matter further. Does you setup work when adding rescue_format: true to your calls to the translation helper?

@firedev
Copy link
Contributor

firedev commented Apr 17, 2014

Sorry, I am just a bit frustrated. Don't see rescue_format: true changing anything.

Also I have tried different error handlers with config.action_view.raise_on_missing_translations=true, this indeed raises I18n::MissingTranslationData error, but I couldn't intercept it.

@tigrish
Copy link
Contributor

tigrish commented Nov 19, 2014

I've created #17676 that fixes this regression.

@edwinv note that config.action_view.raise_on_missing_translations means "raise the exception from ActionView" not, "pass the raise option to I18n". Unfortunately this means that monkepatching raise: false is still necessary to use a custom exception handler.

tigrish added a commit to tigrish/rails that referenced this pull request Nov 19, 2014
Previously, when the `:raise` options was set to `false`, it would get overwritten to `true`, preventing custom exception handlers to be used.
rafaelfranca added a commit that referenced this pull request Jan 5, 2015
…ler_regression

Fix I18n regression introduced by #13832
rafaelfranca added a commit that referenced this pull request Jan 5, 2015
…ler_regression

Fix I18n regression introduced by #13832
rafaelfranca added a commit that referenced this pull request Jan 5, 2015
…ler_regression

Fix I18n regression introduced by #13832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can no longer configure all missing translations to raise in development
10 participants