[Translation] Improve LocaleSwitcher a bit #46045
Conversation
| @@ -164,22 +163,13 @@ | |||
| ->args([service(ContainerInterface::class)]) | |||
| ->tag('container.service_subscriber', ['id' => 'translator']) | |||
| ->tag('kernel.cache_warmer') | |||
| ; | |||
|
|
|||
| if (class_exists(LocaleSwitcher::class)) { | |||
There was a problem hiding this comment.
We should be careful NOT doing conditional definition in the config files please. This logic is for DI extensions. Config files should remain purely declarative. /cc @symfony/mergers for future reviews.
| if (class_exists(LocaleSwitcher::class)) { | ||
| $container->services() | ||
| ->set('translation.locale_switcher', LocaleSwitcher::class) | ||
| ->args([ |
There was a problem hiding this comment.
fixed wrong indentation also
| use Symfony\Contracts\Translation\LocaleAwareInterface; | ||
|
|
||
| /** | ||
| * @author Kevin Bond <kevinbond@gmail.com> | ||
| */ | ||
| final class LocaleSwitcher implements LocaleAwareInterface | ||
| class LocaleSwitcher implements LocaleAwareInterface |
There was a problem hiding this comment.
Because the class is registered as an autowiring alias, it means we're telling ppl they can type-hint for it. Type-hinting for a final class breaks many SOLID principles, so I removed the keyword.
There was a problem hiding this comment.
Imho, we should register LocaleAwareInterface as an autowiring alias, and not advocate typehinting implementations.
There was a problem hiding this comment.
That works for get/setLocale, but runWithLocale isn't in any interfaces, and I'm not sure we want to add one for it.
I'm fine type-hinting with the implem personnaly, but only if it's open for inheritance of course.
| { | ||
| $original = $this->getLocale(); | ||
| $this->setLocale($locale); | ||
|
|
||
| try { | ||
| $callback(); | ||
| return $callback(); |
There was a problem hiding this comment.
Let's return the callback's result.
| } finally { | ||
| $this->setLocale($original); | ||
| } | ||
| } | ||
|
|
||
| public function reset(): void |
There was a problem hiding this comment.
Statefull services must at least reset their state.
Although the default locale is already set back in LocaleAwareListener::onKernelFinishRequest(), this doesn't cover eg message handlers.
There was a problem hiding this comment.
Do we need to implement ResetInterface?
There was a problem hiding this comment.
It's not mandatory: ->tag('kernel.reset', ['method' => 'reset'])
|
well, we have a stateful place storing the locale anyway ( |
e270317 to
c0b8c15
Compare
LocaleSwitcher should be autowired as LocaleAwareInterface! |
c0b8c15 to
822a0a2
Compare
Doesn't every |
822a0a2 to
a71f328
Compare
yes, that's what I meant by "builds on mutable services" ;) |
|
Thank you @nicolas-grekas. |
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.