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

Version 2.15.0 is broken by an I18n change #2202

Closed
pond opened this issue Nov 25, 2020 · 6 comments · Fixed by #2203
Closed

Version 2.15.0 is broken by an I18n change #2202

pond opened this issue Nov 25, 2020 · 6 comments · Fixed by #2203

Comments

@pond
Copy link

pond commented Nov 25, 2020

Describe the bug

2.14.0 -> 2.15.0 breaks internationalisation; the change in https://github.com/faker-ruby/faker/pull/2169/files is invalid.

To Reproduce

Gemfile:

gem 'i18n'
gem 'faker'

Script:

require 'i18n'
require 'faker'

I18n.locale = 'en-NZ'

puts 'First name:'
puts Faker::Name.first_name

Result with 2.14.0, or with 2.15.0 but with https://github.com/faker-ruby/faker/pull/2169/files reverted:

First name:
Amaia

Result with 2.15.0 as released:

Traceback (most recent call last):
	3: from go.rb:4:in `<main>'
	2: from /home/foo/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/i18n-1.8.5/lib/i18n.rb:59:in `locale='
	1: from /home/foo/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/i18n-1.8.5/lib/i18n/config.rb:15:in `locale='
/home/foo/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/i18n-1.8.5/lib/i18n.rb:342:in `enforce_available_locales!': "en-NZ" is not a valid locale (I18n::InvalidLocale)

Expected behavior

The non-exception behaviour is expected.

Additional context

The old code was simple, terse and reliable - albeit heavy-handed! I certainly do appreciate the attempt to improve efficiency, but I think given the resulting high line count and complexity, coupled with the many issues around lazy loading etc. especially in very complex environments such as Rails, that reverting https://github.com/faker-ruby/faker/pull/2169/files could be the fastest way to resolve this until (or if) a more robust solution can be produced. In the mean time we are forced to hard-lock our Gemfile to precisely version 2.14.0.

@vbrazo
Copy link
Member

vbrazo commented Nov 25, 2020

@pond could you please review this PR? #2203

@pond
Copy link
Author

pond commented Nov 25, 2020

@vbrazo +1 from me.

@amatsuda
Copy link
Contributor

@pond
First of all, as the author of #2169, I'm so sorry for breaking your application.

But as far as I can tell, your situation happens only when the given locale en-NZ is not included in I18n.available_locales, which means, none of your I18n resources in your I18n.load_path are en-NZ based, but you're specifying en-NZ as the I18n.locale.
Is that how your application is setup? If so, I should admit that I've never imagined such a use case. And again, I should apologize my lack of imagination.

But could you teach me why is your application configured like that?
Or am I missing anything?

@amatsuda
Copy link
Contributor

Just to make sure, I confirmed that adding something like
I18n.load_path << 'config/locales/en-nz.yml' # A file that contains any en-NZ resource
above
I18n.locale = 'en-NZ' in your script could make it normally finish.
That is to say, all my assumptions written in the above comment seems technically correct.

@vbrazo
Copy link
Member

vbrazo commented Nov 25, 2020

@amatsuda we should add a note for that here. Since it's a breaking change we should release on a major version that's why I did the quick rollback.

@pond
Copy link
Author

pond commented Nov 25, 2020

Have bumped up to 2.15.1 on our code base; this passed our large test suite via AWS CI in full - thanks for the quick action here, awesome stuff.

@amatsuda Faker itself provides en-NZ translations. If it doesn't make these visible to the I18n system, it's IMHO broken.

Our application has en-NZ available "in full" (e.g. in the server), but in the RSpec test suite does not necessarily always have that. I note a shorter list at some points in the test suite. Who knows why - that's what I mean when I refer to the difficulties involved with autoloading etc. in Rails. It doesn't matter if I use 2.15.0 or 2.15.1 in that case; one of our tests that fail in 2.15.0 and passes in 2.15.1 shows this:

(byebug) I18n.available_locales
[:en, :cs, :az, :ca, :"pt-BR", :bg, :es, :hu, :da, :fi, :id, :ro, :sk, :de, :it, :fa, :el, :ja, :ru, :fr, :"zh-TW", :ar, :tr, :"zh-CN", :nl, :ko, :pt, :he, :pl, :nb, :km, :sv, :"zh-HK", :"sv-SE"]

Contrast with a debug halt during request processing in Puma:

(byebug) I18n.available_locales
[:en, :cs, :az, :ca, :"pt-BR", :bg, :es, :hu, :da, :fi, :id, :ro, :sk, :de, :it, :fa, :el, :ja, :ru, :fr, :"zh-TW", :ar, :tr, :"zh-CN", :nl, :"ca-CAT", :uk, :"en-ZA", :"en-CA", :"en-TH", :"en-NG", :"en-au-ocker", :"de-CH", :th, :"fr-CA", :"de-AT", :"da-DK", :ko, :ee, :pt, :he, :pl, :sv, :"en-SG", :"en-GB", :"nb-NO", :"en-BORK", :hy, :"en-IND", :lv, :"en-UG", :"en-NZ", :"en-AU", :"es-MX", :"en-NEP", :"en-PAK", :"en-US", :vi, :"fr-CH", :"no-NO", :"fi-FI", :"en-MS", :nb, :km, :"zh-HK", :"sv-SE"]

All that aside, the real cause of the issue in our case was phone number generation.

In order for Faker to build a phone number for a given locale, one must set Faker's locale - even though to the casual eye, no actual message lookup might be occurring (I presume in Faker's case it's looking up a format string).

irb(main):023:0> Faker::Config.locale = 'en'
=> "en"
irb(main):024:0> Faker::PhoneNumber.cell_phone
=> "207-189-8365"
irb(main):025:0> Faker::Config.locale = 'fr'
=> "fr"
irb(main):026:0> Faker::PhoneNumber.cell_phone
=> "07 16 86 15 76"
irb(main):027:0> Faker::Config.locale = 'en-NZ'
=> "en-NZ"
irb(main):028:0> Faker::PhoneNumber.cell_phone
Traceback (most recent call last):
        1: from (irb):28
I18n::InvalidLocale ("en-NZ" is not a valid locale)

I don't care if our application has I18n messages for en-NZ, I just want Faker to generate a New Zealand cellphone number, because it has that capability. Faker should make sure this works, IMHO, by ensuring the I18n system has load paths that include files apparently essential to Faker's operation.

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

Successfully merging a pull request may close this issue.

3 participants