Skip to content

[Contracts] extract LocaleAwareInterface out of TranslatorInterface #29461

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
Dec 6, 2018

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

While reviewing #28929, I realized we have a design issue in the Translation contract: we missed segregating setLocale()/getLocale() out of TranslatorInterface. E.g. when people type-hint for TranslatorInterface, they should not be auto-suggested with the setLocale() mutator.

Technically, that's a BC break. I think we should do it. The release is close enough in time and that will save us from maintenance issues this will create in the future otherwise.

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.2 December 5, 2018 08:06
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

You explained it perfectly: there may be some (little) pain in this BC change ... but it's tiny compared to its benefits.

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

The pain will only be for people having fully migrated away from the old TranslatorInterface. So I'm fine with doing that.

@nicolas-grekas nicolas-grekas merged commit 73e4a1a into symfony:4.2 Dec 6, 2018
nicolas-grekas added a commit that referenced this pull request Dec 6, 2018
…Interface (nicolas-grekas)

This PR was merged into the 4.2 branch.

Discussion
----------

[Contracts] extract LocaleAwareInterface out of TranslatorInterface

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

While reviewing #28929, I realized we have a design issue in the Translation contract: we missed segregating `setLocale()`/`getLocale()` out of `TranslatorInterface`. E.g. when people type-hint for `TranslatorInterface`, they *should not* be auto-suggested with the `setLocale()` mutator.

Technically, that's a BC break. I think we should do it. The release is close enough in time and that will save us from maintenance issues this will create in the future otherwise.

Commits
-------

73e4a1a [Contracts] extract LocaleAwareInterface out of TranslatorInterface
@fabpot fabpot mentioned this pull request Dec 6, 2018
@Isengo1989
Copy link

Isengo1989 commented Dec 6, 2018

Shouldn`t be the new Service be autowired by default? I updated my project and switched the setLocale() of the TranslatorInterface with the LocaleAwareInterface but I am getting:

Cannot autowire argument $locale of "App\Controller\UsersController::test()": it references interface "Symfony\Contracts\Translation\LocaleAwareInterface" but no such service exists.

when using $locale->setLocale('en');

@JensGck
Copy link

JensGck commented Dec 7, 2018

Shouldn`t be the new Service be autowired by default? I updated my project and switched the setLocale() of the TranslatorInterface with the LocaleAwareInterface but I am getting:

Cannot autowire argument $locale of "App\Controller\UsersController::test()": it references interface "Symfony\Contracts\Translation\LocaleAwareInterface" but no such service exists.

when using $locale->setLocale('en');

Just define the interface in services yaml. I'm not sure if this is the best way, but it will work:

Symfony\Contracts\Translation\LocaleAwareInterface:
    alias: 'translator.default'

@gmponos
Copy link
Contributor

gmponos commented Dec 24, 2018

@nicolas-grekas I recently created a new service that I wanted to make it aware of the locale.

Let's say my service was EmailSender. I tried to have this class implement LocaleAwareInterface but since this interface is under Translation namespace it does not feel correct to have a service about emails implement an Interface for translation. Furthermore the locale is not used there to define the translation but to define the receiver.

So I was wondering if this could be improved even further and have the LocaleAwareInterface somewhere else.

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.

9 participants