From b9f0e17e673b1085d5a7c0678749e5966c3ee345 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Tue, 22 Jan 2013 09:06:13 +0100 Subject: [PATCH] [HttpKernel] made the Request required when using rendering strategies The previous code allowed to pass null as a Request but that does not really make sense as rendering a sub-request can only happen from a master request. This was done to ease testing but that was a mistake. --- .../Extension/HttpKernelExtensionTest.php | 21 ++++++++++++------- .../HttpKernel/HttpContentRenderer.php | 10 ++++++--- .../DefaultRenderingStrategy.php | 21 +++++++------------ .../EsiRenderingStrategy.php | 4 ++-- .../GeneratorAwareRenderingStrategy.php | 6 ++---- .../HIncludeRenderingStrategy.php | 2 +- .../RenderingStrategyInterface.php | 2 +- .../Tests/HttpContentRendererTest.php | 14 ++++++++++++- .../DefaultRenderingStrategyTest.php | 10 ++++----- .../EsiRenderingStrategyTest.php | 2 +- .../GeneratorAwareRenderingStrategyTest.php | 10 ++++----- .../HIncludeRenderingStrategyTest.php | 14 ++++++------- 12 files changed, 65 insertions(+), 51 deletions(-) diff --git a/src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php b/src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php index 489d36bfb205..f9c3f31d537f 100644 --- a/src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php +++ b/src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php @@ -13,6 +13,7 @@ use Symfony\Bridge\Twig\Extension\HttpKernelExtension; use Symfony\Bridge\Twig\Tests\TestCase; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\HttpContentRenderer; @@ -29,13 +30,6 @@ protected function setUp() } } - public function testRenderWithoutMasterRequest() - { - $kernel = $this->getHttpContentRenderer($this->returnValue(new Response('foo'))); - - $this->assertEquals('foo', $this->renderTemplate($kernel)); - } - /** * @expectedException \Twig_Error_Runtime */ @@ -56,7 +50,18 @@ protected function getHttpContentRenderer($return) $strategy->expects($this->once())->method('getName')->will($this->returnValue('default')); $strategy->expects($this->once())->method('render')->will($return); - return new HttpContentRenderer(array($strategy)); + // 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 HttpContentRenderer(array($strategy)); + $renderer->onKernelRequest($event); + + return $renderer; } protected function renderTemplate(HttpContentRenderer $renderer, $template = '{{ render("foo") }}') diff --git a/src/Symfony/Component/HttpKernel/HttpContentRenderer.php b/src/Symfony/Component/HttpKernel/HttpContentRenderer.php index 34b18e13d749..337cb6d11c77 100644 --- a/src/Symfony/Component/HttpKernel/HttpContentRenderer.php +++ b/src/Symfony/Component/HttpKernel/HttpContentRenderer.php @@ -23,7 +23,13 @@ /** * Renders a URI using different strategies. * + * This class handles sub-requests. The response content from the sub-request + * is then embedded into a master request. The handling of the sub-request + * is managed by rendering strategies. + * * @author Fabien Potencier + * + * @see RenderingStrategyInterface */ class HttpContentRenderer implements EventSubscriberInterface { @@ -103,9 +109,7 @@ public function render($uri, $strategy = 'default', array $options = array()) throw new \InvalidArgumentException(sprintf('The "%s" rendering strategy does not exist.', $strategy)); } - $request = $this->requests ? $this->requests[0] : null; - - return $this->deliver($this->strategies[$strategy]->render($uri, $request, $options)); + return $this->deliver($this->strategies[$strategy]->render($uri, $this->requests[0], $options)); } /** diff --git a/src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php b/src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php index e1c737a05dc6..058c82fe62f9 100644 --- a/src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php +++ b/src/Symfony/Component/HttpKernel/RenderingStrategy/DefaultRenderingStrategy.php @@ -42,7 +42,7 @@ public function __construct(HttpKernelInterface $kernel) * * * alt: an alternative URI to render in case of an error */ - public function render($uri, Request $request = null, array $options = array()) + public function render($uri, Request $request, array $options = array()) { if ($uri instanceof ControllerReference) { $uri = $this->generateProxyUri($uri, $request); @@ -74,21 +74,16 @@ public function render($uri, Request $request = null, array $options = array()) } } - protected function createSubRequest($uri, Request $request = null) + protected function createSubRequest($uri, Request $request) { - if (null !== $request) { - $cookies = $request->cookies->all(); - $server = $request->server->all(); - - // the sub-request is internal - $server['REMOTE_ADDR'] = '127.0.0.1'; - } else { - $cookies = array(); - $server = array(); - } + $cookies = $request->cookies->all(); + $server = $request->server->all(); + + // the sub-request is internal + $server['REMOTE_ADDR'] = '127.0.0.1'; $subRequest = Request::create($uri, 'get', array(), $cookies, array(), $server); - if (null !== $request && $session = $request->getSession()) { + if ($session = $request->getSession()) { $subRequest->setSession($session); } diff --git a/src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php b/src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php index e1ecc8a8cb1d..945a388ac745 100644 --- a/src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php +++ b/src/Symfony/Component/HttpKernel/RenderingStrategy/EsiRenderingStrategy.php @@ -55,9 +55,9 @@ public function __construct(Esi $esi, RenderingStrategyInterface $defaultStrateg * * @see Symfony\Component\HttpKernel\HttpCache\ESI */ - public function render($uri, Request $request = null, array $options = array()) + public function render($uri, Request $request, array $options = array()) { - if (null === $request || !$this->esi->hasSurrogateEsiCapability($request)) { + if (!$this->esi->hasSurrogateEsiCapability($request)) { return $this->defaultStrategy->render($uri, $request, $options); } diff --git a/src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php b/src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php index a5ba272f81e9..57e0d55407c7 100644 --- a/src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php +++ b/src/Symfony/Component/HttpKernel/RenderingStrategy/GeneratorAwareRenderingStrategy.php @@ -50,7 +50,7 @@ public function setUrlGenerator(UrlGeneratorInterface $generator) * @throws \LogicException when the _proxy route is not available * @throws \LogicException when there is no registered route generator */ - protected function generateProxyUri(ControllerReference $reference, Request $request = null) + protected function generateProxyUri(ControllerReference $reference, Request $request) { if (null === $this->generator) { throw new \LogicException('Unable to generate a proxy URL as there is no registered route generator.'); @@ -59,10 +59,8 @@ protected function generateProxyUri(ControllerReference $reference, Request $req if (isset($reference->attributes['_format'])) { $format = $reference->attributes['_format']; unset($reference->attributes['_format']); - } elseif (null !== $request) { - $format = $request->getRequestFormat(); } else { - $format = 'html'; + $format = $request->getRequestFormat(); } try { diff --git a/src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php b/src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php index cdcb6acee910..fa13179180ec 100644 --- a/src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php +++ b/src/Symfony/Component/HttpKernel/RenderingStrategy/HIncludeRenderingStrategy.php @@ -53,7 +53,7 @@ public function __construct($templating = null, UriSigner $signer = null, $globa * * * default: The default content (it can be a template name or the content) */ - public function render($uri, Request $request = null, array $options = array()) + public function render($uri, Request $request, array $options = array()) { if ($uri instanceof ControllerReference) { if (null === $this->signer) { diff --git a/src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php b/src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php index fd795445e0d4..b4f8b8e50b40 100644 --- a/src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php +++ b/src/Symfony/Component/HttpKernel/RenderingStrategy/RenderingStrategyInterface.php @@ -32,7 +32,7 @@ interface RenderingStrategyInterface * * @return Response A Response instance */ - public function render($uri, Request $request = null, array $options = array()); + public function render($uri, Request $request, array $options = array()); /** * Gets the name of the strategy. diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php index f97bf51451ed..1051819697a4 100644 --- a/src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/HttpContentRendererTest.php @@ -12,6 +12,7 @@ namespace Symfony\Component\HttpKernel\Tests; use Symfony\Component\HttpKernel\HttpContentRenderer; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; class HttpContentRendererTest extends \PHPUnit_Framework_TestCase @@ -34,6 +35,8 @@ public function testRenderWhenStrategyDoesNotExist() public function testRender() { + $request = Request::create('/'); + $strategy = $this->getMock('Symfony\Component\HttpKernel\RenderingStrategy\RenderingStrategyInterface'); $strategy ->expects($this->any()) @@ -43,13 +46,22 @@ public function testRender() $strategy ->expects($this->any()) ->method('render') - ->with('/', null, array('foo' => 'foo', 'ignore_errors' => true)) + ->with('/', $request, array('foo' => 'foo', 'ignore_errors' => true)) ->will($this->returnValue(new Response('foo'))) ; $renderer = new HttpContentRenderer(); $renderer->addStrategy($strategy); + // 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->onKernelRequest($event); + $this->assertEquals('foo', $renderer->render('/', 'foo', array('foo' => 'foo'))); } diff --git a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php index 8a208f48e389..2a6d9ab7b336 100644 --- a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/DefaultRenderingStrategyTest.php @@ -35,7 +35,7 @@ public function testRender() { $strategy = new DefaultRenderingStrategy($this->getKernel($this->returnValue(new Response('foo')))); - $this->assertEquals('foo', $strategy->render('/')->getContent()); + $this->assertEquals('foo', $strategy->render('/', Request::create('/'))->getContent()); } public function testRenderWithControllerReference() @@ -43,7 +43,7 @@ public function testRenderWithControllerReference() $strategy = new DefaultRenderingStrategy($this->getKernel($this->returnValue(new Response('foo')))); $strategy->setUrlGenerator($this->getUrlGenerator()); - $this->assertEquals('foo', $strategy->render(new ControllerReference('main_controller', array(), array()))->getContent()); + $this->assertEquals('foo', $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'))->getContent()); } /** @@ -53,14 +53,14 @@ public function testRenderExceptionNoIgnoreErrors() { $strategy = new DefaultRenderingStrategy($this->getKernel($this->throwException(new \RuntimeException('foo')))); - $this->assertEquals('foo', $strategy->render('/')->getContent()); + $this->assertEquals('foo', $strategy->render('/', Request::create('/'))->getContent()); } public function testRenderExceptionIgnoreErrors() { $strategy = new DefaultRenderingStrategy($this->getKernel($this->throwException(new \RuntimeException('foo')))); - $this->assertEmpty($strategy->render('/', null, array('ignore_errors' => true))->getContent()); + $this->assertEmpty($strategy->render('/', Request::create('/'), array('ignore_errors' => true))->getContent()); } public function testRenderExceptionIgnoreErrorsWithAlt() @@ -70,7 +70,7 @@ public function testRenderExceptionIgnoreErrorsWithAlt() $this->returnValue(new Response('bar')) ))); - $this->assertEquals('bar', $strategy->render('/', null, array('ignore_errors' => true, 'alt' => '/foo'))->getContent()); + $this->assertEquals('bar', $strategy->render('/', Request::create('/'), array('ignore_errors' => true, 'alt' => '/foo'))->getContent()); } private function getKernel($returnValue) diff --git a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php index cea8aeee5d40..90241a7616a7 100644 --- a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/EsiRenderingStrategyTest.php @@ -32,7 +32,7 @@ protected function setUp() public function testRenderFallbackToDefaultStrategyIfNoRequest() { $strategy = new EsiRenderingStrategy(new Esi(), $this->getDefaultStrategy(true)); - $strategy->render('/'); + $strategy->render('/', Request::create('/')); } public function testRenderFallbackToDefaultStrategyIfEsiNotSupported() diff --git a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php index 387ab3e2a0f2..8d7a0b54b1a5 100644 --- a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php @@ -32,7 +32,7 @@ protected function setUp() public function testGenerateProxyUriWithNoGenerator() { $strategy = new Strategy(); - $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array())); + $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array()), Request::create('/')); } /** @@ -49,7 +49,7 @@ public function testGenerateProxyUriWhenRouteNotFound() $strategy = new Strategy(); $strategy->setUrlGenerator($generator); - $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array())); + $strategy->doGenerateProxyUri(new ControllerReference('controller', array(), array()), Request::create('/')); } /** @@ -57,7 +57,7 @@ public function testGenerateProxyUriWhenRouteNotFound() */ public function testGenerateProxyUri($uri, $controller) { - $this->assertEquals($uri, $this->getStrategy()->doGenerateProxyUri($controller)); + $this->assertEquals($uri, $this->getStrategy()->doGenerateProxyUri($controller, Request::create('/'))); } public function getGeneratorProxyUriData() @@ -91,10 +91,10 @@ private function getStrategy() class Strategy extends GeneratorAwareRenderingStrategy { - public function render($uri, Request $request = null, array $options = array()) {} + public function render($uri, Request $request, array $options = array()) {} public function getName() {} - public function doGenerateProxyUri(ControllerReference $reference, Request $request = null) + public function doGenerateProxyUri(ControllerReference $reference, Request $request) { return parent::generateProxyUri($reference, $request); } diff --git a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php index 8e81b3be2d41..c78f2d153b5d 100644 --- a/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/RenderingStrategy/HIncludeRenderingStrategyTest.php @@ -35,37 +35,37 @@ protected function setUp() public function testRenderExceptionWhenControllerAndNoSigner() { $strategy = new HIncludeRenderingStrategy(); - $strategy->render(new ControllerReference('main_controller', array(), array())); + $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/')); } public function testRenderWithControllerAndSigner() { $strategy = new HIncludeRenderingStrategy(null, new UriSigner('foo')); $strategy->setUrlGenerator($this->getUrlGenerator()); - $this->assertEquals('', $strategy->render(new ControllerReference('main_controller', array(), array()))->getContent()); + $this->assertEquals('', $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'))->getContent()); } public function testRenderWithUri() { $strategy = new HIncludeRenderingStrategy(); - $this->assertEquals('', $strategy->render('/foo')->getContent()); + $this->assertEquals('', $strategy->render('/foo', Request::create('/'))->getContent()); $strategy = new HIncludeRenderingStrategy(null, new UriSigner('foo')); - $this->assertEquals('', $strategy->render('/foo')->getContent()); + $this->assertEquals('', $strategy->render('/foo', Request::create('/'))->getContent()); } public function testRenderWhithDefault() { // only default $strategy = new HIncludeRenderingStrategy(); - $this->assertEquals('default', $strategy->render('/foo', null, array('default' => 'default'))->getContent()); + $this->assertEquals('default', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent()); // only global default $strategy = new HIncludeRenderingStrategy(null, null, 'global_default'); - $this->assertEquals('global_default', $strategy->render('/foo', null, array())->getContent()); + $this->assertEquals('global_default', $strategy->render('/foo', Request::create('/'), array())->getContent()); // global default and default $strategy = new HIncludeRenderingStrategy(null, null, 'global_default'); - $this->assertEquals('default', $strategy->render('/foo', null, array('default' => 'default'))->getContent()); + $this->assertEquals('default', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent()); } }