Skip to content
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

[HttpKernel][Framework] Locale aware services #28929

Merged
merged 1 commit into from Apr 3, 2019

Conversation

@neghmurken
Copy link
Contributor

commented Oct 19, 2018

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 interface

Todo :

  • Documention PR (will be written after some feedback)
@ro0NL
Copy link
Contributor

left a comment

i think this is a nice approach/feature :)

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018

@ro0NL
Copy link
Contributor

left a comment

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

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

curious.. what about using the existing LocaleListener for this? Then we can use tagged again maybe?

cc @stof

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@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!

@nicolas-grekas
Copy link
Member

left a comment

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 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
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

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

Would you mind rebasing, please?

@nicolas-grekas nicolas-grekas changed the title [Translation] [Framework] Locale aware services [HttpKernel][Framework] Locale aware services Dec 14, 2018

@nicolas-grekas
Copy link
Member

left a comment

thanks, here are some comments to help move forward.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

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

Show resolved Hide resolved ...onent/HttpKernel/DependencyInjection/RegisterLocaleAwareServicesPass.php Outdated
@@ -140,11 +141,5 @@
<tag name="kernel.cache_warmer" />
<argument type="service" id="Psr\Container\ContainerInterface" />
</service>

<service id="translator_listener" class="Symfony\Component\HttpKernel\EventListener\TranslatorListener">

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 27, 2019

Member

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

This comment has been minimized.

Copy link
@neghmurken

neghmurken Jan 29, 2019

Author Contributor

How can we deprecate without it conflicting with the new LocaleAwareListener ?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jan 29, 2019

Member

Does it do any harm if both listeners are executed?

This comment has been minimized.

Copy link
@neghmurken

neghmurken Feb 5, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 3, 2019

Member

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

@xabbuh
Copy link
Member

left a comment

looks good apart from a few remaining minor comments

@nicolas-grekas
Copy link
Member

left a comment

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

@fabpot

fabpot approved these changes Apr 3, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thank you @neghmurken.

@fabpot fabpot merged commit b9ac645 into symfony:master Apr 3, 2019

3 checks passed

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

fabpot added a commit that referenced this pull request Apr 3, 2019

feature #28929 [HttpKernel][Framework] Locale aware services (neghmur…
…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 interface

Todo :

* [ ] Documention PR (will be written after some feedback)

Commits
-------

b9ac645 Locale aware service registration

@neghmurken neghmurken deleted the neghmurken:feature/locale-aware-services branch Apr 9, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.