[2.3] [WIP] Synchronized services... #7007
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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]) && !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) { | ||
| $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) | ||
|
There was a problem hiding this comment. This method feels like it could get quite slow. Once you have a decent number of services initialized it's going to result in a lot of iterations, even if only a scant few of them result in a method call. Could we add a tracking index of some kind to keep track of what methods would need to get called on what services? There was a problem hiding this comment. Hm, or maybe the PhpDumper is handling that? I don't understand the dumper code well enough to understand if it is. If the dumped code is more optimized than this and would not iterate over all services every time a synchronized service changed, that's good enough, I think. If it would still use this method, then this method should be optimized. There was a problem hiding this comment. That's the case, this method is not called when using a dumped container (a dumper container extends Container and not ContainerBuilder). Here is an example of a dumped code (see the unit tests below): protected function synchronizeRequestService()
{
if ($this->initialized('depends_on_request')) {
$this->get('depends_on_request')->setRequest($this->get('request', ContainerInterface::NULL_ON_INVALID_REFERENCE));
}
}And for services without the |
||
| { | ||
| 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) | ||
|
There was a problem hiding this comment. The PHPDoc is missing here There was a problem hiding this comment. I don't think private methods require a PHPDoc block in the symfony2 coding standards, no? There was a problem hiding this comment. IMO all methods should have a PHPDoc block, regardless of their scope. Someone is going to read this code and wonder what those variables are supposed to be. There was a problem hiding this comment. @Crell I agree whole-heartedly, but I didn't think private methods needed them (think I'm wrong though according to the standards mentioned here -- http://symfony.com/doc/current/contributing/code/standards.html#documentation) |
||
| { | ||
| $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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this break the retrieval of the service in the CLI (and so the retrieval of Twig) ? AFAIK,
on-invalid="null"is about missing services, not about scope issues as$container->has('request')is always true even ifrequestis only available in the request scope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6140 could be useful to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabpot IMO,
hasshould be fixed to returnfalseif the service is scoped and the scope is inactive. Currently, code wanting to check that it can get the service has to know that it is scoped to the request: https://github.com/symfony/symfony/blob/2.2/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php#L70There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, 29e7b8a fixes the issue quite nicely (part of this commit is actually another fix that I've submitted as another PR -- #7450).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabpot what about
oninvalid="ignore"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would not work as it uses
hasto determine of the service is available or not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the issue is that YAML does not allow setting
on-invalid="null": https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L288There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a problem when using the dumper as it just means that when you use ignore, there is one more check. If not, I've slightly changed my patch to take care of that.