Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 74f96bf

Browse filesBrowse files
fabpotfabpot
committed
merged branch fabpot/contagious-services (PR symfony#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 | symfony#5300, symfony#6756 | License | MIT | Doc PR | symfony/symfony-docs#2343 Todo: - [x] update documentation - [x] find a better name than contagious (synchronized)? refs symfony#6932, refs symfony#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
2 parents ddd30d0 + 17269e1 commit 74f96bf
Copy full SHA for 74f96bf

38 files changed

+561
-110
lines changed

‎src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php
+1-9Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,8 @@ protected function getFragmentHandler($return)
4949
$strategy->expects($this->once())->method('getName')->will($this->returnValue('inline'));
5050
$strategy->expects($this->once())->method('render')->will($return);
5151

52-
// simulate a master request
53-
$event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')->disableOriginalConstructor()->getMock();
54-
$event
55-
->expects($this->once())
56-
->method('getRequest')
57-
->will($this->returnValue(Request::create('/')))
58-
;
59-
6052
$renderer = new FragmentHandler(array($strategy));
61-
$renderer->onKernelRequest($event);
53+
$renderer->setRequest(Request::create('/'));
6254

6355
return $renderer;
6456
}

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/fragment_renderer.xml

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Resources/config/fragment_renderer.xml
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,9 @@
1414

1515
<services>
1616
<service id="fragment.handler" class="%fragment.handler.class%">
17-
<tag name="kernel.event_subscriber" />
1817
<argument type="collection" />
1918
<argument>%kernel.debug%</argument>
19+
<call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
2020
</service>
2121

2222
<service id="fragment.renderer.inline" class="%fragment.renderer.inline.class%">

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@
9494
<argument type="service" id="router" />
9595
<argument type="service" id="router.request_context" on-invalid="ignore" />
9696
<argument type="service" id="logger" on-invalid="ignore" />
97+
<call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
9798
</service>
9899
</services>
99100
</container>

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040
This service definition only defines the scope of the request.
4141
It is used to check references scope.
4242
-->
43-
<service id="request" scope="request" synthetic="true" />
43+
<service id="request" scope="request" synthetic="true" synchronized="true" />
4444

4545
<service id="service_container" synthetic="true" />
4646

‎src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
<tag name="kernel.event_subscriber" />
3939
<argument>%kernel.default_locale%</argument>
4040
<argument type="service" id="router" on-invalid="ignore" />
41+
<call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>
4142
</service>
4243
</services>
4344
</container>

‎src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/SubRequestController.php

Copy file name to clipboard
+66Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller;
13+
14+
use Symfony\Component\HttpFoundation\Request;
15+
use Symfony\Component\HttpFoundation\Response;
16+
use Symfony\Component\DependencyInjection\ContainerAware;
17+
use Symfony\Component\HttpKernel\Controller\ControllerReference;
18+
19+
class SubRequestController extends ContainerAware
20+
{
21+
public function indexAction()
22+
{
23+
$handler = $this->container->get('fragment.handler');
24+
25+
$errorUrl = $this->generateUrl('subrequest_fragment_error', array('_locale' => 'fr', '_format' => 'json'));
26+
$altUrl = $this->generateUrl('subrequest_fragment', array('_locale' => 'fr', '_format' => 'json'));
27+
28+
// simulates a failure during the rendering of a fragment...
29+
// should render fr/json
30+
$content = $handler->render($errorUrl, 'inline', array('alt' => $altUrl));
31+
32+
// ...to check that the FragmentListener still references the right Request
33+
// when rendering another fragment after the error occured
34+
// should render en/html instead of fr/json
35+
$content .= $handler->render(new ControllerReference('TestBundle:SubRequest:fragment'));
36+
37+
// forces the LocaleListener to set fr for the locale...
38+
// should render fr/json
39+
$content .= $handler->render($altUrl);
40+
41+
// ...and check that after the rendering, the original Request is back
42+
// and en is used as a locale
43+
// should use en/html instead of fr/json
44+
$content .= '--'.$this->generateUrl('subrequest_fragment');
45+
46+
// The RouterListener is also tested as if it does not keep the right
47+
// Request in the context, a 301 would be generated
48+
49+
return new Response($content);
50+
}
51+
52+
public function fragmentAction(Request $request)
53+
{
54+
return new Response('--'.$request->getLocale().'/'.$request->getRequestFormat());
55+
}
56+
57+
public function fragmentErrorAction()
58+
{
59+
throw new \RuntimeException('error');
60+
}
61+
62+
protected function generateUrl($name, $arguments = array())
63+
{
64+
return $this->container->get('router')->generate($name, $arguments);
65+
}
66+
}

