Permalink
Browse files

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
  • Loading branch information...
2 parents bdc1ffe + 5d0f0eb commit 82a3e6b78aea8295be1bcf1667f5433411248a04 @fabpot fabpot committed Mar 23, 2013
@@ -58,11 +58,13 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
try {
$response = parent::handle($request, $type, $catch);
} catch (\Exception $e) {
+ $this->container->set('request', null, 'request');
$this->container->leaveScope('request');
throw $e;
}
+ $this->container->set('request', null, 'request');
$this->container->leaveScope('request');
return $response;
@@ -12,7 +12,6 @@
namespace Symfony\Component\HttpKernel\EventListener;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
-use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\RequestContextAwareInterface;
@@ -27,49 +26,41 @@ class LocaleListener implements EventSubscriberInterface
{
private $router;
private $defaultLocale;
- private $locales = array();
public function __construct($defaultLocale = 'en', RequestContextAwareInterface $router = null)
{
$this->defaultLocale = $defaultLocale;
$this->router = $router;
}
- public function onKernelResponse(FilterResponseEvent $event)
+ public function setRequest(Request $request = null)
{
- array_shift($this->locales);
+ if (null === $request) {
+ return;
+ }
- // setting back the locale to the previous value
- $locale = isset($this->locales[0]) ? $this->locales[0] : $this->defaultLocale;
- $request = $event->getRequest();
- $this->setLocale($request, $locale);
+ if ($locale = $request->attributes->get('_locale')) {
+ $request->setLocale($locale);
+ }
+
+ if (null !== $this->router) {
+ $this->router->getContext()->setParameter('_locale', $request->getLocale());
+ }
}
public function onKernelRequest(GetResponseEvent $event)
{
$request = $event->getRequest();
-
$request->setDefaultLocale($this->defaultLocale);
- $this->setLocale($request, $request->attributes->get('_locale', $this->defaultLocale));
- array_unshift($this->locales, $request->getLocale());
+ $this->setRequest($request);
}
public static function getSubscribedEvents()
{
return array(
// must be registered after the Router to have access to the _locale
KernelEvents::REQUEST => array(array('onKernelRequest', 16)),
- KernelEvents::RESPONSE => 'onKernelResponse',
);
}
-
- private function setLocale(Request $request, $locale)
- {
- $request->setLocale($locale);
-
- if (null !== $this->router) {
- $this->router->getContext()->setParameter('_locale', $request->getLocale());
- }
- }
}
@@ -23,6 +23,7 @@
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\RequestContextAwareInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\Request;
/**
* Initializes the context from the request and sets request attributes based on a matching route.
@@ -59,12 +60,31 @@ public function __construct($matcher, RequestContext $context = null, LoggerInte
$this->logger = $logger;
}
+ /**
+ * Sets the current Request.
+ *
+ * The application should call this method whenever the Request
+ * object changes (entering a Request scope for instance, but
+ * also when leaving a Request scope -- especially when they are
+ * nested).
+ *
+ * @param Request|null $request A Request instance
+ */
+ public function setRequest(Request $request = null)
+ {
+ if (null !== $request) {
+ $this->context->fromRequest($request);
+ }
+ }
+
public function onKernelRequest(GetResponseEvent $event)
{
$request = $event->getRequest();
// initialize the context that is also used by the generator (assuming matcher and generator share the same context instance)
- $this->context->fromRequest($request);
+ // we call setRequest even if most of the time, it has already been done to keep compatibility
+ // with frameworks which do not use the Symfony service container
+ $this->setRequest($request);
if ($request->attributes->has('_controller')) {
// routing is already done
@@ -15,10 +15,6 @@
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\StreamedResponse;
use Symfony\Component\HttpKernel\Controller\ControllerReference;
-use Symfony\Component\HttpKernel\KernelEvents;
-use Symfony\Component\HttpKernel\Event\GetResponseEvent;
-use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
-use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
* Renders a URI that represents a resource fragment.
@@ -30,11 +26,11 @@
*
* @see FragmentRendererInterface
*/
-class FragmentHandler implements EventSubscriberInterface
+class FragmentHandler
{
private $debug;
private $renderers;
- private $requests;
+ private $request;
/**
* Constructor.
@@ -49,7 +45,6 @@ public function __construct(array $renderers = array(), $debug = false)
$this->addRenderer($renderer);
}
$this->debug = $debug;
- $this->requests = array();
}
/**
@@ -63,23 +58,13 @@ public function addRenderer(FragmentRendererInterface $renderer)
}
/**
- * Stores the Request object.
+ * Sets the current Request.
*
- * @param GetResponseEvent $event A GetResponseEvent instance
+ * @param Request $request The current Request
*/
- public function onKernelRequest(GetResponseEvent $event)
+ public function setRequest(Request $request = null)
{
- array_unshift($this->requests, $event->getRequest());
- }
-
- /**
- * Removes the most recent Request object.
- *
- * @param FilterResponseEvent $event A FilterResponseEvent instance
- */
- public function onKernelResponse(FilterResponseEvent $event)
- {
- array_shift($this->requests);
+ $this->request = $request;
}
/**
@@ -108,7 +93,11 @@ public function render($uri, $renderer = 'inline', array $options = array())
throw new \InvalidArgumentException(sprintf('The "%s" renderer does not exist.', $renderer));
}
- return $this->deliver($this->renderers[$renderer]->render($uri, $this->requests[0], $options));
+ if (null === $this->request) {
+ throw new \LogicException('Rendering a fragment can only be done when handling a master Request.');
+ }
+
+ return $this->deliver($this->renderers[$renderer]->render($uri, $this->request, $options));
}
/**
@@ -126,7 +115,7 @@ public function render($uri, $renderer = 'inline', array $options = array())
protected function deliver(Response $response)
{
if (!$response->isSuccessful()) {
- throw new \RuntimeException(sprintf('Error when rendering "%s" (Status code is %s).', $this->requests[0]->getUri(), $response->getStatusCode()));
+ throw new \RuntimeException(sprintf('Error when rendering "%s" (Status code is %s).', $this->request->getUri(), $response->getStatusCode()));
}
if (!$response instanceof StreamedResponse) {
@@ -135,12 +124,4 @@ protected function deliver(Response $response)
$response->sendContent();
}
-
- public static function getSubscribedEvents()
- {
- return array(
- KernelEvents::REQUEST => 'onKernelRequest',
- KernelEvents::RESPONSE => 'onKernelResponse',
- );
- }
}
@@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/
-namespace Symfony\Component\HttpKernel\Tests;
+namespace Symfony\Component\HttpKernel\Tests\DependencyInjection;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel;
@@ -54,10 +54,15 @@ public function testHandle($type)
->with($this->equalTo('request'))
;
$container
- ->expects($this->once())
+ ->expects($this->at(1))
->method('set')
->with($this->equalTo('request'), $this->equalTo($request), $this->equalTo('request'))
;
+ $container
+ ->expects($this->at(2))
+ ->method('set')
+ ->with($this->equalTo('request'), $this->equalTo(null), $this->equalTo('request'))
+ ;
$dispatcher = new EventDispatcher();
$resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');
@@ -101,10 +106,15 @@ public function testHandleRestoresThePreviousRequestOnException($type)
->with($this->equalTo('request'))
;
$container
- ->expects($this->once())
+ ->expects($this->at(1))
->method('set')
->with($this->equalTo('request'), $this->equalTo($request), $this->equalTo('request'))
;
+ $container
+ ->expects($this->at(2))
+ ->method('set')
+ ->with($this->equalTo('request'), $this->equalTo(null), $this->equalTo('request'))
+ ;
$dispatcher = new EventDispatcher();
$resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');
@@ -17,13 +17,6 @@
class FragmentHandlerTest extends \PHPUnit_Framework_TestCase
{
- protected function setUp()
- {
- if (!class_exists('Symfony\Component\EventDispatcher\EventDispatcher')) {
- $this->markTestSkipped('The "EventDispatcher" component is not available');
- }
- }
-
/**
* @expectedException \InvalidArgumentException
*/
@@ -81,14 +74,7 @@ protected function getHandler($returnValue, $arguments = array())
$handler = new FragmentHandler();
$handler->addRenderer($renderer);
-
- $event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock();
- $event
- ->expects($this->once())
- ->method('getRequest')
- ->will($this->returnValue(Request::create('/')))
- ;
- $handler->onKernelRequest($event);
+ $handler->setRequest(Request::create('/'));
return $handler;
}

0 comments on commit 82a3e6b

Please sign in to comment.