8000 merged branch fabpot/kernel-refactor (PR #6459) · symfony/symfony@b58e8ce · GitHub
[go: up one dir, main page]
Skip to content

Commit b58e8ce

Browse files
fabpotfabpot
committed
merged branch fabpot/kernel-refactor (PR #6459)
This PR was merged into the master branch. Commits ------- 76fefe3 updated CHANGELOG and UPGRADE files f7da1f0 added some unit tests (and fixed some bugs) f17f586 moved the container aware HTTP kernel to the HttpKernel component 2eea768 moved the deprecation logic calls outside the new HttpContentRenderer class bd102c5 made the content renderer work even when ESI is disabled or when no templating engine is available (the latter being mostly useful when testing) a8ea4e4 [FrameworkBundle] deprecated HttpKernel::forward() (it is only used once now and not part of any interface anyway) 1240690 [HttpKernel] made the strategy a regular parameter in HttpContentRenderer::render() adc067e [FrameworkBundle] made some services private 1f1392d [HttpKernel] simplified and enhanced code managing the hinclude strategy 403bb06 [HttpKernel] added missing phpdoc and tweaked existing ones 892f00f [HttpKernel] added a URL signer mechanism for hincludes a0c49c3 [TwigBridge] added a render_* function to ease usage of custom rendering strategies 9aaceb1 moved the logic from HttpKernel in FrameworkBundle to the HttpKernel component Discussion ---------- [WIP] Kernel refactor Currently, the handling of sub-requests (including ESI and hinclude) is mostly done in FrameworkBundle. It makes these important features harder to implement for people using only HttpKernel (like Drupal and Silex for instance). This PR moves the code to HttpKernel instead. The code has also been refactored to allow easier integration of other rendering strategies (refs #6108). The internal route has been re-introduced but it can only be used for trusted IPs (so for the internal rendering which is managed by Symfony itself, or by a trusted reverse proxy like Varnish for ESI handling). For the hinclude strategy, when using a controller, the URL is automatically signed (see #6463). The usage of a listener instead of a controller to handle internal sub-requests speeds up things quite a lot as it saves one sub-request handling. In Symfony 2.0 and 2.1, the handling of a sub-request actually creates two sub-requests. Rendering a sub-request from a controller can be done with the following code: ```jinja {# default strategy #} {{ render(path("partial")) }} {{ render(controller("SomeBundle:Controller:partial")) }} {# ESI strategy #} {{ render(path("partial"), { strategy: 'esi' }) }} {{ render(controller("SomeBundle:Controller:partial"), { strategy: 'esi' }) }} {# hinclude strategy #} {{ render(path("default1"), { strategy: 'hinclude' }) }} ``` The second commit allows to simplify the calls a little bit thanks to some nice syntactic sugar: ```jinja {# default strategy #} {{ render(path("partial")) }} {{ render(controller("SomeBundle:Controller:partial")) }} {# ESI strategy #} {{ render_esi(path("partial")) }} {{ render_esi(controller("SomeBundle:Controller:partial")) }} {# hinclude strategy #} {{ render_hinclude(path("default1")) }} ``` --------------------------------------------------------------------------- by fabpot at 2013-01-03T17:58:49Z I've just pushed a new version of the code that actually works in my browser (but I've not yet written any unit tests). I've updated the PR description accordingly. All comments welcome! --------------------------------------------------------------------------- by Koc at 2013-01-03T20:11:43Z what about `render(controller="SomeBundle:Controller:partial", strategy="esi")`? --------------------------------------------------------------------------- by stof at 2013-01-04T09:01:01Z shouldn't we have interfaces for the UriSigner and the HttpContentRenderer ? --------------------------------------------------------------------------- by lsmith77 at 2013-01-04T19:28:09Z btw .. as mentioned in #6213 i think it would make sense to refactor the HttpCache to use a cache layer to allow more flexibility in where to cache the data (including clustering) and better invalidation. as such if you are refactoring HttpKernel .. it might also make sense to explore splitting off HttpCache. --------------------------------------------------------------------------- by fabpot at 2013-01-04T19:30:07Z @lsmith77 This is a totally different topic. This PR is just about moving things from FrameworkBundle to HttpKernel to make them more reusable outside of the full-stack framework. --------------------------------------------------------------------------- by fabpot at 2013-01-05T09:39:52Z I think this PR is almost ready now. I still need to update the docs and add some unit tests. Any other comments on the whole approach? The class names? The `controller` function thingy? The URI signer mechanism? The proxy protection for the internal controller? The proxy to handle internal routes? --------------------------------------------------------------------------- by sstok at 2013-01-05T10:08:25Z Looks good to me :+1: --------------------------------------------------------------------------- by sdboyer at 2013-01-07T18:17:08Z @Crell asked me to weigh in, since i'm one of the Drupal folks who's likely to work most with this. i think i've grokked about 60% of the big picture here, and i'm generally happy with what i see. the assumption that the HInclude strategy makes about working with templates probably isn't one that we'll be able to use (and so, would need to write our own), but that's not a big deal since the whole goal here is to make strategies pluggable. so, yeah. +1. --------------------------------------------------------------------------- by winzou at 2013-01-09T20:21:44Z Just for my information: will this PR be merged for 2.2 version? Thanks. --------------------------------------------------------------------------- by stof at 2013-01-09T20:41:04Z @winzou according to the blog post announcing the beta 1 release, yes. It is explicitly listed as being one of the reason to make it a beta instead of the first RC. --------------------------------------------------------------------------- by winzou at 2013-01-09T20:49:36Z OK thanks, I've totally skipped this blog post. --------------------------------------------------------------------------- by fabpot at 2013-01-10T15:26:15Z I've just added a bunch of unit tests and fix some bugs I found while writing the tests.
2 parents 2aba3aa + 76fefe3 commit b58e8ce

40 files changed

+1825
-255
lines changed

‎UPGRADE-2.2.md

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,9 @@
1414
After:
1515

1616
```
17-
{% render url('post_list', { 'limit': 2 }), { 'alt': 'BlogBundle:Post:error' } %}
17+
{% render controller('BlogBundle:Post:list', { 'limit': 2 }), { 'alt': 'BlogBundle:Post:error' } %}
1818
```
1919

20-
where `post_list` is the route name for the `BlogBundle:Post:list` controller.
21-
2220
### HttpFoundation
2321

2422
* The MongoDbSessionHandler default field names and timestamp type have changed.
@@ -537,7 +535,12 @@
537535
<?php echo $view['actions']->render($view['router']->generate('post_list', array('limit' => 2)), array('alt' => 'BlogBundle:Post:error')) ?>
538536
```
539537

540-
where `post_list` is the route name for the `BlogBundle:Post:list` controller.
538+
where `post_list` is the route name for the `BlogBundle:Post:list`
539+
controller, or if you don't want to create a route:
540+
541+
```
542+
<?php echo $view['actions']->render(new ControllerReference('BlogBundle:Post:list', array('limit' => 2)), array('alt' => 'BlogBundle:Post:error')) ?>
543+
```
541544

542545
#### Configuration
543546

‎src/Symfony/Bridge/Twig/CHANGELOG.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ CHANGELOG
44
2.2.0
55
-----
66

7-
* [BC BREAK] restricted the `render` tag to only accept URIs as reference (the signature changed)
8-
* added a render function to render a request
7+
* added a `controller` function to help generating controller references
8+
* added a `render_esi` and a `render_hinclude` function
9+
* [BC BREAK] restricted the `render` tag to only accept URIs or ControllerReference instances (the signature changed)
10+
* added a `render` function to render a request
911
* The `app` global variable is now injected even when using the twig service directly.
1012
* Added an optional parameter to the `path` and `url` function which allows to generate
1113
relative paths (e.g. "../parent-file") and scheme-relative URLs (e.g. "//example.com/dir/file").

‎src/Symfony/Bridge/Twig/Extension/HttpKernelExtension.php

Lines changed: 22 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -11,83 +11,65 @@
1111

1212
namespace Symfony\Bridge\Twig\Extension;
1313

14-
use Symfony\Component\HttpFoundation\Request;
15-
use Symfony\Component\HttpKernel\KernelEvents;
16-
use Symfony\Component\HttpKernel\HttpKernelInterface;
17-
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
18-
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
14+
use Symfony\Component\HttpKernel\HttpContentRenderer;
15+
use Symfony\Component\HttpKernel\Controller\ControllerReference;
1916

2017
/**
2118
* Provides integration with the HttpKernel component.
2219
*
2320
* @author Fabien Potencier <fabien@symfony.com>
2421
*/
25-
class HttpKernelExtension extends \Twig_Extension implements EventSubscriberInterface
22+
class HttpKernelExtension extends \Twig_Extension
2623
{
27-
private $kernel;
28-
private $request;
24+
private $renderer;
2925

3026
/**
3127
* Constructor.
3228
*
33-
* @param HttpKernelInterface $kernel A HttpKernelInterface install
29+
* @param HttpContentRenderer $kernel A HttpContentRenderer instance
3430
*/
35-
public function __construct(HttpKernelInterface $kernel)
31+
public function __construct(HttpContentRenderer $renderer)
3632
{
37-
$this->kernel = $kernel;
33+
$this->renderer = $renderer;
3834
}
3935

4036
public function getFunctions()
4137
{
4238
return array(
43-
'render' => new \Twig_Function_Method($this, 'render', array('needs_environment' => true, 'is_safe' => array('html'))),
39+
'render' => new \Twig_Function_Method($this, 'render', array('is_safe' => array('html'))),
40+
'render_*' => new \Twig_Function_Method($this, 'renderStrategy', array('is_safe' => array('html'))),
41+
'controller' => new \Twig_Function_Method($this, 'controller'),
4442
);
4543
}
4644

4745< 93C6 /code>
/**
4846
* Renders a URI.
4947
*
50-
* @param \Twig_Environment $twig A \Twig_Environment instance
51-
* @param string $uri The URI to render
48+
* @param string $uri A URI
49+
* @param array $options An array of options
5250
*
5351
* @return string The Response content
5452
*
55-
* @throws \RuntimeException
53+
* @see Symfony\Component\HttpKernel\HttpContentRenderer::render()
5654
*/
57-
public function render(\Twig_Environment $twig, $uri)
55+
public function render($uri, $options = array())
5856
{
59-
if (null !== $this->request) {
60-
$cookies = $this->request->cookies->all();
61-
$server = $this->request->server->all();
62-
} else {
63-
$cookies = array();
64-
$server = array();
65-
}
57+
$options = $this->renderer->fixOptions($options);
6658

67-
$subRequest = Request::create($uri, 'get', array(), $cookies, array(), $server);
68-
if (null !== $this->request && $this->request->getSession()) {
69-
$subRequest->setSession($this->request->getSession());
70-
}
59+
$strategy = isset($options['strategy']) ? $options['strategy'] : 'default';
60+
unset($options['strategy']);
7161

72-
$response = $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST, false);
73-
74-
if (!$response->isSuccessful()) {
75-
throw new \RuntimeException(sprintf('Error when rendering "%s" (Status code is %s).', $subRequest->getUri(), $response->getStatusCode()));
76-
}
77-
78-
return $response->getContent();
62+
return $this->renderer->render($uri, $strategy, $options);
7963
}
8064

81-
public function onKernelRequest(GetResponseEvent $event)
65+
public function renderStrategy($strategy, $uri, $options = array())
8266
{
83-
$this->request = $event->getRequest();
67+
return $this->renderer->render($uri, $strategy, $options);
8468
}
8569

86-
public static function getSubscribedEvents()
70+
public function controller($controller, $attributes = array(), $query = array())
8771
{
88-
return array(
89-
KernelEvents::REQUEST => array('onKernelRequest'),
90-
);
72+
return new ControllerReference($controller, $attributes, $query);
9173
}
9274

9375
public function getName()

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

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,7 @@
1313

1414
use Symfony\Bridge\Twig\Extension\HttpKernelExtension;
1515 10000
use Symfony\Bridge\Twig\Tests\TestCase;
16-
use Symfony\Component\HttpFoundation\Response;
17-
use Symfony\Component\HttpKernel\HttpKernelInterface;
16+
use Symfony\Component\HttpKernel\HttpContentRenderer;
1817

1918
class HttpKernelExtensionTest extends TestCase
2019
{
@@ -31,7 +30,7 @@ protected function setUp()
3130

3231
public function testRenderWithoutMasterRequest()
3332
{
34-
$kernel = $this->getKernel($this->returnValue(new Response('foo')));
33+
$kernel = $this->getHttpContentRenderer($this->returnValue('foo'));
3534

3635
$this->assertEquals('foo', $this->renderTemplate($kernel));
3736
}
@@ -41,7 +40,7 @@ public function testRenderWithoutMasterRequest()
4140
*/
4241
public function testRenderWithError()
4342
{
44-
$kernel = $this->getKernel($this->throwException(new \Exception('foo')));
43+
$kernel = $this->getHttpContentRenderer($this->throwException(new \Exception('foo')));
4544

4645
$loader = new \Twig_Loader_Array(array('index' => '{{ render("foo") }}'));
4746
$twig = new \Twig_Environment($loader, array('debug' => true, 'cache' => false));
@@ -50,23 +49,20 @@ public function testRenderWithError()
5049
$this->renderTemplate($kernel);
5150
}
5251

53-
protected function getKernel($return)
52+
protected function getHttpContentRenderer($return)
5453
{
55-
$kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
56-
$kernel
57-
->expects($this->once())
58-
->method('handle')
59-
->will($return)
60-
;
54+
$strategy = $this->getMock('Symfony\\Component\\HttpKernel\\RenderingStrategy\\RenderingStrategyInterface');
55+
$strategy->expects($this->once())->method('getName')->will($this->returnValue('default'));
56+
$strategy->expects($this->once())->method('render')->will($return);
6157

62-
return $kernel;
58+
return new HttpContentRenderer(array($strategy));
6359
}
6460

65-
protected function renderTemplate(HttpKernelInterface $kernel, $template = '{{ render("foo") }}')
61+
protected function renderTemplate(HttpContentRenderer $renderer, $template = '{{ render("foo") }}')
6662
{
6763
$loader = new \Twig_Loader_Array(array('index' => $template));
6864
$twig = new \Twig_Environment($loader, array('debug' => true, 'cache' => false));
69-
$twig->addExtension(new HttpKernelExtension($kernel));
65+
$twig->addExtension(new HttpKernelExtension($renderer));
7066

7167
return $twig->render('index');
7268
}

‎src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,16 @@ CHANGELOG
44
2.2.0
55
-----
66

7-
* [BC BREAK] restricted the `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` method to only accept URIs as reference
7+
* added a new `uri_signer` service to help sign URIs
8+
* deprecated `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` and `Symfony\Bundle\FrameworkBundle\HttpKernel::forward()`
9+
* deprecated the `Symfony\Bundle\FrameworkBundle\HttpKernel` class in favor of `Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel`
10+
* added support for adding new HTTP content rendering strategies (like ESI and Hinclude)
11+
in the DIC via the `kernel.content_renderer_strategy` tag
12+
* [BC BREAK] restricted the `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` method to only accept URIs or ControllerReference instances
813
* `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` method signature changed and the first argument
9-
must now be a URI (the `generateInternalUri()` method was removed)
10-
* The internal routes have been removed (`Resources/config/routing/internal.xml`)
11-
* The `render` method of the `actions` templating helper signature and arguments changed:
14+
must now be a URI or a ControllerReference instance (the `generateInternalUri()` method was removed)
15+
* The internal routes (`Resources/config/routing/internal.xml`) have been replaced with a new proxy route (`Resources/config/routing/proxy.xml`)
16+
* The `render` method of the `actions` templating helper signature and arguments changed
1217
* replaced Symfony\Bundle\FrameworkBundle\Controller\TraceableControllerResolver by Symfony\Component\HttpKernel\Controller\TraceableControllerResolver
1318
* replaced Symfony\Component\HttpKernel\Debug\ContainerAwareTraceableEventDispatcher by Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher
1419
* added Client::enableProfiler()

‎src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,10 @@ public function generateUrl($route, $parameters = array(), $referenceType = UrlG
5959
*/
6060
public function forward($controller, array $path = array(), array $query = array())
6161
{
62-
return $this->container->get('http_kernel')->forward($controller, $path, $query);
62+
$path['_controller'] = $controller;
63+
$subRequest = $this->container->get('request')->duplicate($query, null, $path);
64+
65+
return $this->container->get('http_kernel')->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
6366
}
6467

6568
/**

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/HttpRenderingStrategyPass.php

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
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\DependencyInjection\Compiler;
13+
14+
use Symfony\Component\DependencyInjection\Reference;
15+
use Symfony\Component\DependencyInjection\ContainerBuilder;
16+
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
17+
18+
/**
19+
* Adds services tagged kernel.content_renderer_strategy as HTTP content rendering strategies.
20+
*
21+
* @author Fabien Potencier <fabien@symfony.com>
22+
*/
23+
class HttpRenderingStrategyPass implements CompilerPassInterface
24+
{
25+
public function process(ContainerBuilder $container)
26+
{
27+
if (false === $container->hasDefinition('http_content_renderer')) {
28+
return;
29+
}
30+
31+
$definition = $container->getDefinition('http_content_renderer');
32+
foreach (array_keys($container->findTaggedServiceIds('kernel.content_renderer_strategy')) as $id) {
33+
// We must assume that the class value has been correctly filled, even if the service is created by a factory
34+
$class = $container->getDefinition($id)->getClass();
35+
36+
$refClass = new \ReflectionClass($class);
37+
$interface = 'Symfony\Component\HttpKernel\RenderingStrategy\RenderingStrategyInterface';
38+
if (!$refClass->implementsInterface($interface)) {
39+
throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
40+
}
41+
42+
$definition->addMethodCall('addStrategy', array(new Reference($id)));
43+
}
44+
}
45+
}

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public function load(array $configs, ContainerBuilder $container)
4141

4242
$loader->load('web.xml');
4343
$loader->load('services.xml');
44+
$loader->load('content_generator.xml');
4445

4546
// A translator must always be registered (as support is included by
4647
// default in the Form component). If disabled, an identity translator

‎src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\CompilerDebugDumpPass;
2626
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationExtractorPass;
2727
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslationDumperPass;
28+
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\HttpRenderingStrategyPass;
2829
use Symfony\Component\DependencyInjection\ContainerBuilder;
2930
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
3031
use Symfony\Component\DependencyInjection\Scope;
@@ -65,6 +66,7 @@ public function build(ContainerBuilder $container)
6566
$container->addCompilerPass(new AddCacheClearerPass());
6667
$container->addCompilerPass(new TranslationExtractorPass());
6768
$container->addCompilerPass(new TranslationDumperPass());
69+
$container->addCompilerPass(new HttpRenderingStrategyPass(), PassConfig::TYPE_AFTER_REMOVING);
6870

6971
if ($container->getParameter('kernel.debug')) {
7072
$container->addCompilerPass(new ContainerBuilderDebugDumpPass(), PassConfig::TYPE_AFTER_REMOVING);

0 commit comments

Comments
 (0)
0