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

Regression in 0.9.3 #404

Closed
clemens opened this issue Jan 23, 2018 · 13 comments
Closed

Regression in 0.9.3 #404

clemens opened this issue Jan 23, 2018 · 13 comments

Comments

@clemens
Copy link
Collaborator

@clemens clemens commented Jan 23, 2018

What I tried to do

I have a Rails app with RSpec tests that use Faker (1.8.7) for fake data generation.

I updated i18n to 0.9.3 to have all my gems at their most recent version.

What I expected to happen

I expected the test suite to pass since no changes apart from gem updates were made.

What actually happened

The whole test suite exploded with variations of the following message:

I18n::MissingTranslationData:
  translation missing: en.faker.name.first_name

I could isolate this to i18n: When I downgraded to 0.9.1, everything worked fine – but with 0.9.3 it breaks, so my assumption is that it's related to i18n, not faker.

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

  • Ruby 2.4.2
  • Rails 5.1.4
  • i18n 0.9.3
  • faker 1.8.7

Let me know if you need any further information.

@radar
Copy link
Collaborator

@radar radar commented Jan 23, 2018

@clemens
Copy link
Collaborator Author

@clemens clemens commented Jan 23, 2018

Sure, here are some snippets with what I think are the most important areas:

# config/application.rb
config.i18n.default_locale = :de
config.i18n.locale = :de
config.i18n.available_locales = [:de]

# spec/support/faker.rb
if defined?(Faker)
  # we only set this in environments that support fixture/factory data
  # (i.e. currently development, test and staging)
  DEFAULT_PASSWORD = '12345678' unless defined?(DEFAULT_PASSWORD)

  I18n.enforce_available_locales = false # faker questionably sets this to true
  Faker::Config.locale = :de
end

# spec/factories/users.rb
FactoryBot.define do
  factory :user do
    first_name { Faker::Name.first_name }
    last_name { Faker::Name.last_name }
  end
end

# spec/models/user_spec.rb
RSpec.describe User, type: :model do
  subject(:user) { FactoryBot.create(:user) } # <= whenever a test calls the subject, it explodes
end

Hope that helps.

@radar
Copy link
Collaborator

@radar radar commented Jan 24, 2018

Thanks @clemens. I've attempted to reproduce this issue in a brand new Rails 5.1.4 application: https://github.com/radar/i18n-404-repro

I was following the steps you've graciously provided. I was unable to reproduce this issue. The test in that application is still passing for me:

.

Finished in 0.45536 seconds (files took 1.89 seconds to load)
1 example, 0 failures

Perhaps there's one (or more) steps that are missing? Could you try cloning down the application and see if you could reproduce the issue?

Very interested to get to the bottom of this but it's hard for me to do that if I am unable to reproduce the issue. Thank you for your assistance.

@tneems
Copy link

@tneems tneems commented Jan 27, 2018

I had this same/similar problem it was solved by adding 'en' to our our list of available_locales in the test environment. I18n.enforce_available_locales = false was being set, so this shouldn't have been happening. 🤷‍♂️ YMMV

@clemens
Copy link
Collaborator Author

@clemens clemens commented Jan 27, 2018

@radar I'll try to create a failing app later today/tomorrow.

@tneems The problem is: We're using faker in both development (to fill the database with fake data) and test. While there certainly isn't any harm in adding :en and not enforcing available locales in development, I prefer to have the dev/prod configs differ as little as possible.

@radar
Copy link
Collaborator

@radar radar commented Jan 27, 2018

@clemens
Copy link
Collaborator Author

@clemens clemens commented Jan 27, 2018

@radar Here you go: https://github.com/clemens/i18n-regression

In this setup (i18n 0.9.3), the test fails. When you comment in the line in the Gemfile to enforce i18n 0.9.1, the test passes.

Hope this helps

@clemens
Copy link
Collaborator Author

@clemens clemens commented Jan 27, 2018

I think I even just found the culprit by accident in the very first commit after 0.9.1: 5077ef9.

This seems to be an optimization which doesn't store translations for locales that aren't in the available_locales list. So when faker tries to put in its English translations (which it needs for Faker::Internet), i18n rejects them because en is not in the available_locales list and thus isn't considered relevant.

