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

Added an option to disable Locale listener #235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

romaricdrigon
Copy link

Hi,

In some case, using the Symfony2 locale (for example from the Request) to automatically filter entities using the Translatable extension can lead to unwanted results.
I came across such case in an application where the locale was already managed explicitly.

In this PR, we create an optional disable_locale_listener option. The default value is true, so there are no BC break.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? ~
Fixed tickets ~
License MIT
Doc PR Doc written

@@ -30,6 +30,7 @@ public function getConfigTreeBuilder()
->booleanNode('translation_fallback')->defaultFalse()->end()
->booleanNode('persist_default_translation')->defaultFalse()->end()
->booleanNode('skip_translation_on_load')->defaultFalse()->end()
->booleanNode('disable_locale_listener')->defaultFalse()->end()
Copy link
Owner

Choose a reason for hiding this comment

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

the double negation is weird. I would use use_request_locale instead, defaulting to true

@stof
Copy link
Owner

stof commented Aug 11, 2015

@romaricdrigon are you still interested in this feature ? If yes, please update it based on the feedback I gave you previously, and please rebase it to fix conflicts

@tyx
Copy link

tyx commented Aug 13, 2015

I can continue the work if needed. I definitively need this feature

@romaricdrigon
Copy link
Author

I was not sure anyone but me was needing that one. Sure I will work on it.

@shouze
Copy link

shouze commented Aug 13, 2015

Definitely a must have 👍

@romaricdrigon
Copy link
Author

Hi all,
I've just updated my code with your feedback @stof , and squashed my commits.

@shouze
Copy link

shouze commented Aug 18, 2015

Oh yeah! can't wait!

@Pierstoval
Copy link

Ping @stof : this can be merged IMO ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants