[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.
symfony · Jan 23, 2013 · b9f0e17 · b9f0e17
1 parent 8a351f0
commit b9f0e17
Show file tree

Hide file tree

Showing 12 changed files with 65 additions and 51 deletions.
diff --git 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
@@ -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 <fabien@symfony.com>
+ *
+ * @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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -35,15 +35,15 @@ 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()
     {
         $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
@@ -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/...fony/Component/HttpKernel/Tests/RenderingStrategy/GeneratorAwareRenderingStrategyTest.php b/...fony/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,15 +49,15 @@ 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('/'));
     }
 
     /**
      * @dataProvider getGeneratorProxyUriData
      */
     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
@@ -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('<hx:include src="/main_controller.html?_hash=6MuxpWUHcqIddMMmoN36uPsEjws%3D"></hx:include>', $strategy->render(new ControllerReference('main_controller', array(), array()))->getContent());
+        $this->assertEquals('<hx:include src="/main_controller.html?_hash=6MuxpWUHcqIddMMmoN36uPsEjws%3D"></hx:include>', $strategy->render(new ControllerReference('main_controller', array(), array()), Request::create('/'))->getContent());
     }
 
     public function testRenderWithUri()
     {
         $strategy = new HIncludeRenderingStrategy();
-        $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo')->getContent());
+        $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo', Request::create('/'))->getContent());
 
         $strategy = new HIncludeRenderingStrategy(null, new UriSigner('foo'));
-        $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo')->getContent());
+        $this->assertEquals('<hx:include src="/foo"></hx:include>', $strategy->render('/foo', Request::create('/'))->getContent());
     }
 
     public function testRenderWhithDefault()
     {
         // only default
         $strategy = new HIncludeRenderingStrategy();
-        $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', null, array('default' => 'default'))->getContent());
+        $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent());
 
         // only global default
         $strategy = new HIncludeRenderingStrategy(null, null, 'global_default');
-        $this->assertEquals('<hx:include src="/foo">global_default</hx:include>', $strategy->render('/foo', null, array())->getContent());
+        $this->assertEquals('<hx:include src="/foo">global_default</hx:include>', $strategy->render('/foo', Request::create('/'), array())->getContent());
 
         // global default and default
         $strategy = new HIncludeRenderingStrategy(null, null, 'global_default');
-        $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', null, array('default' => 'default'))->getContent());
+        $this->assertEquals('<hx:include src="/foo">default</hx:include>', $strategy->render('/foo', Request::create('/'), array('default' => 'default'))->getContent());
     }
 }