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

[Contracts] extract LocaleAwareInterface out of TranslatorInterface #29461

Merged
merged 1 commit into from Dec 6, 2018

Conversation

Projects
None yet
8 participants
@nicolas-grekas
Member

nicolas-grekas commented Dec 5, 2018

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 added this to the 4.2 milestone Dec 5, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:locale-aware branch from 82c0363 to 95e0bd2 Dec 5, 2018

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.2 Dec 5, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:locale-aware branch from 95e0bd2 to 73e4a1a Dec 5, 2018

@javiereguiluz

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

@stof

stof approved these changes Dec 5, 2018

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

@Tobion

Tobion approved these changes Dec 5, 2018

@nicolas-grekas nicolas-grekas merged commit 73e4a1a into symfony:4.2 Dec 6, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 6, 2018

bug #29461 [Contracts] extract LocaleAwareInterface out of Translator…
…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 referenced this pull request Dec 6, 2018

Merged

Release v4.2.1 #29493

@Isengo1989

This comment has been minimized.

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');

@goeckus

This comment has been minimized.

goeckus 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'

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:locale-aware branch Dec 10, 2018

@nicolas-grekas nicolas-grekas referenced this pull request Dec 13, 2018

Open

[HttpKernel][Framework] Locale aware services #28929

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment