-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
82c0363
to
95e0bd2
Compare
95e0bd2
to
73e4a1a
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
…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
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:
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:
|
@nicolas-grekas I recently created a new service that I wanted to make it Let's say my service was So I was wondering if this could be improved even further and have the |
While reviewing #28929, I realized we have a design issue in the Translation contract: we missed segregating
setLocale()
/getLocale()
out ofTranslatorInterface
. E.g. when people type-hint forTranslatorInterface
, they should not be auto-suggested with thesetLocale()
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.