‎src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Resources/config/routing.yml

Copy file name to clipboardExpand all lines: src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Resources/config/routing.yml
+15Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,18 @@ session_showflash:
2121
profiler:
2222
path: /profiler
2323
defaults: { _controller: TestBundle:Profiler:index }
24+
25+
subrequest_index:
26+
path: /subrequest/{_locale}.{_format}
27+
defaults: { _controller: TestBundle:SubRequest:index, _format: "html" }
28+
schemes: [https]
29+
30+
subrequest_fragment_error:
31+
path: /subrequest/fragment/error/{_locale}.{_format}
32+
defaults: { _controller: TestBundle:SubRequest:fragmentError, _format: "html" }
33+
schemes: [http]
34+
35+
subrequest_fragment:
36+
path: /subrequest/fragment/{_locale}.{_format}
37+
defaults: { _controller: TestBundle:SubRequest:fragment, _format: "html" }
38+
schemes: [http]

‎src/Symfony/Bundle/FrameworkBundle/Tests/Functional/SubRequestsTest.php

Copy file name to clipboard
+26Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bundle\FrameworkBundle\Tests\Functional;
13+
14+
/**
15+
* @group functional
16+
*/
17+
class SubRequestsTest extends WebTestCase
18+
{
19+
public function testStateAfterSubRequest()
20+
{
21+
$client = $this->createClient(array('test_case' => 'Session', 'root_config' => 'config.yml'));
22+
$client->request('GET', 'https://localhost/subrequest/en');
23+
24+
$this->assertEquals('--fr/json--en/html--fr/json--http://localhost/subrequest/fragment/en', $client->getResponse()->getContent());
25+
}
26+
}

‎src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Compiler/ResolveInvalidReferencesPass.php
+1-3Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,7 @@ private function processArguments(array $arguments, $inMethodCall = false)
8888
$exists = $this->container->has($id);
8989

9090
// resolve invalid behavior
91-
if ($exists && ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE !== $invalidBehavior) {
92-
$arguments[$k] = new Reference($id, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, $argument->isStrict());
93-
} elseif (!$exists && ContainerInterface::NULL_ON_INVALID_REFERENCE === $invalidBehavior) {
91+
if (!$exists && ContainerInterface::NULL_ON_INVALID_REFERENCE === $invalidBehavior) {
9492
$arguments[$k] = null;
9593
} elseif (!$exists && ContainerInterface::IGNORE_ON_INVALID_REFERENCE === $invalidBehavior) {
9694
if ($inMethodCall) {

‎src/Symfony/Component/DependencyInjection/Container.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/Container.php
+18-6Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\DependencyInjection;
1313

14+
use Symfony\Component\DependencyInjection\Exception\InactiveScopeException;
1415
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
1516
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
1617
use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
@@ -206,6 +207,10 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
206207
}
207208

208209
$this->services[$id] = $service;
210+
211+
if (method_exists($this, $method = 'synchronize'.strtr($id, array('_' => '', '.' => '_')).'Service')) {
212+
$this->$method();
213+
}
209214
}
210215

211216
/**
@@ -221,7 +226,7 @@ public function has($id)
221226
{
222227
$id = strtolower($id);
223228

224-
return isset($this->services[$id]) || method_exists($this, 'get'.strtr($id, array('_' => '', '.' => '_')).'Service');
229+
return array_key_exists($id, $this->services) || method_exists($this, 'get'.strtr($id, array('_' => '', '.' => '_')).'Service');
225230
}
226231

227232
/**
@@ -247,7 +252,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
247252
{
248253
$id = strtolower($id);
249254

250-
if (isset($this->services[$id])) {
255+
if (array_key_exists($id, $this->services)) {
251256
return $this->services[$id];
252257
}
253258

@@ -263,10 +268,14 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
263268
} catch (\Exception $e) {
264269
unset($this->loading[$id]);
265270

266-
if (isset($this->services[$id])) {
271+
if (array_key_exists($id, $this->services)) {
267272
unset($this->services[$id]);
268273
}
269274

275+
if ($e instanceof InactiveScopeException && self::EXCEPTION_ON_INVALID_REFERENCE !== $invalidBehavior) {
276+
return null;
277+
}
278+
270279
throw $e;
271280
}
272281

@@ -289,7 +298,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
289298
*/
290299
public function initialized($id)
291300
{
292-
return isset($this->services[strtolower($id)]);
301+
return array_key_exists(strtolower($id), $this->services);
293302
}
294303

295304
/**
@@ -393,8 +402,11 @@ public function leaveScope($name)
393402
$services = $this->scopeStacks[$name]->pop();
394403
$this->scopedServices += $services;
395404

396-
array_unshift($services, $this->services);
397-
$this->services = call_user_func_array('array_merge', $services);
405+
foreach ($services as $array) {
406+
foreach ($array as $id => $service) {
407+
$this->set($id, $service, $name);
408+
}
409+
}
398410
}
399411
}
400412

‎src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/DependencyInjection/ContainerBuilder.php
+60-14Lines changed: 60 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
4646
*/
4747
private $definitions = array();
4848

49+
/**
50+
* @var Definition[]
51+
*/
52+
private $obsoleteDefinitions = array();
53+
4954
/**
5055
* @var Alias[]
5156
*/
@@ -351,14 +356,28 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
351356

352357
if ($this->isFrozen()) {
353358
// setting a synthetic service on a frozen container is alright
354-
if (!isset($this->definitions[$id]) || !$this->definitions[$id]->isSynthetic()) {
359+
if (
360+
(!isset($this->definitions[$id]) && !isset($this->obsoleteDefinitions[$id]))
361+
||
362+
(isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())
363+
||
364+
(isset($this->obsoleteDefinitions[$id]) && !$this->obsoleteDefinitions[$id]->isSynthetic())
365+
) {
355366
throw new BadMethodCallException(sprintf('Setting service "%s" on a frozen container is not allowed.', $id));
356367
}
357368
}
358369

370+
if (isset($this->definitions[$id])) {
371+
$this->obsoleteDefinitions[$id] = $this->definitions[$id];
372+
}
373+
359374
unset($this->definitions[$id], $this->aliases[$id]);
360375

361376
parent::set($id, $service, $scope);
377+
378+
if (isset($this->obsoleteDefinitions[$id]) && $this->obsoleteDefinitions[$id]->isSynchronized()) {
379+
$this->synchronize($id);
380+
}
362381
}
363382

364383
/**
@@ -885,19 +904,7 @@ private function createService(Definition $definition, $id)
885904
}
886905

887906
foreach ($definition->getMethodCalls() as $call) {
888-
$services = self::getServiceConditionals($call[1]);
889-
890-
$ok = true;
891-
foreach ($services as $s) {
892-
if (!$this->has($s)) {
893-
$ok = false;
894-
break;
895-
}
896-
}
897-
898-
if ($ok) {
899-
call_user_func_array(array($service, $call[0]), $this->resolveServices($parameterBag->resolveValue($call[1])));
900-
}
907+
$this->callMethod($service, $call);
901908
}
902909

903910
$properties = $this->resolveServices($parameterBag->resolveValue($definition->getProperties()));
@@ -999,4 +1006,43 @@ public static function getServiceConditionals($value)
9991006

10001007
return $services;
10011008
}
1009+
1010+
/**
1011+
* Synchronizes a service change.
1012+
*
1013+
* This method updates all services that depend on the given
1014+
* service by calling all methods referencing it.
1015+
*
1016+
* @param string $id A service id
1017+
*/
1018+
private function synchronize($id)
1019+
{
1020+
foreach ($this->definitions as $definitionId => $definition) {
1021+
// only check initialized services
1022+
if (!$this->initialized($definitionId)) {
1023+
continue;
1024+
}
1025+
1026+
foreach ($definition->getMethodCalls() as $call) {
1027+
foreach ($call[1] as $argument) {
1028+
if ($argument instanceof Reference && $id == (string) $argument) {
1029+
$this->callMethod($this->get($definitionId), $call);
1030+
}
1031+
}
1032+
}
1033+
}
1034+
}
1035+
1036+
private function callMethod($service, $call)
1037+
{
1038+
$services = self::getServiceConditionals($call[1]);
1039+
1040+
foreach ($services as $s) {
1041+
if (!$this->has($s)) {
1042+
return;
1043+
}
1044+
}
1045+
1046+
call_user_func_array(array($service, $call[0]), $this->resolveServices($this->getParameterBag()->resolveValue($call[1])));
1047+
}
10021048
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.