Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpKernel][Framework] Locale aware services#28929

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

Merged
fabpot merged 1 commit intosymfony:masterfromneghmurken:feature/locale-aware-services
Apr 3, 2019
Merged

[HttpKernel][Framework] Locale aware services#28929

fabpot merged 1 commit intosymfony:masterfromneghmurken:feature/locale-aware-services
Apr 3, 2019

Conversation

@neghmurken
Copy link
Contributor

@neghmurkenneghmurken commentedOct 19, 2018•
edited by nicolas-grekas
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

Added aLocaleAwareInterface (and also a new service tagkernel.locale_aware) to be implemented on services that require the current locale at request time.
Also, refactored the actual Translator service to implement the overmentioned interface

Todo :

  • Documention PR (will be written after some feedback)

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

i think this is a nice approach/feature :)

@nicolas-grekasnicolas-grekas added this to thenext milestoneOct 20, 2018
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

the autoconfiguration should also be updated (each service being an instance of LocaleAwareInterface should get thelocale_aware tag automatically.

@ro0NL
Copy link
Contributor

curious.. what about using the existingLocaleListener for this? Then we can usetagged again maybe?

cc@stof

@javiereguiluz
Copy link
Member

@neghmurken this is an interesting proposal! Thanks for contributing it.

If this is decided to be merged, we'd need some examples of how can we use this new feature in practice (inside a Symfony app and/or as a stand-alone component). If this feature improves an existing feature, please show a before/after example. Thanks a lot!

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm mixed here:

  • on one side this is very pragmatic;
  • on the other side, this is building on top of service mutation - while services should be stateless.

I'd very prefer we could think about a stateless approach instead.

nicolas-grekas added a commit that referenced this pull requestDec 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
@nicolas-grekas
Copy link
Member

I completely swapped my mind here and actually, we mergedLocaleAwareInterface in Contracts v1.0.2 as a bug fix, see#29461

Would you mind rebasing, please?

@nicolas-grekasnicolas-grekas changed the title[Translation] [Framework] Locale aware services[HttpKernel][Framework] Locale aware servicesDec 14, 2018
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks, here are some comments to help move forward.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJan 26, 2019•
edited
Loading

Small rebase needed + lowest version of httpkernel should be bumped in framework bundle to make tests pass.
Good to me after that!

<argumenttype="service"id="Psr\Container\ContainerInterface" />
</service>

<serviceid="translator_listener"class="Symfony\Component\HttpKernel\EventListener\TranslatorListener">
Copy link
Member

Choose a reason for hiding this comment

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

Should we deprecate the service instead in case someone is relying on its presence?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

How can we deprecate without it conflicting with the newLocaleAwareListener ?

Copy link
Member

Choose a reason for hiding this comment

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

Does it do any harm if both listeners are executed?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah you're right. There is no undesirable side effect beside setting the locale twice

Choose a reason for hiding this comment

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

I removed thetranslator_listener service completely as keeping it would trigger deprecation notices. I don't think that's an issue.

Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

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

looks good apart from a few remaining minor comments

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Green. I rebased this PR on latest master and made some tweaks to polish it. I removed thetranslator_listener service completely as keeping it would trigger deprecation notices. I don't think that's an issue.

@fabpot
Copy link
Member

Thank you@neghmurken.

@fabpotfabpot merged commitb9ac645 intosymfony:masterApr 3, 2019
fabpot added a commit that referenced this pull requestApr 3, 2019
…ken)This PR was merged into the 4.3-dev branch.Discussion----------[HttpKernel][Framework] Locale aware services| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Added a `LocaleAwareInterface` (and also a new service tag `kernel.locale_aware`) to be implemented on services that require the current locale at request time.Also, refactored the actual Translator service to implement the overmentioned interfaceTodo :* [ ] Documention PR (will be written after some feedback)Commits-------b9ac645 Locale aware service registration
@neghmurkenneghmurken deleted the feature/locale-aware-services branchApril 9, 2019 13:39
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
@winzou
Copy link

Hello@neghmurken,@nicolas-grekas
I'm not sure to understand this block in theonKernelFinishRequest method:

    if (null === $parentRequest = $this->requestStack->getParentRequest()) {        $this->setLocale($event->getRequest()->getDefaultLocale());        return;    }

My usecase:

  • I have a request with locale=fr and defaultLocale=en
  • No subquery (hence a null$parentRequest in the above block)
  • all strings translated in templates are done infr, that's perfect
  • but I also have strings translated in an event subscriber during thekernel.terminate event. These strings are translated inen => this is not what I want.

In the if I pasted, during thekernel.finish event, theLocaleAwareListener is defining the locale aware services (translator in my case) to the value of the request defaultLocale. Why? Why changing unilaterally the current locale of these services by thedefault locale of the request? If I define mycurrent locale tofr, I wantfr until the very end of the process, right?

This was not mentionned in the changelog and looks like a BC break (and a bug?) to me. Am I missing something?

Thanks for your insights!

@xabbuh
Copy link
Member

@winzou Would you mind opening a new issue with your insights? Otherwise I am afraid that your input is likely to get lost.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh requested changes

@stofstofstof requested changes

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@jvasseurjvasseurjvasseur left review comments

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

10 participants

@neghmurken@ro0NL@javiereguiluz@nicolas-grekas@fabpot@winzou@xabbuh@stof@jvasseur@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp