ruby-i18n/i18n

I18n.t('.') returns hash representing translation values

oorion opened this issue · 4 comments

oorion commented

What I tried to do

  • Tried to run I18n.t('.') or any string that only contains periods

What I expected to happen

  • I expected it to return something telling me there was no associated translation value since we haven't defined one

What actually happened

  • It returns a hash representing all translation values

Versions of i18n, rails, and anything else you think is necessary

It's not clear to me if this is a bug or just undocumented behavior but I would expect the method to return the same type of object regardless of the input.

Bonus points for providing an application or a small code example which reproduces the issue.

Thanks! ❤️

codealchemy commented

📝 Some additional context, this appears to only happen when the key given matches the separator used.

# Ruby 2.7.5
# i18n 1.8.11

>> require 'i18n'
=> true
>> I18n.backend.store_translations('en', foo: { bar: 'baz' })
=> {:foo=>{:bar=>"baz"}}
>> I18n.t('.', raise: true)
=> {:foo=>{:bar=>"baz"}}
>> I18n.t('.', separator: '-', raise: true)
I18n::MissingTranslationData: translation missing: en.no key
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'
>> I18n.t('-', raise: true)
I18n::MissingTranslationData: translation missing: en.-
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'
>> I18n.t('-', separator: '-', raise: true)
=> {:foo=>{:bar=>"baz"}}

Due to how keys are split up when normalizing

i18n/lib/i18n.rb

Line 409 in a1dc424

keys = key.to_s.split(separator)

Adding a separate case in #normalize_key for when the provided key matches the separator resolves the issue, ie.

        when Array
          key.flat_map { |k| normalize_key(k, separator) }
+      when /^#{Regexp.quote(separator)}+$/
+        [key]
        else

Switching the split to use a regex instead is another approach that could work (and solve #247)

        when Array
          key.flat_map { |k| normalize_key(k, separator) }
        else
-         keys = key.to_s.split(separator) 
+         keys = key.to_s.split(/(?!(\b?|^)#{Regexp.quote(separator)}+$)#{Regexp.quote(separator)}{1}/)
          keys.delete('')

Though not sure if either would be the desired approach (or behavior) given the use case - a custom separator can be passed or configured, and effectively do the same thing:

>> require 'i18n'
=> true
>> I18n.default_separator =  /(?!(\b?|^)\.+$)\.{1}/
=> /(?!(\b?|^)\.+$)\.{1}/
>> I18n.backend.store_translations('en', foo: { bar: 'baz', :'qu
  ix.' => 'quix' })
=> {:foo=>{:bar=>"baz", :"quix."=>"quix"}}
>> I18n.t('foo.quix.')
=> "quix"
>> I18n.t('.', raise: true)
I18n::MissingTranslationData (translation missing: en..)
from /bundle/ruby/2.7.0/gems/i18n-1.8.11/lib/i18n.rb:374:in `handle_exception'
radar commented

@oorion I can confirm that's indeed what happens for me too.

Thank you @codealchemy for the additional context here and along with a suggested fix. Would you like to try your hand at submitting a PR for this?

  • 👍1
codealchemy commented

@radar in looking into this further there looks to be a decent performance hit (14%-52% slower per check) when using a regex in either of the implementations mentioned above. I'm happy to run with one, but given that all calls for translations pass through normalize_key several times wanted to get your thoughts on this prior to a PR.

Measurements 
# Ruby 2.7.3

require 'benchmark/ips'

sample = 'foo.bar.baz.qux.yada.yada'
separator = '.'

Benchmark.ips do |x|
  x.report("split string") do
    sample.split(separator)
  end
  x.report("split regex") do
    sample.split(/(?!(\b|^)#{Regexp.quote(separator)}+$)#{Regexp.quote(separator)}{1}/)
  end
  x.compare!
end
# Warming up --------------------------------------
#         split string   168.847k i/100ms
#          split regex    10.869k i/100ms
# Calculating -------------------------------------
#         split string      1.693M (± 1.8%) i/s -      8.611M in   5.088305s
#          split regex    116.806k (± 1.8%) i/s -    586.926k in   5.026523s

# Comparison:
#         split string:  1692893.6 i/s
#          split regex:   116806.1 i/s - 14.49x  (± 0.00) slower

Benchmark.ips do |x|
  sample_leaf = sample.split(separator)[0]
  x.report("case comp") do
    case sample_leaf
    when separator then ''
    end
  end
  x.report("case regex") do
    case sample_leaf
    when /^#{Regexp.quote(separator)}+$/ then ''
    end
  end
  x.compare!
end

# Warming up --------------------------------------
#            case comp     1.447M i/100ms
#           case regex    28.170k i/100ms
# Calculating -------------------------------------
#            case comp     14.585M (± 0.7%) i/s -     73.778M in   5.058784s
#           case regex    276.301k (± 4.0%) i/s -      1.380M in   5.004822s

# Comparison:
#            case comp: 14584828.3 i/s
#           case regex:   276300.8 i/s - 52.79x  (± 0.00) slower
radar commented

If it's going to be that much slower, then no I don't think we could support it. It would essentially undo a lot of the performance improvement work that the Shopify team have been doing.

Workaround here is to not have keys ending in dots.

  • 👍1