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

EZP-26600: Use the request's languages to translate pjax #730

Merged
merged 2 commits into from Nov 28, 2016

Conversation

@bdunogier
Copy link
Member

commented Nov 24, 2016

Implements EZP-26600

Adds a Request Subscriber that runs before our / symfony's LocaleListener, and sets the request's locale for PJAX requests.

TODO

  • [x] Unit test
@dpobel
dpobel approved these changes Nov 25, 2016
{
$request = $event->getRequest();
if (!$event->isMasterRequest() || !$request->headers->has('x-pjax')) {

This comment has been minimized.

Copy link
@nicolas-bastien

nicolas-bastien Nov 25, 2016

Contributor

x-pjax is lowercase here and uppercase in your test, don't we have constant for that ?

This comment has been minimized.

Copy link
@bdunogier

bdunogier Nov 25, 2016

Author Member

Nope, we do not. As said, while I don't disagree with the usage of constants for this, I don't have a good place for it at the moment. It is also used in another listener, and therefore can't go into this listener. The PJAX controllers do not share any common class that we could use for this.

Would you create an interface or class for this ? I'd suggest that we postpone this for when we will work further on PJAX, and find a suitable place for the constant at this point. What do you think ?

/**
* On PJAX requests, sets the request's locale from the browser's accept-language header.
*/
class PjaxBrowserLanguageSubscriber implements EventSubscriberInterface

This comment has been minimized.

Copy link
@nicolas-bastien

nicolas-bastien Nov 25, 2016

Contributor

In general I would remove the Pjax to something more generic like LocalSubscriber so it would work even if it's not pjax.

And if in the future we have to has a condition based on user settings then it's not only brower languages

This comment has been minimized.

Copy link
@bdunogier

bdunogier Nov 25, 2016

Author Member

The first name I chose was actually based on Locale something.

But given that this implementation/class is meant to act only on Pjax requests, and tests for it, I'd actually keep this name.

I don't disagree, so I'm gonna cut the apple in half: RequestLocaleSubscriber::setPjaxRequestLocale(). In addition, I have added a PjaxRequestMatcher, that has the pjax request matching logic.

@bdunogier bdunogier force-pushed the ezp26600-pjax_i18n_listener branch from c805a79 to 9ea1e38 Nov 28, 2016

@bdunogier bdunogier force-pushed the ezp26600-pjax_i18n_listener branch from 9ea1e38 to cd7c03b Nov 28, 2016

@bdunogier bdunogier merged commit 06ad494 into master Nov 28, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ezrobot Code review by ezrobot
Details

@bdunogier bdunogier deleted the ezp26600-pjax_i18n_listener branch Nov 28, 2016

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