Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
82c0363 to95e0bd2Compare95e0bd2 to73e4a1aCompare
javiereguiluz left a comment
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.
stof left a comment
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
Isengo1989 commentedDec 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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'); |
JensGck commentedDec 7, 2018
Just define the interface in services yaml. I'm not sure if this is the best way, but it will work: |
gmponos commentedDec 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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, theyshould 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.