Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
remove response as a service
The Response is not available in the DIC anymore.

When you need to create a response, create an instance of
Symfony\Component\HttpFoundation\Response instead.

As a side effect, the Controller::createResponse() and Controller::redirect()
methods have been removed and can easily be replaced as follows:

  return $this->createResponse('content', 200, array('foo' => 'bar'));
  return new Response('content', 200, array('foo' => 'bar'));

  return $this->redirect($url);
  return Response::createRedirect($url);
  • Loading branch information
fabpot committed Feb 21, 2011
1 parent bf20238 commit d94acd8
Show file tree
Hide file tree
Showing 16 changed files with 36 additions and 144 deletions.
33 changes: 0 additions & 33 deletions src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php
Expand Up @@ -23,27 +23,6 @@
*/
class Controller extends ContainerAware
{
/**
* Creates a Response instance.
*
* @param string $content The Response body
* @param integer $status The status code
* @param array $headers An array of HTTP headers
*
* @return Response A Response instance
*/
public function createResponse($content = '', $status = 200, array $headers = array())
{
$response = $this->container->get('response');
$response->setContent($content);
$response->setStatusCode($status);
foreach ($headers as $name => $value) {
$response->headers->set($name, $value);
}

return $response;
}

/**
* Generates a URL from the given parameters.
*
Expand Down Expand Up @@ -72,18 +51,6 @@ public function forward($controller, array $path = array(), array $query = array
return $this->container->get('http_kernel')->forward($controller, $path, $query);
}

/**
* Returns an HTTP redirect Response.
*
* @return Response A Response instance
*/
public function redirect($url, $status = 302)
{
$response = $this->container->get('response');
$response->setRedirect($url, $status);
return $response;
}

/**
* Returns a rendered view.
*
Expand Down
Expand Up @@ -38,21 +38,13 @@ class RedirectController extends ContainerAware
public function redirectAction($route, $permanent = false)
{
if (!$route) {
$response = $this->container->get('response');
$response->setStatusCode(410);

return $response;
return new Response(null, 410);
}

$code = $permanent ? 301 : 302;

$attributes = $this->container->get('request')->attributes->all();
unset($attributes['_route'], $attributes['route'], $attributes['permanent'] );

$response = $this->container->get('response');
$response->setRedirect($this->container->get('router')->generate($route, $attributes), $code);

return $response;
return Response::createRedirect($this->container->get('router')->generate($route, $attributes), $permanent ? 301 : 302);
}

/**
Expand All @@ -72,17 +64,9 @@ public function redirectAction($route, $permanent = false)
public function urlRedirectAction($url, $permanent = false)
{
if (!$url) {
$response = $this->container->get('response');
$response->setStatusCode(410);

return $response;
return new Response(null, 410);
}

$code = $permanent ? 301 : 302;

$response = $this->container->get('response');
$response->setRedirect($url, $code);

return $response;
return Response::createRedirect($url, $permanent ? 301 : 302);
}
}
Expand Up @@ -7,7 +7,6 @@
<parameters>
<parameter key="event_dispatcher.class">Symfony\Bundle\FrameworkBundle\EventDispatcher</parameter>
<parameter key="http_kernel.class">Symfony\Bundle\FrameworkBundle\HttpKernel</parameter>
<parameter key="response.class">Symfony\Component\HttpFoundation\Response</parameter>
<parameter key="error_handler.class">Symfony\Component\HttpKernel\Debug\ErrorHandler</parameter>
<parameter key="error_handler.level">null</parameter>
<parameter key="filesystem.class">Symfony\Bundle\FrameworkBundle\Util\Filesystem</parameter>
Expand Down Expand Up @@ -49,12 +48,6 @@
-->
<service id="request" scope="request" synthetic="true" />

<service id="response" class="%response.class%" scope="prototype">
<call method="setCharset">
<argument>%kernel.charset%</argument>
</call>
</service>

<service id="filesystem" class="%filesystem.class%"></service>

<service id="file_locator" class="%file_locator.class%">
Expand Down
Expand Up @@ -84,7 +84,7 @@ protected function getEngine($name)
public function renderResponse($view, array $parameters = array(), Response $response = null)
{
if (null === $response) {
$response = $this->container->get('response');
$response = new Response();
}

$response->setContent($this->render($view, $parameters));
Expand Down
Expand Up @@ -77,7 +77,7 @@ public function setHelpers(array $helpers)
public function renderResponse($view, array $parameters = array(), Response $response = null)
{
if (null === $response) {
$response = $this->container->get('response');
$response = new Response();
}

$response->setContent($this->render($view, $parameters));
Expand Down
Expand Up @@ -20,25 +20,14 @@
use Symfony\Bundle\FrameworkBundle\Tests\Logger;
use Symfony\Bundle\FrameworkBundle\Tests\Kernel;



/**
*
* @author Marcin Sikon<marcin.sikon@gmail.com>
*/
class RedirectControllerTest extends TestCase
{
public function testEmptyRoute()
{
$response = new Response();

$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
$container
->expects($this->once())
->method('get')
->with($this->equalTo('response'))
->will($this->returnValue($response))
;

$controller = new RedirectController();
$controller->setContainer($container);
Expand All @@ -50,21 +39,17 @@ public function testEmptyRoute()
$this->assertEquals(410, $returnResponse->getStatusCode());
}



/**
* @dataProvider provider
*/
public function testRoute($permanent, $expectedCode)
{
$response = new Response();
$request = new Request();

$route = 'new-route';
$url = '/redirect-url';
$params = array('additional-parameter' => 'value');


$request->attributes = new ParameterBag(array('route' => $route, '_route' => 'current-route', 'permanent' => $permanent) + $params);

$router = $this->getMock('Symfony\Component\Routing\RouterInterface');
Expand All @@ -86,37 +71,26 @@ public function testRoute($permanent, $expectedCode)
$container
->expects($this->at(1))
->method('get')
->with($this->equalTo('response'))
->will($this->returnValue($response));

$container
->expects($this->at(2))
->method('get')
->with($this->equalTo('router'))
->will($this->returnValue($router));


$controller = new RedirectController();
$controller->setContainer($container);

$returnResponse = $controller->redirectAction($route, $permanent);


$this->assertInstanceOf('\Symfony\Component\HttpFoundation\Response', $returnResponse);

$this->assertTrue($returnResponse->isRedirect());
$this->assertTrue($returnResponse->isRedirected($url));
$this->assertEquals($expectedCode, $returnResponse->getStatusCode());
}



public function provider()
{
return array(
array(true, 301),
array(false, 302),
);
}

}
1 change: 0 additions & 1 deletion src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml
Expand Up @@ -29,7 +29,6 @@

<service id="templating.engine.twig" class="%templating.engine.twig.class%" public="false">
<argument type="service" id="twig" />
<argument type="service" id="service_container" />
<argument type="service" id="templating.name_parser" />
<argument type="service" id="twig.globals" />
</service>
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Bundle/TwigBundle/Tests/TwigEngineTest.php
Expand Up @@ -25,7 +25,7 @@ public function testEvalutateAddsAppGlobal()
{
$environment = $this->getTwigEnvironment();
$container = $this->getContainer();
$engine = new TwigEngine($environment, $container, new TemplateNameParser(), $app = new GlobalVariables($container));
$engine = new TwigEngine($environment, new TemplateNameParser(), $app = new GlobalVariables($container));

$template = $this->getMock('\Twig_TemplateInterface');

Expand All @@ -44,7 +44,7 @@ public function testEvalutateWithoutAvailableRequest()
{
$environment = $this->getTwigEnvironment();
$container = new Container();
$engine = new TwigEngine($environment, $container, new TemplateNameParser(), new GlobalVariables($container));
$engine = new TwigEngine($environment, new TemplateNameParser(), new GlobalVariables($container));

$template = $this->getMock('\Twig_TemplateInterface');

Expand Down
7 changes: 2 additions & 5 deletions src/Symfony/Bundle/TwigBundle/TwigEngine.php
Expand Up @@ -24,21 +24,18 @@
class TwigEngine implements EngineInterface
{
protected $environment;
protected $container;
protected $parser;

/**
* Constructor.
*
* @param \Twig_Environment $environment A \Twig_Environment instance
* @param ContainerInterface $container The DI container
* @param TemplateNameParserInterface $parser A TemplateNameParserInterface instance
* @param GlobalVariables $globals A GlobalVariables instance
*/
public function __construct(\Twig_Environment $environment, ContainerInterface $container, TemplateNameParserInterface $parser, GlobalVariables $globals)
public function __construct(\Twig_Environment $environment, TemplateNameParserInterface $parser, GlobalVariables $globals)
{
$this->environment = $environment;
$this->container = $container;
$this->parser = $parser;

$environment->addGlobal('app', $globals);
Expand Down Expand Up @@ -108,7 +105,7 @@ public function supports($name)
public function renderResponse($view, array $parameters = array(), Response $response = null)
{
if (null === $response) {
$response = $this->container->get('response');
$response = new Response();
}

$response->setContent($this->render($view, $parameters));
Expand Down
Expand Up @@ -70,12 +70,10 @@ public function exportAction($token)
throw new NotFoundHttpException(sprintf('Token "%s" does not exist.', $token));
}

$response = $this->container->get('response');
$response->setContent($profiler->export());
$response->headers->set('Content-Type', 'text/plain');
$response->headers->set('Content-Disposition', 'attachment; filename= '.$token.'.txt');

return $response;
return new Response($profiler->export(), 200, array(
'Content-Type' => 'text/plain',
'Content-Disposition' => 'attachment; filename= '.$token.'.txt',
));
}

/**
Expand All @@ -89,10 +87,7 @@ public function purgeAction()
$profiler->disable();
$profiler->purge();

$response = $this->container->get('response');
$response->setRedirect($this->container->get('router')->generate('_profiler', array('token' => '-')));

return $response;
return Response::createRedirect($this->container->get('router')->generate('_profiler', array('token' => '-')));
}

/**
Expand All @@ -116,10 +111,7 @@ public function importAction()
throw new \RuntimeException('Problem uploading the data (token already exists).');
}

$response = $this->container->get('response');
$response->setRedirect($this->container->get('router')->generate('_profiler', array('token' => $token)));

return $response;
return Response::createRedirect($this->container->get('router')->generate('_profiler', array('token' => $token)));
}

/**
Expand All @@ -133,7 +125,7 @@ public function importAction()
public function toolbarAction($token, $position = null)
{
if (null === $token) {
return $this->container->get('response');
return new Response();
}

$profiler = $this->container->get('profiler');
Expand All @@ -142,7 +134,7 @@ public function toolbarAction($token, $position = null)
$profiler = $profiler->loadFromToken($token);

if ($profiler->isEmpty()) {
return $this->container->get('response');
return new Response();
}

if (null === $position) {
Expand Down Expand Up @@ -228,10 +220,7 @@ public function searchAction()
$request = $this->container->get('request');

if ($token = $request->query->get('token')) {
$response = $this->container->get('response');
$response->setRedirect($this->container->get('router')->generate('_profiler', array('token' => $token)));

return $response;
return Response::createRedirect($this->container->get('router')->generate('_profiler', array('token' => $token)));
}

$session = $request->getSession();
Expand All @@ -243,10 +232,7 @@ public function searchAction()
$profiler->disable();
$tokens = $profiler->find($ip, $url, $limit);

$response = $this->container->get('response');
$response->setRedirect($this->container->get('router')->generate('_profiler_search_results', array('token' => $tokens ? $tokens[0]['token'] : '')));

return $response;
return Response::createRedirect($this->container->get('router')->generate('_profiler_search_results', array('token' => $tokens ? $tokens[0]['token'] : '')));
}

protected function getTemplateNames($profiler)
Expand Down
16 changes: 10 additions & 6 deletions src/Symfony/Component/HttpFoundation/Response.php
Expand Up @@ -627,23 +627,27 @@ public function setNotModified()
}

/**
* Modifies the response so that it conforms to the rules defined for a redirect status code.
* Creates a redirect response so that it conforms to the rules defined for a redirect status code.
*
* @see http://tools.ietf.org/html/rfc2616#section-10.3.5
*/
public function setRedirect($url, $status = 302)
static public function createRedirect($url, $status = 302)

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 21, 2011

Author Member

That makes sense. I will create a RedirectResponse class.

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 21, 2011

Author Member
{
if (empty($url)) {
throw new \InvalidArgumentException('Cannot redirect to an empty URL.');
}

$this->setStatusCode($status);
if (!$this->isRedirect()) {
$response = new static(
sprintf('<html><head><meta http-equiv="refresh" content="1;url=%s"/></head></html>', htmlspecialchars($url, ENT_QUOTES)),
$status,
array('Location' => $url)
);

if (!$response->isRedirect()) {
throw new \InvalidArgumentException(sprintf('The HTTP status code is not a redirect ("%s" given).', $status));
}

$this->headers->set('Location', $url);
$this->setContent(sprintf('<html><head><meta http-equiv="refresh" content="1;url=%s"/></head></html>', htmlspecialchars($url, ENT_QUOTES)));
return $response;
}

/**
Expand Down

5 comments on commit d94acd8

@jwage
Copy link
Contributor

@jwage jwage commented on d94acd8 Feb 21, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is problematic for me. I don't think we should be encouraging and making available static methods for people to be using throughout their code. It breaks testability. I think the best practice and offered solution in Symfony should be a ResponseFactory instead.

@stof
Copy link
Member

@stof stof commented on d94acd8 Feb 21, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Response::createRedirect method has been dropped in the next commit.

@jwage
Copy link
Contributor

@jwage jwage commented on d94acd8 Feb 21, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it now. Cool. Looks good! :) Now we just need in the docs to not promote direct use of new Response and that a proper testable solution would be a factory.

@stof
Copy link
Member

@stof stof commented on d94acd8 Feb 21, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By ResponseFactory you mean having a service calling new Response() (which is basically what has been removed here) or the eager response supported by johannes and lsmith ?

@jwage
Copy link
Contributor

@jwage jwage commented on d94acd8 Feb 21, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it is not basically what has been removed. The DI container was creating the Response objects so it was essentially acting as the factory. Fabien didn't want the DI container to be the factory for responses. If you directly create new Responses or create them via static methods you have no chance to mock them. That is why a ResponseFactory would be ideal, so that you can mock it and tell phpunit to return a mock response when you call createResponse()

Please sign in to comment.