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.

Sign up for GitHub

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

Jump to bottom

ActionView translate regression since 4.1 when default is missing #19967

Closed
semaperepelitsa opened this issue Apr 30, 2015 · 3 comments · Fixed by #19998
Closed

ActionView translate regression since 4.1 when default is missing #19967

semaperepelitsa opened this issue Apr 30, 2015 · 3 comments · Fixed by #19998

Comments

@semaperepelitsa
Copy link
Contributor

Here is my test case:

case ARGV[0]
when '4.2'
  gem "actionview", "4.2.1"
when '4.1'
  gem "actionview", "4.1.10"
when '4.0'
  gem "actionpack", "4.0.13"
else
  raise KeyError
end

gem "minitest"
require "action_view"
require "active_support/all"
require "i18n"
require "minitest/autorun"

I18n.enforce_available_locales = false
I18n.backend.store_translations(:en, hello: "Hello")

class TranslateTest < Minitest::Unit::TestCase
  include ActionView::Helpers::TranslationHelper

  def test_ok
    assert_equal "Hello", t(:hello)
  end

  def test_missing
    assert_equal "<span class=\"translation_missing\" title=\"translation missing: en.greeting\">Greeting</span>", t(:greeting)
  end

  def test_default
    assert_equal "Hello", t(:greeting, default: :hello)
  end

  def test_default_missing
    assert_equal "<span class=\"translation_missing\" title=\"translation missing: en.welcome\">Welcome</span>", t(:greeting, default: :welcome)
  end
end

The last test is failing since version 4.1.

> ruby -w test.rb 4.0
Run options: --seed 46515

# Running tests:

....

Finished tests in 0.007289s, 548.7721 tests/s, 548.7721 assertions/s.

4 tests, 4 assertions, 0 failures, 0 errors, 0 skips

> ruby -w test.rb 4.1
MiniTest::Unit::TestCase is now Minitest::Test. From test.rb:21:in `<main>'
Run options: --seed 473

# Running:

..E.

Finished in 0.019266s, 207.6196 runs/s, 155.7147 assertions/s.

  1) Error:
TranslateTest#test_default_missing:
I18n::MissingTranslationData: translation missing: en.welcome
    /Users/sema/.gem/ruby/2.2.0/gems/i18n-0.7.0/lib/i18n.rb:311:in `handle_exception'
    /Users/sema/.gem/ruby/2.2.0/gems/i18n-0.7.0/lib/i18n.rb:161:in `translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.1.10/lib/action_view/helpers/translation_helper.rb:68:in `translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.1.10/lib/action_view/helpers/translation_helper.rb:72:in `rescue in translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.1.10/lib/action_view/helpers/translation_helper.rb:38:in `translate'
    test.rb:37:in `test_default_missing'

4 runs, 3 assertions, 0 failures, 1 errors, 0 skips

> ruby -w test.rb 4.2
MiniTest::Unit::TestCase is now Minitest::Test. From test.rb:22:in `<main>'
Run options: --seed 19624

# Running:

.E..

Finished in 0.024629s, 162.4102 runs/s, 121.8076 assertions/s.

  1) Error:
TranslateTest#test_default_missing:
I18n::MissingTranslationData: translation missing: en.welcome
    /Users/sema/.gem/ruby/2.2.0/gems/i18n-0.7.0/lib/i18n.rb:311:in `handle_exception'
    /Users/sema/.gem/ruby/2.2.0/gems/i18n-0.7.0/lib/i18n.rb:161:in `translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.2.1/lib/action_view/helpers/translation_helper.rb:69:in `translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.2.1/lib/action_view/helpers/translation_helper.rb:73:in `rescue in translate'
    /Users/sema/.gem/ruby/2.2.0/gems/actionview-4.2.1/lib/action_view/helpers/translation_helper.rb:39:in `translate'
    test.rb:38:in `test_default_missing'

4 runs, 3 assertions, 0 failures, 1 errors, 0 skips

Is this a feature or a bug?
@imanel
Copy link
Contributor

imanel commented May 3, 2015

Looks like between 4.0 and 4.1 new options was introduced that allows enabling or disabling raise in case of missing translation, with false as default.

But then something went horribly wrong and it looks like this option is now always set to true in case of fallbacks. You can see some discussion about it in #13832.

In theory #17676 was supposed to fix it, but from what I can see it's not doing it in this case. You could fix it by adding :rescue_format => nil, but that's all I can see in there... My theory is that correct line should look like this:

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

@matthewd
Copy link
Member

matthewd commented May 3, 2015

The problem is that (per the comment above that assignment) options[:raise] is being set for the underlying call to I18n.translate -- we need that to raise, so that we'll end up in the right place.

However, having mangled options for that purpose, we're then reusing it when we recurse.

Thus, I suspect we want to add raise: raise_error to the merged hash on that line.

imanel added a commit to imanel/rails that referenced this issue May 3, 2015
@imanel
Handle raise flag in translate when both main and default translation…
87f876d 
… is missing. Fixes rails#19967
@imanel imanel mentioned this issue May 3, 2015
Merged
@imanel
Copy link
Contributor

imanel commented May 3, 2015

#19998 should fix this problem.

imanel added a commit to imanel/rails that referenced this issue May 3, 2015
@imanel
Handle raise flag in translate when both main and default translation…
6f477bb 
… is missing. Fixes rails#19967
@semaperepelitsa semaperepelitsa mentioned this issue May 4, 2015
Closed
imanel added a commit to imanel/rails that referenced this issue May 4, 2015
@imanel
Handle raise flag in translate when both main and default translation…
9c8542b 
… is missing. Fixes rails#19967
semaperepelitsa pushed a commit to semaperepelitsa/rails that referenced this issue May 21, 2015
@imanel @semaperepelitsa
Handle raise flag in translate when both main and default translation…
96eea85 
… is missing. Fixes rails#19967
@semaperepelitsa semaperepelitsa mentioned this issue May 21, 2015
Merged
semaperepelitsa pushed a commit to semaperepelitsa/rails that referenced this issue May 21, 2015
@imanel @semaperepelitsa
Handle raise flag in translate when both main and default translation…
97a527e 
… is missing. Fixes rails#19967
@semaperepelitsa semaperepelitsa mentioned this issue May 21, 2015
Merged
mwatt pushed a commit to mwatt/rails that referenced this issue Jul 2, 2016
@semaperepelitsa
Refactoring translate helper & fix regression rails#19967.
8e900ec 
It would raise error when default translation is missing instead of returning HTML "missing translation" message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle raise flag in translate when both main and default translation is missing. imanel/rails
3 participants
@matthewd @imanel @semaperepelitsa