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

Add reasoning for `I18n.with_locale` #34911

Merged
merged 1 commit into from Jan 10, 2019

Conversation

Projects
None yet
3 participants
@duleorlovic
Copy link
Contributor

duleorlovic commented Jan 9, 2019

[ci skip]

Summary

Some explanation why I18n.locale = :es is not a good practice.

Other Information

If someone set locale based on parametar

if params[:locale].present?
  I18n.locale = params[:locale].to_sym
end

Than it could be that localization is set only in thread that serve this request, and it is not cleared at the end, so future requests can be in default locale or in newly locale (depending on which thread is serving it)

@rails-bot rails-bot bot added the docs label Jan 9, 2019

@lsylvester

This comment has been minimized.

Copy link
Contributor

lsylvester commented Jan 10, 2019

The issue with the locale leaking into other requests if it is not consistently set in every controller is not related to thread safety, or Puma. I think that mentioning them implies that if you aren't using a threaded web server then it is not an issue you need to consider, which is not the case.

Maybe something like I18n.locale can leek into subsequent requests served by the same thread/process if it is not consistently set in every controller.

@duleorlovic duleorlovic force-pushed the duleorlovic:guides_i18n branch from 167cc7b to 03fdb3a Jan 10, 2019

@duleorlovic

This comment has been minimized.

Copy link
Contributor Author

duleorlovic commented Jan 10, 2019

@lsylvester yes, you are right. This Puma example is just example that I notice in my test. But I would leave it as an example since this "localization toggling" looks very strange when you first notice it...
I updated explanation with provided sentence.
Feel free to close this pull request if "multithreaded Puma" example is too much for I18n guide.

Show resolved Hide resolved guides/source/i18n.md Outdated
Show resolved Hide resolved guides/source/i18n.md Outdated
Add reasoning for `I18n.with_locale` and explanation that the problem is
about leak into subsequent requests.

[ci skip]

@duleorlovic duleorlovic force-pushed the duleorlovic:guides_i18n branch from b4eb651 to 569a889 Jan 10, 2019

@duleorlovic

This comment has been minimized.

Copy link
Contributor Author

duleorlovic commented Jan 10, 2019

@rafaelfranca yea, it is too much overhead for I18n guide to talk about web servers, so I accepted your suggestions

@rafaelfranca rafaelfranca merged commit fd21b59 into rails:master Jan 10, 2019

1 check passed

codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.