Skip to content

[FrameworkBundle][Translation] add LocaleSwitcher service #45793

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

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 19, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #35925
License MIT
Doc PR todo

This adds a LocaleSwitcher object/service. When setting it's locale, it sets it for all kernel.locale_aware services, \Locale, and, if applicable, the _locale parameter for the RequestContext. A switchLocale() convenience method exists to do set the locale, execute a callback, and set the locale back to the original.

Usage:

/** @var Symfony\Component\Translation\LocaleSwitcher $switcher */

$swicher->getLocale(); // kernel.defaul_locale

$switcher->setLocale('fr');

$switcher->getLocale(); // "fr"

$switcher->runWithLocale('de', function() use ($switcher) {
    $switcher->getLocale(); // "de"
});

$switcher->getLocale(); // "fr"

Todo:

  • tests

@carsonbot carsonbot added this to the 6.1 milestone Mar 19, 2022
@carsonbot carsonbot changed the title [Translation][FrameworkBundle] add LocaleSwitcher service [FrameworkBundle][Translation] add LocaleSwitcher service Mar 19, 2022
@kbond kbond force-pushed the locale-switcher branch 2 times, most recently from 8b9e60e to acf8082 Compare March 19, 2022 14:07
Copy link
Contributor

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@kbond kbond force-pushed the locale-switcher branch from acf8082 to 627351a Compare March 20, 2022 11:41
@kbond kbond force-pushed the locale-switcher branch 3 times, most recently from d09e0d3 to 83379b8 Compare March 22, 2022 13:45
@kbond
Copy link
Member Author

kbond commented Mar 22, 2022

Tests added, fabbot failure is unrelated to this PR.

@kbond kbond force-pushed the locale-switcher branch from 83379b8 to 4dc7654 Compare March 22, 2022 13:48
@kbond kbond force-pushed the locale-switcher branch from 4dc7654 to 90f82c0 Compare March 22, 2022 13:55
@fabpot
Copy link
Member

fabpot commented Mar 26, 2022

Thank you @kbond.

@nicolas-grekas
Copy link
Member

I'm proposing #46045 on top of this PR.

fabpot added a commit that referenced this pull request Apr 14, 2022
…ekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[Translation] Improve LocaleSwitcher a bit

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Today's blog post made me review #45793. Here are changes I think would be nice doing.

This is an edgy feature btw to me because it builds on mutable services. An idea that I did not submit here would be to rename LocaleSwitcher to LocaleProvider and tell ppl that need the locale to use this instead of storing the locale as a string in a property. Dunno if that'd make sense to others.

Commits
-------

a71f328 [Translation] Improve LocaleSwitcher a bit
@fabpot fabpot mentioned this pull request Apr 15, 2022
nicolas-grekas added a commit that referenced this pull request Mar 7, 2023
…leSwitcher in a class_exists call (larowlan)

This PR was merged into the 6.2 branch.

Discussion
----------

[Translation] Wrap call to \Locale::setDefault from LocaleSwitcher in a class_exists call

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | N/a - see below
| License       | MIT
| Doc PR        | n/a

In #45793 a new LocaleSwitcher was added which uses the \Locale class from ext-intl.
On upgrading an English only project from 5.4 to 6.2, I received the following error as we don't have the ext-intl extension

```
Error: Class "Locale" not found
```

I searched for previous PRs to add a dependency on `ext-intl` and [came across one](#49280) for the string component directing the user to install a polyfill.

Should symfony/translation therefore depend on the polyfill - otherwise updating is broken without manually installing the polyfill?

If so, here's a PR for that.

Keep up the good work folks ❤️

Commits
-------

c043e93 Wrap use of \Locale in a class_exists test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Translator] Add LocaleSwitcher Service
6 participants