It looks like https://github.com/stympy/faker/pull/1133/files introduces workarounds in faker tests which become necessary for the same reason.

Based on a gut feeling (I haven't fully reflected this), I see three possibilities to approach this:

  • revert #391 since it was a performance optimization but introduced a regression
  • add a configuration flag to disable the optimization, e.g. store_translations_for_unavailable_locales = true so people can explicitly opt into the optimization
  • probably the best one: fix faker to not rely on English but instead simply use something else (e.g. lorem ipsum words) instead of a company name as a domain_word.
@clemens
Copy link
Collaborator Author

@clemens clemens commented Jan 28, 2018

Thinking about this some more, I actually think the Faker fix should be independent of the fix of this regression.

I could provide PRs for both approaches if desired. Just let me know.

@radar
Copy link
Collaborator

@radar radar commented Jan 30, 2018

Hi @clemens. Thanks for providing some excellent steps to reproduce this issue.

I believe that while there is a change in i18n that introduced this issue, the change is worthwhile keeping as it prevents unused locales from being stored, which can take up memory in the Ruby process and slow down booting of Rails applications. So the fix in i18n will be staying.

I think the way to sort out this problem is to change Faker's behaviour, particularly the with_locale line in this method:

https://github.com/stympy/faker/blob/679fd508c9f41f41b6df62024d67bc2281953395/lib/faker/internet.rb#L17-L20

There is absolutely no guarantee that :en would be an available locale in every application, as you have demonstrated here. I think the wrapping of with_locale is erroneous and should be removed and that will fix this issue that you're seeing, as it will use Faker::Config.locale, which you've configured to be :de.

Faker does have the proper translations (at least in the de locale) to make this method work without the with_locale wrapping.

So what I think you should do is open a PR on faker to remove that with_locale wrapping and then wait for a new release to be made.

As a substitute, may I suggest using SecureRandom.urlsafe_base64(8)?

@radar radar closed this Jan 30, 2018
@wjordan
Copy link
Contributor

@wjordan wjordan commented Feb 12, 2018

This regression also breaks the i18n pipeline in my team's Rails application (code-dot-org/code-dot-org), which sets config.i18n.enforce_available_locales = false in its Rails application initializer.

Even if the performance optimization may be worthwhile, I believe there is still a regression here in that the existing enforce_available_locales setting is no longer being respected. According to "The Public i18n API" section of the Rails i18n API Guide (which seems to be the only reference documentation for this gem), enforce_available_locales is defined as:

Enforce locale whitelisting (true or false)

Given this documentation, the expected behavior is that when enforce_available_locales is false, locale whitelisting would be un-enforced, implying that any optimization enforcing the set of whitelisted locales would not be applied.

@radar
Copy link
Collaborator

@radar radar commented Feb 12, 2018

Thanks for confirming @wjordan. Could you please try 0.9.4? If it still causes an issue, please show me an app which reproduces it and I can take a look.

Thanks!

@wjordan
Copy link
Contributor

@wjordan wjordan commented Feb 13, 2018

@radar here's a minimal repro script in a rails new app that sets enforce_available_locales = false and config.i18n.available_locales to ["en-US"], and calls I18n.t on a key only available in the 'en' locale:

issue-404.sh:

#!/bin/bash
RAILS_PATH=${RAILS_PATH:-/tmp/issue-404}
RAILS_VERSION=${RAILS_VERSION:-5.1.4}
I18N_VERSION=${I18N_VERSION:-0.9.4}

rm -rf ${RAILS_PATH}
rails _${RAILS_VERSION}_ new ${RAILS_PATH} -q > /dev/null
pushd ${RAILS_PATH} > /dev/null
sed -i '/Rails::Application/a config.i18n.enforce_available_locales = false\nconfig.i18n.available_locales = ["en-US"]' config/application.rb
echo "gem 'i18n', '${I18N_VERSION}'" >> Gemfile
bundle update i18n --quiet
DISABLE_SPRING=1 bin/rails r "puts I18n.t 'hello'"
popd > /dev/null
$ I18N_VERSION=0.9.4 ./issue-404.sh; I18N_VERSION=0.9.1 ./issue-404.sh
translation missing: en.hello
Hello world
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants