Inconsistent locale handling in subrequests #6932

Closed
stof opened this Issue Jan 31, 2013 · 32 comments

Comments

Projects
None yet
@stof
Member

stof commented Jan 31, 2013

If the master request uses a locale different from the default locale and the subrequest does not change the locale through the routing, the locale of the subrequest will be the app default locale instead of the master request locale.
This causes some issues as the translator will not change its locale (it retrieves it only once from the request and is in the container scope) but the routing context will change its _locale parameter and the intl default locale will change.
But this will also impact any rendering done in the master request after the subrequest as the locale is not reset to the master one.

An easy workaround a would be to set the locale of the master request as default locale for subrequest instead of the app default locale. But this would make the locale in the subrequest inconsistent when enablign ESI.

A way to fix it in a better way could be to trigger an event after the handling of a subrequest to announce the return to the master request. The request object available in this listener should be the master one (which means it cannot be achieved cleanly with a late kernel.response event listener on the subrequest). This listener would then reset the Intl and routing locale with the value from the request (and any other action needed).
@fabpot what do you think about it ?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 4, 2013

Member

I think the current behavior is the right one as the locale information must be part of the routing (as of Symfony 2.1, we do not support locale management except when part of the routing path)... or am I missing something here?

Member

fabpot commented Feb 4, 2013

I think the current behavior is the right one as the locale information must be part of the routing (as of Symfony 2.1, we do not support locale management except when part of the routing path)... or am I missing something here?

@fabpot fabpot referenced this issue Feb 7, 2013

Merged

[2.3] [WIP] Synchronized services... #7007

2 of 2 tasks complete
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 11, 2013

Member

@fabpot The issue is that a controller reference will not propagate the locale to the subrequest as the proxy route does not have the locale. And you may set the request locale from elsewhere than the routing (the logged user for instance)

Member

stof commented Feb 11, 2013

@fabpot The issue is that a controller reference will not propagate the locale to the subrequest as the proxy route does not have the locale. And you may set the request locale from elsewhere than the routing (the logged user for instance)

@lsmith77 lsmith77 referenced this issue in lunetics/LocaleBundle Feb 27, 2013

Merged

LocaleListener should also handle subrequests #57

fabpot added a commit that referenced this issue Mar 23, 2013

merged branch fabpot/contagious-services (PR #7007)
This PR was merged into the master branch.

Discussion
----------

[2.3] [WIP] Synchronized services...

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5300, #6756
| License       | MIT
| Doc PR        | symfony/symfony-docs#2343

Todo:

 - [x] update documentation
 - [x] find a better name than contagious (synchronized)?

refs #6932, refs #5012

This PR is a proof of concept that tries to find a solution for some problems we have with scopes and services depending on scoped services (mostly the request service in Symfony).

Basically, whenever you want to inject the Request into a service, you have two possibilities:

 * put your own service into the request scope (a new service will be created whenever a sub-request is run, and the service is not available outside the request scope);

 * set the request service reference as non-strict (your service is always available but the request you have depends on when the service is created the first time).

This PR addresses this issue by allowing to use the second option but you service still always has the right Request service (see below for a longer explanation on how it works).

There is another issue that this PR fixes: edge cases and weird behaviors. There are several bug reports about some weird behaviors, and most of the time, this is related to the sub-requests. That's because the Request is injected into several Symfony objects without being updated correctly when leaving the request scope. Let me explain that: when a listener for instance needs the Request object, it can listen to the `kernel.request` event and store the request somewhere. So, whenever you enter a sub-request, the listener will get the new one. But when the sub-request ends, the listener has no way to know that it needs to reset the request to the master one. In practice, that's not really an issue, but let me show you an example of this issue in practice:

 * You have a controller that is called with the English locale;
 * The controller (probably via a template) renders a sub-request that uses the French locale;
 *  After the rendering, and from the controller, you try to generate a URL. Which locale the router will use? Yes, the French locale, which is wrong.

To fix these issues, this PR introduces a new notion in the DIC: synchronized services. When a service is marked as synchronized, all method calls involving this service will be called each time this service is set. When in a scope, methods are also called to restore the previous version of the service when the scope leaves.

If you have a look at the router or the locale listener, you will see that there is now a `setRequest` method that will called whenever the request service changes (because the `Container::set()` method is called or because the service is changed by a scope change).

Commits
-------

17269e1 [DependencyInjection] fixed management of scoped services with an invalid behavior set to null
bb83b3e [HttpKernel] added a safeguard for when a fragment is rendered outside the context of a master request
5d7b835 [FrameworkBundle] added some functional tests
ff9d688 fixed Request management for FragmentHandler
1b98ad3 fixed Request management for LocaleListener
a7b2b7e fixed Request management for RequestListener
0892135 [HttpKernel] ensured that the Request is null when outside of the Request scope
2ffcfb9 [FrameworkBundle] made the Request service synchronized
ec1e7ca [DependencyInjection] added a way to automatically update scoped services
@clementblanco

This comment has been minimized.

Show comment
Hide comment
@clementblanco

clementblanco Mar 28, 2013

Is this solved ? Because I'm currently running the same problem with Symfony 2.2. I've successfully set the locale in the master request but as soon as I use a render inside a Twig template file like :
{{ render(controller('AcmeBlogBundle:Default:index')) }}
the locale in the sub request is the defaullt app one. Not the one I previously set.

Is this solved ? Because I'm currently running the same problem with Symfony 2.2. I've successfully set the locale in the master request but as soon as I use a render inside a Twig template file like :
{{ render(controller('AcmeBlogBundle:Default:index')) }}
the locale in the sub request is the defaullt app one. Not the one I previously set.

@zebraf1

This comment has been minimized.

Show comment
Hide comment
@zebraf1

zebraf1 Mar 28, 2013

Don't know if it is the best solution, but I had to make a workaround for this problem.

onKernelRequest the locale is set to the correct one in main request and in onKernelResponse the locale array is shifted back. Sub-requests do not have _locale parameter (unless explicitly set) so they always use defaultLocale.
I made a rewrite to set mainRequestLocale and use it in any sub-requests here: #5532 (comment)

zebraf1 commented Mar 28, 2013

Don't know if it is the best solution, but I had to make a workaround for this problem.

onKernelRequest the locale is set to the correct one in main request and in onKernelResponse the locale array is shifted back. Sub-requests do not have _locale parameter (unless explicitly set) so they always use defaultLocale.
I made a rewrite to set mainRequestLocale and use it in any sub-requests here: #5532 (comment)

@dewos

This comment has been minimized.

Show comment
Hide comment
@dewos

dewos Apr 21, 2013

+1. Same problem.

dewos commented Apr 21, 2013

+1. Same problem.

@jaymecd

This comment has been minimized.

Show comment
Hide comment
@jaymecd

jaymecd May 2, 2013

+1. Problem still appears for render subrequest from template, initial locale is not passed through by default, have to explicitly type it.

jaymecd commented May 2, 2013

+1. Problem still appears for render subrequest from template, initial locale is not passed through by default, have to explicitly type it.

@dr1ss

This comment has been minimized.

Show comment
Hide comment
@dr1ss

dr1ss May 7, 2013

Something is clearly wierd here.. I'm experiencing the same problem (using version 2.2)... I just noticed it (it was working fine on 2.1.x)..
I'm using ESI includes, basicaly I've the main request and two ESI calls for the header and the footer.. not only the locale is not set on the subrequests ($request->getLocale() always returns the default locale) but I've also noticed that $request->getClientIp() returns 127.0.0.1 in the subqueries controllers not sure if it's related but it's like the subqueries are using a new instance of the request component.

dr1ss commented May 7, 2013

Something is clearly wierd here.. I'm experiencing the same problem (using version 2.2)... I just noticed it (it was working fine on 2.1.x)..
I'm using ESI includes, basicaly I've the main request and two ESI calls for the header and the footer.. not only the locale is not set on the subrequests ($request->getLocale() always returns the default locale) but I've also noticed that $request->getClientIp() returns 127.0.0.1 in the subqueries controllers not sure if it's related but it's like the subqueries are using a new instance of the request component.

@mboughaba

This comment has been minimized.

Show comment
Hide comment
@mboughaba

mboughaba May 7, 2013

I am facing the a very similar issue #7954 where:

After a successful login, the request route is redirected to the originating page "/" and loses the locale parameter. I expected the route to be "/%locale%" but got "/" ; where %locale% is the locale defined by the user on the login or register page.

I am using FOSUserBundle but since the redirection and security is handled by Symfony, I guess I am at the right place.

A have found a way to force setting the locale after login by setting the firewalls in security.yml as below:
firewalls:
main:
pattern: ^/
form_login:
always_use_default_target_path: true
default_target_path: /%locale%
Even using this, the locale will be equal to the default one ('en' in my case).

I am facing the a very similar issue #7954 where:

After a successful login, the request route is redirected to the originating page "/" and loses the locale parameter. I expected the route to be "/%locale%" but got "/" ; where %locale% is the locale defined by the user on the login or register page.

I am using FOSUserBundle but since the redirection and security is handled by Symfony, I guess I am at the right place.

A have found a way to force setting the locale after login by setting the firewalls in security.yml as below:
firewalls:
main:
pattern: ^/
form_login:
always_use_default_target_path: true
default_target_path: /%locale%
Even using this, the locale will be equal to the default one ('en' in my case).

@dr1ss

This comment has been minimized.

Show comment
Hide comment
@dr1ss

dr1ss May 8, 2013

forget about the getClientIp() it was just related to trusted proxies and ESI...
@benamlas the issue you're describing has nothing to do with this either :)
and as @jaymecd suggested, I ended up setting the local within the ESI calls in Twig as a temporary solution :
{{ render_esi(controller( "xxxBundle:Default:footerSection", {'_locale':app.request.locale} )) }}

dr1ss commented May 8, 2013

forget about the getClientIp() it was just related to trusted proxies and ESI...
@benamlas the issue you're describing has nothing to do with this either :)
and as @jaymecd suggested, I ended up setting the local within the ESI calls in Twig as a temporary solution :
{{ render_esi(controller( "xxxBundle:Default:footerSection", {'_locale':app.request.locale} )) }}

@mboughaba

This comment has been minimized.

Show comment
Hide comment
@mboughaba

mboughaba May 8, 2013

@dr1ss tnx for the highlight, but I think it is related:
After login, a redirect to the originating page is performed where the locale will be unset.
When a listener is added to the GetResponseEvent and/or InteractiveLoginEvent, locale will be equal to the default one at the time the events are fired.
I believe that it is indeed to different problem but I think that they share the same root cause.

@dr1ss tnx for the highlight, but I think it is related:
After login, a redirect to the originating page is performed where the locale will be unset.
When a listener is added to the GetResponseEvent and/or InteractiveLoginEvent, locale will be equal to the default one at the time the events are fired.
I believe that it is indeed to different problem but I think that they share the same root cause.

@zebraf1

This comment has been minimized.

Show comment
Hide comment
@zebraf1

zebraf1 May 9, 2013

The router will take the locale from context, since sub request context locale will be set back to default, it will cause any routes generated to be wrong. I had to make my own LocaleListener to remember the main locale and use that in sub-request: #5532 (comment)

zebraf1 commented May 9, 2013

The router will take the locale from context, since sub request context locale will be set back to default, it will cause any routes generated to be wrong. I had to make my own LocaleListener to remember the main locale and use that in sub-request: #5532 (comment)

@statefull

This comment has been minimized.

Show comment
Hide comment
@statefull

statefull May 9, 2013

with 2.2.1 version the subrequest problem persist. My solution was in each called action do:

$this->getRequest()->setLocale($session->get("_locale"));

where in the action which initiates the process I set manually _locale in session from the request.

with 2.2.1 version the subrequest problem persist. My solution was in each called action do:

$this->getRequest()->setLocale($session->get("_locale"));

where in the action which initiates the process I set manually _locale in session from the request.

@dr1ss

This comment has been minimized.

Show comment
Hide comment
@dr1ss

dr1ss May 13, 2013

just tested it with 2.3 Beta 2.. same problem

dr1ss commented May 13, 2013

just tested it with 2.3 Beta 2.. same problem

@alain-flaus alain-flaus referenced this issue in prestaconcept/PrestaCMSCoreBundle May 16, 2013

Merged

Issue #27: transmit manually locale to sf sub-request #42

@berenerchamion

This comment has been minimized.

Show comment
Hide comment
@berenerchamion

berenerchamion May 27, 2013

I have same issue with render of twig tempaltes :-( The locale is lost.

I have same issue with render of twig tempaltes :-( The locale is lost.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad May 27, 2013

+1 same issue in 2.2.* and 2.3.*

+1 same issue in 2.2.* and 2.3.*

@krixer

This comment has been minimized.

Show comment
Hide comment
@krixer

krixer May 28, 2013

I have the same problem with 2.2.* and 2.3 branches :(

krixer commented May 28, 2013

I have the same problem with 2.2.* and 2.3 branches :(

@Mopster

This comment has been minimized.

Show comment
Hide comment
@Mopster

Mopster May 31, 2013

+1
We've been building a language chooser bundle using the LunaticsLocaleBundle locale guesser, but in the subrequests he can't find a locale and falls back to the default one, meaning we have EN dates for NL and FR pages.

Mopster commented May 31, 2013

+1
We've been building a language chooser bundle using the LunaticsLocaleBundle locale guesser, but in the subrequests he can't find a locale and falls back to the default one, meaning we have EN dates for NL and FR pages.

@thomask

This comment has been minimized.

Show comment
Hide comment
@thomask

thomask Jun 1, 2013

+1 on 2.2.*

thomask commented Jun 1, 2013

+1 on 2.2.*

@dewos

This comment has been minimized.

Show comment
Hide comment
@dewos

dewos Jun 9, 2013

+1 on 2.3.*

dewos commented Jun 9, 2013

+1 on 2.3.*

@drm

This comment has been minimized.

Show comment
Hide comment
@drm

drm Jun 20, 2013

Contributor

You can fix this on another level by overriding the FrameworkBundle's HttpKernel (set the http_kernel.class to your custom kernel) and override the related render methods to copy the _locale parameter from the master request. Seems more sensible anyway to have the creation of the subrequest fix the problem, rather than the service container ...?

(edit):
Here's the code. This works in 2.1, but a similar solution would work in newer versions.

<?php
namespace Foo;

use Symfony\Bundle\FrameworkBundle\HttpKernel as BaseHttpKernel;

class HttpKernel extends BaseHttpKernel
{
    public function render($controller, array $options = array())
    {
        /** @var $request \Symfony\Component\HttpFoundation\Request */
        $request = $this->container->get('request');
        if (!isset($options['attributes']['_locale'])) {
            $options['attributes']['_locale']= $request->attributes->get('_locale');
        }
        return parent::render($controller, $options);
    }
}
Contributor

drm commented Jun 20, 2013

You can fix this on another level by overriding the FrameworkBundle's HttpKernel (set the http_kernel.class to your custom kernel) and override the related render methods to copy the _locale parameter from the master request. Seems more sensible anyway to have the creation of the subrequest fix the problem, rather than the service container ...?

(edit):
Here's the code. This works in 2.1, but a similar solution would work in newer versions.

<?php
namespace Foo;

use Symfony\Bundle\FrameworkBundle\HttpKernel as BaseHttpKernel;

class HttpKernel extends BaseHttpKernel
{
    public function render($controller, array $options = array())
    {
        /** @var $request \Symfony\Component\HttpFoundation\Request */
        $request = $this->container->get('request');
        if (!isset($options['attributes']['_locale'])) {
            $options['attributes']['_locale']= $request->attributes->get('_locale');
        }
        return parent::render($controller, $options);
    }
}
@alex-pex

This comment has been minimized.

Show comment
Hide comment
@alex-pex

alex-pex Jun 20, 2013

I have the same problem with sf 2.3.1, rendering controllers.

Instead of
{% render(controller("MyBundle:Slider:show", {'country': country})) %}
I must write
{% render(controller("MyBundle:Slider:show", {'country': country, '_locale': app.request.locale})) %}

This is a weird behaviour, passing the locale automatically would be better. It was annoying to search why the translation were good but my subrequest routes were wrong.

I have the same problem with sf 2.3.1, rendering controllers.

Instead of
{% render(controller("MyBundle:Slider:show", {'country': country})) %}
I must write
{% render(controller("MyBundle:Slider:show", {'country': country, '_locale': app.request.locale})) %}

This is a weird behaviour, passing the locale automatically would be better. It was annoying to search why the translation were good but my subrequest routes were wrong.

@keyermann

This comment has been minimized.

Show comment
Hide comment
@keyermann

keyermann Jun 21, 2013

+1 on 2.3.1

+1 on 2.3.1

@azine

This comment has been minimized.

Show comment
Hide comment
@azine

azine Jun 27, 2013

Contributor

+1 on Symfony 2.2.3

Contributor

azine commented Jun 27, 2013

+1 on Symfony 2.2.3

@samuelvogel

This comment has been minimized.

Show comment
Hide comment
@samuelvogel

samuelvogel Jul 22, 2013

Contributor

The handling of the _locale parameter is also inconsistent between creating a subrequest in a controller vs. in a template. I've created a simple testcase against master/2.4 and 2.3.2 is also affected.
https://github.com/VIISON/symfony-standard/commit/de93558cbf083781314798f589f7c78f5dcedbbe just install it and call web/app_dev.php/fr, you'll get:

Locale in main request: fr

Locale in subrequest by controller forward: fr

Locale in subrequest by template render: en

It seems that #7007 only fixed this for subrequests created by calling forward() in a controller. Don't know if #7063 fixed this in 2.1/2.2, but subrequests made from templates are still broken in >= 2.3.

Contributor

samuelvogel commented Jul 22, 2013

The handling of the _locale parameter is also inconsistent between creating a subrequest in a controller vs. in a template. I've created a simple testcase against master/2.4 and 2.3.2 is also affected.
https://github.com/VIISON/symfony-standard/commit/de93558cbf083781314798f589f7c78f5dcedbbe just install it and call web/app_dev.php/fr, you'll get:

Locale in main request: fr

Locale in subrequest by controller forward: fr

Locale in subrequest by template render: en

It seems that #7007 only fixed this for subrequests created by calling forward() in a controller. Don't know if #7063 fixed this in 2.1/2.2, but subrequests made from templates are still broken in >= 2.3.

@wimvds

This comment has been minimized.

Show comment
Hide comment
@wimvds

wimvds Jul 24, 2013

The workaround mentioned by @alex-pex (ie. explicitly passing _locale when rendering controllers) is what we've been using since Symfony 2.0 (since ESI calls were acting up if we didn't do it)...

wimvds commented Jul 24, 2013

The workaround mentioned by @alex-pex (ie. explicitly passing _locale when rendering controllers) is what we've been using since Symfony 2.0 (since ESI calls were acting up if we didn't do it)...

@stmichael

This comment has been minimized.

Show comment
Hide comment
@stmichael

stmichael Jul 29, 2013

👍 on 2.3.1

👍 on 2.3.1

@toooni

This comment has been minimized.

Show comment
Hide comment
@toooni

toooni Jul 29, 2013

👍 on 2.3.1 either

toooni commented Jul 29, 2013

👍 on 2.3.1 either

@liuggio

This comment has been minimized.

Show comment
Hide comment
@liuggio

liuggio Aug 5, 2013

Contributor

any news on it?

Contributor

liuggio commented Aug 5, 2013

any news on it?

@csarrazi

This comment has been minimized.

Show comment
Hide comment
@csarrazi

csarrazi Aug 6, 2013

Contributor

Well, my PR is still pending, and still waiting for review, so please don't be too impatient, @liuggio ! ;)

Contributor

csarrazi commented Aug 6, 2013

Well, my PR is still pending, and still waiting for review, so please don't be too impatient, @liuggio ! ;)

@alex-pex

This comment has been minimized.

Show comment
Hide comment
@alex-pex

alex-pex Aug 14, 2013

I didn't look at the PR details, but maybe part of the solution is to merge RequestContext on render(controller(...)) calls ?
It is used by the Router to handle default parameters, like _locale. RequestContext can be extended with listeners (see LocaleListener), and "getParameters" methods returns {"_locale": "en"}

I didn't look at the PR details, but maybe part of the solution is to merge RequestContext on render(controller(...)) calls ?
It is used by the Router to handle default parameters, like _locale. RequestContext can be extended with listeners (see LocaleListener), and "getParameters" methods returns {"_locale": "en"}

@csarrazi

This comment has been minimized.

Show comment
Hide comment
@csarrazi

csarrazi Aug 16, 2013

Contributor

This is exactly what I did in my PR. The _locale parameter now behaves exactly the same way as the _format parameter does.

Contributor

csarrazi commented Aug 16, 2013

This is exactly what I did in my PR. The _locale parameter now behaves exactly the same way as the _format parameter does.

fabpot added a commit to fabpot/symfony that referenced this issue Aug 22, 2013

Fixed issue #6932 - Inconsistent locale handling in subrequests
The fix consists in passing the locale in the controller reference,
inside the routablefragment renderer.

fabpot added a commit that referenced this issue Aug 23, 2013

merged branch fabpot/fragment-locale (PR #8822)
This PR was merged into the 2.2 branch.

Discussion
----------

fixed locale management in sub-requests

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6932, #5532, #8604
| License       | MIT
| Doc PR        | n/a

Same fix as #8821 but for `_locale`, and a rebase of #8604.

Commits
-------

7117328 [HttpKernel] added a comment to warn about possible inconsistencies
c4636e1 added a functional test for locale handling in sub-requests
05fdb12 Fixed issue #6932 - Inconsistent locale handling in subrequests
b3c3159 fixed locale of sub-requests when explicitely set by the developer (refs #8821)

fabpot added a commit that referenced this issue Aug 23, 2013

merged branch fabpot/format-forward (PR #8829)
This PR was merged into the 2.2 branch.

Discussion
----------

fixed request format when forwarding a request

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6932
| License       | MIT
| Doc PR        | n/a

When calling forward() in a controller, the format is not the same as for the master request.

Commits
-------

7e87eb1 fixed request format when forwarding a request

@fabpot fabpot closed this Aug 23, 2013

fabpot added a commit that referenced this issue Aug 23, 2013

Merge branch '2.2' into 2.3
* 2.2:
  fixed request format when forwarding a request
  [HttpKernel] added a comment to warn about possible inconsistencies
  added a functional test for locale handling in sub-requests
  Fixed issue #6932 - Inconsistent locale handling in subrequests
  fixed locale of sub-requests when explicitely set by the developer (refs #8821)

Conflicts:
	src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php

fabpot added a commit that referenced this issue Aug 23, 2013

Merge branch '2.3'
* 2.3:
  fixed request format when forwarding a request
  [HttpKernel] added a comment to warn about possible inconsistencies
  added a functional test for locale handling in sub-requests
  Fixed issue #6932 - Inconsistent locale handling in subrequests
  fixed locale of sub-requests when explicitely set by the developer (refs #8821)

Conflicts:
	src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php
	src/Symfony/Component/HttpKernel/Fragment/RoutableFragmentRenderer.php
	src/Symfony/Component/HttpKernel/Tests/Fragment/RoutableFragmentRendererTest.php
@jaymecd

This comment has been minimized.

Show comment
Hide comment
@jaymecd

jaymecd Aug 23, 2013

Superb! Merci

jaymecd commented Aug 23, 2013

Superb! Merci

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