Support falling back to 2 letter language code locales and fix locale leaks across requests #1711
Conversation
nickpith
changed the title
nickpith
force-pushed
the
fallback_to_2_letter_language_code
branch
from
0d5d653
to
881e76b
Compare
In the `ShopifyApp::Localization` concern, an app must specify the available locales exactly the way they expect them to be requested. If an app specifies that only the 2 letter language codes (i.e. `es`) are available, it won't get `I18n.locale` set to that language if given fully qualified locale (i.e. `es-ES`).
Rails does not recommend using `I18n.locale =` to set the locale for controllers because it can leak in multi-threaded applications across requests. See https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests. Instead, `I18n.with_locale` should be used with an `around_action`. Fixes #635.
nickpith
force-pushed
the
fallback_to_2_letter_language_code
branch
from
881e76b
to
93a96a7
Compare
| if params[:locale] | ||
| session[:locale] = params[:locale] | ||
| else | ||
| session[:locale] ||= I18n.default_locale |
There was a problem hiding this comment.
Noticed this being removed means the locale is not stored in the session.
Single page apps like (Hotwire-turbo) do not include the locale param on in-app page navigation. The locale is only included when the shopify admin page is reloaded.
There was a problem hiding this comment.
Maybe it's been too long since this change but the session[:locale] is still being set down on line 21?
There was a problem hiding this comment.
Yep - it is being set there.
Think the issue is more with single page apps like react / Hotwire-turbo that don't reload the Shopify iframe for embedded apps. Meaning the locale is not included after the initial page load. The session is overridden on the next page request - set to default.
Previous version was only setting when provided via params, and only setting the session[:locale] if it was not already set via ||=
There was a problem hiding this comment.
Yeah, I see that now. Thanks for pointing that out.
That definitely was not intended. If you want to fix that, go ahead and put up a PR and I'll gladly review. Otherwise, I'll get a fix out tomorrow if that is ok.
There was a problem hiding this comment.
FYI - I've fixed the issue and opening a PR shortly.
There was a problem hiding this comment.
Fix is up at #1878
The changes in #1711 introduced a bug where single-page apps that don't include the locale param in subsequent requests were unable to properly set the locale that was persisted in the user session. Instead they would use the default locale. See https://github.com/Shopify/shopify_app/pull/1711/files#r1664570299 for background.
The changes in #1711 introduced a bug where single-page apps that don't include the locale param in subsequent requests were unable to properly set the locale that was persisted in the user session. Instead they would use the default locale. See https://github.com/Shopify/shopify_app/pull/1711/files#r1664570299 for background.
The changes in #1711 introduced a bug where single-page apps that don't include the locale param in subsequent requests were unable to properly set the locale that was persisted in the user session. Instead they would use the default locale. See https://github.com/Shopify/shopify_app/pull/1711/files#r1664570299 for background.
What this PR does
In the
ShopifyApp::Localizationconcern, an app must specify the available locales exactly the way they expect them to be requested. If an app specifies that only the 2 letter language codes (i.e.es) are available, it won't getI18n.localeset to that language if given fully qualified locale (i.e.es-ES).Also, Rails does not recommend using
I18n.locale =to set the locale for controllers because it can leak in multi-threaded applications across requests. See https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests. Instead,I18n.with_localeshould be used with anaround_action.Fixes #635.
Reviewer's guide to testing
Unit tests verify this change.
Things to focus on
Is there any reason why we would not want to fallback to the 2 letter language code locales?
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary