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 ddd30d0 + 17269e1 commit 74f96bfebf79d018bd20d7de222735a82c7f4caf @fabpot fabpot committed Mar 23, 2013
Showing with 561 additions and 110 deletions.
  1. +1 −9 src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php
  2. +1 −1 src/Symfony/Bundle/FrameworkBundle/Resources/config/fragment_renderer.xml
  3. +1 −0 src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
  4. +1 −1 src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml
  5. +1 −0 src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
  6. +66 −0 ...ony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/SubRequestController.php
  7. +15 −0 src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Resources/config/routing.yml
  8. +26 −0 src/Symfony/Bundle/FrameworkBundle/Tests/Functional/SubRequestsTest.php
  9. +1 −3 src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php
  10. +18 −6 src/Symfony/Component/DependencyInjection/Container.php
  11. +60 −14 src/Symfony/Component/DependencyInjection/ContainerBuilder.php
  12. +30 −0 src/Symfony/Component/DependencyInjection/Definition.php
  13. +57 −2 src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
  14. +9 −0 src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php
  15. +8 −0 src/Symfony/Component/DependencyInjection/Dumper/YamlDumper.php
  16. +1 −1 src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
  17. +4 −0 src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
  18. +1 −0 src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
  19. +47 −0 src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
  20. +16 −1 src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
  21. +13 −1 src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php
  22. +9 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php
  23. +3 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/graphviz/services9.dot
  24. +5 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php
  25. +40 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
  26. +40 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php
  27. +1 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services6.xml
  28. +6 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services9.xml
  29. +4 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services6.yml
  30. +9 −0 src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services9.yml
  31. +3 −0 src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php
  32. +3 −0 src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
  33. +2 −0 src/Symfony/Component/HttpKernel/DependencyInjection/ContainerAwareHttpKernel.php
  34. +12 −21 src/Symfony/Component/HttpKernel/EventListener/LocaleListener.php
  35. +21 −1 src/Symfony/Component/HttpKernel/EventListener/RouterListener.php
  36. +12 −31 src/Symfony/Component/HttpKernel/Fragment/FragmentHandler.php
  37. +13 −3 src/Symfony/Component/HttpKernel/Tests/DependencyInjection/ContainerAwareHttpKernelTest.php
  38. +1 −15 src/Symfony/Component/HttpKernel/Tests/Fragment/FragmentHandlerTest.php
@@ -49,16 +49,8 @@ protected function getFragmentHandler($return)
$strategy->expects($this->once())->method('getName')->will($this->returnValue('inline'));
$strategy->expects($this->once())->method('render')->will($return);
- // simulate a master request
- $event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock();
- $event
- ->expects($this->once())
- ->method('getRequest')
- ->will($this->returnValue(Request::create('/')))
- ;
-
$renderer = new FragmentHandler(array($strategy));
- $renderer->onKernelRequest($event);
+ $renderer->setRequest(Request::create('/'));
return $renderer;
}
@@ -14,9 +14,9 @@
<services>
<service id="fragment.handler" class="%fragment.handler.class%">
- <tag name="kernel.event_subscriber" />
<argument type="collection" />
<argument>%kernel.debug%</argument>
+ <call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
</service>
<service id="fragment.renderer.inline" class="%fragment.renderer.inline.class%">
@@ -94,6 +94,7 @@
<argument type="service" id="router" />
<argument type="service" id="router.request_context" on-invalid="ignore" />
<argument type="service" id="logger" on-invalid="ignore" />
+ <call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
</service>
</services>
</container>
@@ -40,7 +40,7 @@
This service definition only defines the scope of the request.
It is used to check references scope.
-->
- <service id="request" scope="request" synthetic="true" />
+ <service id="request" scope="request" synthetic="true" synchronized="true" />
<service id="service_container" synthetic="true" />
@@ -38,6 +38,7 @@
<tag name="kernel.event_subscriber" />
<argument>%kernel.default_locale%</argument>
<argument type="service" id="router" on-invalid="ignore" />
+ <call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
</service>
</services>
</container>
@@ -0,0 +1,66 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\DependencyInjection\ContainerAware;
+use Symfony\Component\HttpKernel\Controller\ControllerReference;
+
+class SubRequestController extends ContainerAware
+{
+ public function indexAction()
+ {
+ $handler = $this->container->get('fragment.handler');
+
+ $errorUrl = $this->generateUrl('subrequest_fragment_error', array('_locale' => 'fr', '_format' => 'json'));
+ $altUrl = $this->generateUrl('subrequest_fragment', array('_locale' => 'fr', '_format' => 'json'));
+
+ // simulates a failure during the rendering of a fragment...
+ // should render fr/json
+ $content = $handler->render($errorUrl, 'inline', array('alt' => $altUrl));
+
+ // ...to check that the FragmentListener still references the right Request
+ // when rendering another fragment after the error occured
+ // should render en/html instead of fr/json
+ $content .= $handler->render(new ControllerReference('TestBundle:SubRequest:fragment'));
+
+ // forces the LocaleListener to set fr for the locale...
+ // should render fr/json
+ $content .= $handler->render($altUrl);
+
+ // ...and check that after the rendering, the original Request is back
+ // and en is used as a locale
+ // should use en/html instead of fr/json
+ $content .= '--'.$this->generateUrl('subrequest_fragment');
+
+ // The RouterListener is also tested as if it does not keep the right
+ // Request in the context, a 301 would be generated
+
+ return new Response($content);
+ }
+
+ public function fragmentAction(Request $request)
+ {
+ return new Response('--'.$request->getLocale().'/'.$request->getRequestFormat());
+ }
+
+ public function fragmentErrorAction()
+ {
+ throw new \RuntimeException('error');
+ }
+
+ protected function generateUrl($name, $arguments = array())
+ {
+ return $this->container->get('router')->generate($name, $arguments);
+ }
+}
@@ -21,3 +21,18 @@ session_showflash:
profiler:
path: /profiler
defaults: { _controller: TestBundle:Profiler:index }
+
+subrequest_index:
+ path: /subrequest/{_locale}.{_format}
+ defaults: { _controller: TestBundle:SubRequest:index, _format: "html" }
+ schemes: [https]
+
+subrequest_fragment_error:
+ path: /subrequest/fragment/error/{_locale}.{_format}
+ defaults: { _controller: TestBundle:SubRequest:fragmentError, _format: "html" }
+ schemes: [http]
+
+subrequest_fragment:
+ path: /subrequest/fragment/{_locale}.{_format}
+ defaults: { _controller: TestBundle:SubRequest:fragment, _format: "html" }
+ schemes: [http]
@@ -0,0 +1,26 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\Tests\Functional;
+
+/**
+ * @group functional
+ */
+class SubRequestsTest extends WebTestCase
+{
+ public function testStateAfterSubRequest()
+ {
+ $client = $this->createClient(array('test_case' => 'Session', 'root_config' => 'config.yml'));
+ $client->request('GET', 'https://localhost/subrequest/en');
+
+ $this->assertEquals('--fr/json--en/html--fr/json--http://localhost/subrequest/fragment/en', $client->getResponse()->getContent());
+ }
+}
@@ -88,9 +88,7 @@ private function processArguments(array $arguments, $inMethodCall = false)
$exists = $this->container->has($id);
// resolve invalid behavior
- if ($exists && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $invalidBehavior) {
- $arguments[$k] = new Reference($id, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, $argument->isStrict());
- } elseif (!$exists && ContainerInterface::NULL_ON_INVALID_REFERENCE === $invalidBehavior) {
+ if (!$exists && ContainerInterface::NULL_ON_INVALID_REFERENCE === $invalidBehavior) {
$arguments[$k] = null;
} elseif (!$exists && ContainerInterface::IGNORE_ON_INVALID_REFERENCE === $invalidBehavior) {
if ($inMethodCall) {
@@ -11,6 +11,7 @@
namespace Symfony\Component\DependencyInjection;
+use Symfony\Component\DependencyInjection\Exception\InactiveScopeException;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
@@ -206,6 +207,10 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
}
$this->services[$id] = $service;
+
+ if (method_exists($this, $method = 'synchronize'.strtr($id, array('_' => '', '.' => '_')).'Service')) {
+ $this->$method();
+ }
}
/**
@@ -221,7 +226,7 @@ public function has($id)
{
$id = strtolower($id);
- return isset($this->services[$id]) || method_exists($this, 'get'.strtr($id, array('_' => '', '.' => '_')).'Service');
+ return array_key_exists($id, $this->services) || method_exists($this, 'get'.strtr($id, array('_' => '', '.' => '_')).'Service');
}
/**
@@ -247,7 +252,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
{
$id = strtolower($id);
- if (isset($this->services[$id])) {
+ if (array_key_exists($id, $this->services)) {
return $this->services[$id];
}
@@ -263,10 +268,14 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
} catch (\Exception $e) {
unset($this->loading[$id]);
- if (isset($this->services[$id])) {
+ if (array_key_exists($id, $this->services)) {
unset($this->services[$id]);
}
+ if ($e instanceof InactiveScopeException && self::EXCEPTION_ON_INVALID_REFERENCE !== $invalidBehavior) {
+ return null;
+ }
+
throw $e;
}
@@ -289,7 +298,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
*/
public function initialized($id)
{
- return isset($this->services[strtolower($id)]);
+ return array_key_exists(strtolower($id), $this->services);
}
/**
@@ -393,8 +402,11 @@ public function leaveScope($name)
$services = $this->scopeStacks[$name]->pop();
$this->scopedServices += $services;
- array_unshift($services, $this->services);
- $this->services = call_user_func_array('array_merge', $services);
+ foreach ($services as $array) {
+ foreach ($array as $id => $service) {
+ $this->set($id, $service, $name);
+ }
+ }
}
}
@@ -46,6 +46,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
*/
private $definitions = array();
+ /**
+ * @var Definition[]
+ */
+ private $obsoleteDefinitions = array();
+
/**
* @var Alias[]
*/
@@ -351,14 +356,28 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
if ($this->isFrozen()) {
// setting a synthetic service on a frozen container is alright
- if (!isset($this->definitions[$id]) || !$this->definitions[$id]->isSynthetic()) {
+ if (
+ (!isset($this->definitions[$id]) && !isset($this->obsoleteDefinitions[$id]))
+ ||
+ (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())
+ ||
+ (isset($this->obsoleteDefinitions[$id]) && !$this->obsoleteDefinitions[$id]->isSynthetic())
+ ) {
throw new BadMethodCallException(sprintf('Setting service "%s" on a frozen container is not allowed.', $id));
}
}
+ if (isset($this->definitions[$id])) {
+ $this->obsoleteDefinitions[$id] = $this->definitions[$id];
+ }
+
unset($this->definitions[$id], $this->aliases[$id]);
parent::set($id, $service, $scope);
+
+ if (isset($this->obsoleteDefinitions[$id]) && $this->obsoleteDefinitions[$id]->isSynchronized()) {
+ $this->synchronize($id);
+ }
}
/**
@@ -885,19 +904,7 @@ private function createService(Definition $definition, $id)
}
foreach ($definition->getMethodCalls() as $call) {
- $services = self::getServiceConditionals($call[1]);
-
- $ok = true;
- foreach ($services as $s) {
- if (!$this->has($s)) {
- $ok = false;
- break;
- }
- }
-
- if ($ok) {
- call_user_func_array(array($service, $call[0]), $this->resolveServices($parameterBag->resolveValue($call[1])));
- }
+ $this->callMethod($service, $call);
}
$properties = $this->resolveServices($parameterBag->resolveValue($definition->getProperties()));
@@ -999,4 +1006,43 @@ public static function getServiceConditionals($value)
return $services;
}
+
+ /**
+ * Synchronizes a service change.
+ *
+ * This method updates all services that depend on the given
+ * service by calling all methods referencing it.
+ *
+ * @param string $id A service id
+ */
+ private function synchronize($id)
+ {
+ foreach ($this->definitions as $definitionId => $definition) {
+ // only check initialized services
+ if (!$this->initialized($definitionId)) {
+ continue;
+ }
+
+ foreach ($definition->getMethodCalls() as $call) {
+ foreach ($call[1] as $argument) {
+ if ($argument instanceof Reference && $id == (string) $argument) {
+ $this->callMethod($this->get($definitionId), $call);
+ }
+ }
+ }
+ }
+ }
+
+ private function callMethod($service, $call)
+ {
+ $services = self::getServiceConditionals($call[1]);
+
+ foreach ($services as $s) {
+ if (!$this->has($s)) {
+ return;
+ }
+ }
+
+ call_user_func_array(array($service, $call[0]), $this->resolveServices($this->getParameterBag()->resolveValue($call[1])));
+ }
}
Oops, something went wrong.

0 comments on commit 74f96bf

Please sign in to comment.