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

TranslatorInterface::getLocale() and setLocale() #29530

Closed
fmonts opened this issue Dec 8, 2018 · 8 comments
Closed

TranslatorInterface::getLocale() and setLocale() #29530

fmonts opened this issue Dec 8, 2018 · 8 comments
Labels

Comments

@fmonts
Copy link

@fmonts fmonts commented Dec 8, 2018

The interface Symfony\Contracts\Translation, which replaces Symfony\Component\Translation from Symfony 4.2, doesn't have the getLocale and setLocale methods.

So $translator->getLocale() in a controller still works, but throws a warning in the IDE about method not found.
Since I don't see any reference to this in the UPGRADE file, what should we do?

These methods are useful for example when you are logged in as admin and you're sending an email to an user who has a different language, so the email template needs to be translated into his language, which is different from the current language.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 8, 2018

To get the locale, you should use the request instead.

@xabbuh xabbuh added the Translator label Dec 9, 2018
@mdeboer

This comment has been minimized.

Copy link
Contributor

@mdeboer mdeboer commented Dec 9, 2018

Well yes, to get the requested locale. That is different (from what I understand) from what he's asking. The translation component works independent from the request and should have a getter/setter for the locale, like is implemented but missing from the interface.

@ro0NL

This comment has been minimized.

Copy link
Contributor

@ro0NL ro0NL commented Dec 9, 2018

should have a getter/setter for the locale

IMHO that's an implementation detail of trans(), i.e. which default locale to use if $locale is not provided. So i think we're striving to follow the principle of least knowledge here.

Framework bundle exposes it's default locale, perhaps you can rely on that?

$container->setParameter('kernel.default_locale', $config['default_locale']);

@fmonts

This comment has been minimized.

Copy link
Author

@fmonts fmonts commented Dec 9, 2018

The main issue was on the setter, rather than the getter.

But since trans() has a locale parameter, we can replace

$translator->setLocale('it');
$translator->trans('Click here');

with:

$translator->trans('Click here', [], null, 'it')
@ro0NL

This comment has been minimized.

Copy link
Contributor

@ro0NL ro0NL commented Dec 9, 2018

in that case, i definitively suggest the alternative solution yes. That avoids messing with the translator state in a global sense.

@Shoplifter

This comment has been minimized.

Copy link
Contributor

@Shoplifter Shoplifter commented Dec 10, 2018

For me it's rather the getLocale() that I'm missing. I use it for example in forms to add a locale specific pattern attribute for NumberTypes, or to set the locale for a \Collator to sort an array or as a parameter for \IntlDateFormatter::create.

As @fmonts stated above, this still works, just the IDE shows an error. You can get rid of this by adding a phpdoc comment something like

public function __construct(TranslatorInterface $translator) {
    /** @var TranslatorTrait $translator **/
    $this->locale = $translator->getLocale();
}

Looks strange, but actually the trait is what you get, right?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 10, 2018

@Shoplifter this would be a dirty hack.

Relying on getLocale() on the translator means relying on its current mutability.
Since services should be stateless, this should not be the case and that's actually something that we might fix in the future.

Use Request::getLocale() instead.
Closing as there is no bug here - as explained.

@Shoplifter

This comment has been minimized.

Copy link
Contributor

@Shoplifter Shoplifter commented Dec 10, 2018

@nicolas-grekas Thanks for the clarification. Now I know that it had a smell for a reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.