From d81926fdacbeb384d211e22cff66195c1a753eb5 Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Wed, 5 Aug 2020 10:27:50 -0500 Subject: [PATCH] Improve Action View `translate` helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This disentangles the control flow between Action View's `translate` and I18n's `translate`. In doing so, it fixes a handful of corner cases, for which tests have now been added. It also reduces memory allocations, and improves speed when using a default: **Memory** ```ruby require "benchmark/memory" Benchmark.memory do |x| x.report("warmup") { translate(:"translations.foo"); translate(:"translations.html") } x.report("text") { translate(:"translations.foo") } x.report("html") { translate(:"translations.html") } x.report("text 1 default") { translate(:"translations.missing", default: :"translations.foo") } x.report("html 1 default") { translate(:"translations.missing", default: :"translations.html") } x.report("text 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.foo"]) } x.report("html 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.html"]) } end ``` Before: ``` text 1.240k memsize ( 0.000 retained) 13.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) html 1.600k memsize ( 0.000 retained) 19.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) text 1 default 4.728k memsize ( 1.200k retained) 39.000 objects ( 4.000 retained) 5.000 strings ( 0.000 retained) html 1 default 5.056k memsize ( 1.160k retained) 41.000 objects ( 3.000 retained) 4.000 strings ( 0.000 retained) text 2 defaults 7.464k memsize ( 2.392k retained) 54.000 objects ( 6.000 retained) 4.000 strings ( 0.000 retained) html 2 defaults 7.944k memsize ( 2.384k retained) 60.000 objects ( 6.000 retained) 4.000 strings ( 0.000 retained) ``` After: ``` text 952.000 memsize ( 0.000 retained) 9.000 objects ( 0.000 retained) 1.000 strings ( 0.000 retained) html 1.008k memsize ( 0.000 retained) 10.000 objects ( 0.000 retained) 1.000 strings ( 0.000 retained) text 1 default 2.400k memsize ( 40.000 retained) 24.000 objects ( 1.000 retained) 4.000 strings ( 0.000 retained) html 1 default 2.464k memsize ( 0.000 retained) 22.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) text 2 defaults 3.232k memsize ( 0.000 retained) 30.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) html 2 defaults 3.456k memsize ( 0.000 retained) 32.000 objects ( 0.000 retained) 2.000 strings ( 0.000 retained) ``` **Speed** ```ruby require "benchmark/ips" Benchmark.ips do |x| x.report("text") { translate(:"translations.foo") } x.report("html") { translate(:"translations.html") } x.report("text 1 default") { translate(:"translations.missing", default: :"translations.foo") } x.report("html 1 default") { translate(:"translations.missing", default: :"translations.html") } x.report("text 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.foo"]) } x.report("html 2 defaults") { translate(:"translations.missing", default: [:"translations.missing", :"translations.html"]) } end ``` Before: ``` text 35.685k (± 0.7%) i/s - 179.050k in 5.017773s html 28.569k (± 3.1%) i/s - 143.871k in 5.040128s text 1 default 13.953k (± 2.0%) i/s - 70.737k in 5.071651s html 1 default 12.507k (± 0.4%) i/s - 63.546k in 5.080908s text 2 defaults 9.103k (± 0.3%) i/s - 46.308k in 5.087323s html 2 defaults 8.570k (± 4.3%) i/s - 43.071k in 5.034322s ``` After: ``` text 36.694k (± 2.0%) i/s - 186.864k in 5.094367s html 30.415k (± 0.5%) i/s - 152.900k in 5.027226s text 1 default 18.095k (± 2.7%) i/s - 91.086k in 5.036857s html 1 default 15.934k (± 1.7%) i/s - 80.223k in 5.036085s text 2 defaults 12.179k (± 0.6%) i/s - 61.659k in 5.062910s html 2 defaults 11.193k (± 2.1%) i/s - 56.406k in 5.041433s ``` --- .../action_view/helpers/translation_helper.rb | 98 +++++++++---------- .../test/template/translation_helper_test.rb | 47 ++++++++- 2 files changed, 89 insertions(+), 56 deletions(-) diff --git a/actionview/lib/action_view/helpers/translation_helper.rb b/actionview/lib/action_view/helpers/translation_helper.rb index f292b475ef..513290275b 100644 --- a/actionview/lib/action_view/helpers/translation_helper.rb +++ b/actionview/lib/action_view/helpers/translation_helper.rb @@ -1,8 +1,7 @@ # frozen_string_literal: true require "action_view/helpers/tag_helper" -require "active_support/core_ext/string/access" -require "i18n/exceptions" +require "active_support/core_ext/symbol/starts_ends_with" module ActionView # = Action View Translation Helpers @@ -69,58 +68,43 @@ module TranslationHelper # resolved against. # def translate(key, **options) - unless options[:default].nil? - remaining_defaults = Array.wrap(options.delete(:default)).compact - options[:default] = remaining_defaults unless remaining_defaults.first.kind_of?(Symbol) - end + return key.map { |k| translate(k, **options) } if key.is_a?(Array) - # If the user has explicitly decided to NOT raise errors, pass that option to I18n. - # Otherwise, tell I18n to raise an exception, which we rescue further in this method. - # Note: `raise_error` refers to us re-raising the error in this method. I18n is forced to raise by default. - if options[:raise] == false - raise_error = false - i18n_raise = false - else - raise_error = options[:raise] || ActionView::Base.raise_on_missing_translations - i18n_raise = true + alternatives = if options.key?(:default) + options[:default].is_a?(Array) ? options.delete(:default).compact : [options.delete(:default)] end - fully_resolved_key = scope_key_by_partial(key) + options[:raise] = true if options[:raise].nil? && ActionView::Base.raise_on_missing_translations + default = MISSING_TRANSLATION - if html_safe_translation_key?(key) - html_safe_options = html_escape_translation_options(options) - html_safe_options[:default] = MISSING_TRANSLATION unless html_safe_options[:default].blank? + translation = while key + if alternatives.blank? && !options[:raise].nil? + default = NO_DEFAULT # let I18n handle missing translation + end - translation = I18n.translate(fully_resolved_key, **html_safe_options.merge(raise: i18n_raise)) + key = scope_key_by_partial(key) + first_key ||= key - if translation.equal?(MISSING_TRANSLATION) - translated_text = options[:default].first + if html_safe_translation_key?(key) + html_safe_options ||= html_escape_translation_options(options) + translated = I18n.translate(key, **html_safe_options, default: default) + break html_safe_translation(translated) unless translated.equal?(MISSING_TRANSLATION) else - translated_text = html_safe_translation(translation) + translated = I18n.translate(key, **options, default: default) + break translated unless translated.equal?(MISSING_TRANSLATION) end - else - translated_text = I18n.translate(fully_resolved_key, **options.merge(raise: i18n_raise)) - end - if block_given? - yield(translated_text, fully_resolved_key) - else - translated_text - end - rescue I18n::MissingTranslationData => e - if remaining_defaults.present? - translate remaining_defaults.shift, **options.merge(default: remaining_defaults) - else - raise e if raise_error + break alternatives.first if alternatives.present? && !alternatives.first.is_a?(Symbol) - translated_fallback = missing_translation(e, options) + key = alternatives&.shift + end - if block_given? - yield(translated_fallback, scope_key_by_partial(key)) - else - translated_fallback - end + if key.nil? + translation = missing_translation(first_key, options) + key = first_key end + + block_given? ? yield(translation, key) : translation end alias :t :translate @@ -137,13 +121,19 @@ def localize(object, **options) MISSING_TRANSLATION = Object.new private_constant :MISSING_TRANSLATION + NO_DEFAULT = [].freeze + private_constant :NO_DEFAULT + + def self.i18n_option?(name) + (@i18n_option_names ||= I18n::RESERVED_KEYS.to_set).include?(name) + end + def scope_key_by_partial(key) - stringified_key = key.to_s - if stringified_key.start_with?(".") + if key.start_with?(".") if @current_template&.virtual_path @_scope_key_by_partial_cache ||= {} @_scope_key_by_partial_cache[@current_template.virtual_path] ||= @current_template.virtual_path.gsub(%r{/_?}, ".") - "#{@_scope_key_by_partial_cache[@current_template.virtual_path]}#{stringified_key}" + "#{@_scope_key_by_partial_cache[@current_template.virtual_path]}#{key}" else raise "Cannot use t(#{key.inspect}) shortcut because path is not available" end @@ -153,10 +143,11 @@ def scope_key_by_partial(key) end def html_escape_translation_options(options) + return options if options.empty? html_safe_options = options.dup - options.except(*I18n::RESERVED_KEYS).each do |name, value| - unless name == :count && value.is_a?(Numeric) + options.each do |name, value| + unless TranslationHelper.i18n_option?(name) || (name == :count && value.is_a?(Numeric)) html_safe_options[name] = ERB::Util.html_escape(value.to_s) end end @@ -165,7 +156,7 @@ def html_escape_translation_options(options) end def html_safe_translation_key?(key) - /(?:_|\b)html\z/.match?(key.to_s) + /(?:_|\b)html\z/.match?(key) end def html_safe_translation(translation) @@ -176,14 +167,15 @@ def html_safe_translation(translation) end end - def missing_translation(error, options) - keys = I18n.normalize_keys(error.locale, error.key, error.options[:scope]) + def missing_translation(key, options) + keys = I18n.normalize_keys(options[:locale] || I18n.locale, key, options[:scope]) title = +"translation missing: #{keys.join(".")}" - interpolations = options.except(:default, :scope) - if interpolations.any? - title << ", " << interpolations.map { |k, v| "#{k}: #{ERB::Util.html_escape(v)}" }.join(", ") + options.each do |name, value| + unless name == :scope + title << ", " << name.to_s << ": " << ERB::Util.html_escape(value) + end end if ActionView::Base.debug_missing_translation diff --git a/actionview/test/template/translation_helper_test.rb b/actionview/test/template/translation_helper_test.rb index 29c5af6a60..a563378055 100644 --- a/actionview/test/template/translation_helper_test.rb +++ b/actionview/test/template/translation_helper_test.rb @@ -49,9 +49,18 @@ class TranslationHelperTest < ActiveSupport::TestCase end def test_delegates_setting_to_i18n - assert_called_with(I18n, :translate, [:foo, locale: "en", raise: true], returns: "") do + matcher_called = false + matcher = ->(key, options) do + matcher_called = true + assert_equal :foo, key + assert_equal "en", options[:locale] + end + + I18n.stub(:translate, matcher) do translate :foo, locale: "en" end + + assert matcher_called end def test_delegates_localize_to_i18n @@ -229,15 +238,26 @@ def test_translate_marks_array_of_translations_with_a_html_safe_suffix_as_safe_h end end + def test_translate_with_default_and_raise_false + translation = translate(:"translations.missing", default: :"translations.foo", raise: false) + assert_equal "Foo", translation + end + def test_translate_with_default_named_html translation = translate(:'translations.missing', default: :'translations.hello_html') assert_equal "Hello World", translation assert_equal true, translation.html_safe? end + def test_translate_with_default_named_html_and_raise_false + translation = translate(:"translations.missing", default: :"translations.hello_html", raise: false) + assert_equal "Hello World", translation + assert_predicate translation, :html_safe? + end + def test_translate_with_missing_default - translation = translate(:'translations.missing', default: :'translations.missing_html') - expected = 'Missing Html' + translation = translate(:"translations.missing", default: :also_missing) + expected = 'Missing' assert_equal expected, translation assert_equal true, translation.html_safe? end @@ -319,6 +339,27 @@ def test_translate_bulk_lookup assert_equal ["Foo", "Foo"], translations end + def test_translate_bulk_lookup_with_default + translations = translate([:"translations.missing", :"translations.missing"], default: :"translations.foo") + assert_equal ["Foo", "Foo"], translations + end + + def test_translate_bulk_lookup_html + translations = translate([:"translations.html", :"translations.hello_html"]) + assert_equal ["Hello World", "Hello World"], translations + translations.each do |translation| + assert_predicate translation, :html_safe? + end + end + + def test_translate_bulk_lookup_html_with_default + translations = translate([:"translations.missing", :"translations.missing"], default: :"translations.html") + assert_equal ["Hello World", "Hello World"], translations + translations.each do |translation| + assert_predicate translation, :html_safe? + end + end + def test_translate_does_not_change_options options = {} if RUBY_VERSION >= "2.7" -- GitLab