Skip to content

Commit

Permalink
[FrameworkBundle] allowed attributes of the render() method to be arrays
Browse files Browse the repository at this point in the history
  • Loading branch information
fabpot committed Jan 2, 2012
1 parent cadf3d4 commit 789d5ad
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 5 deletions.
8 changes: 4 additions & 4 deletions src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
Expand Up @@ -107,11 +107,11 @@ public function render($controller, array $options = array())

// enforce all attribute values to be scalars to be sure that the same
// render() call will work with or without a reverse proxy
foreach ($options['attributes'] as $key => $value) {
if (!is_scalar($value)) {
throw new \InvalidArgumentException(sprintf('Unable to render the "%s" controller as the "%s" attribute is not a scalar.', $controller, $key));
array_walk_recursive($options['attributes'], function ($value) use ($controller) {
if (is_object($value)) {
throw new \InvalidArgumentException(sprintf('Unable to render the "%s" controller as one of the attribute is an object.', $controller));

This comment has been minimized.

Copy link
@stloyd

stloyd Jan 3, 2012

Contributor

This is BC break, as before this we can put i.e. an Request object as a param (which AFAIK is possible when normally using Controllers), now we need to hack around.

This comment has been minimized.

Copy link
@asm89

asm89 Jan 3, 2012

Contributor

This is a follow up on an earlier commit of @fabpot:
#2941
254e49b#L1R111

This comment has been minimized.

Copy link
@stof

stof Jan 3, 2012

Member

@stloyd the request can still be passed as an argument to the controller. But it makes no sense to pass a Request instance to the subrequest as the instance passed to the controller will be the Request object created for the HTTP request (be it a subrequest or a master request for ESI).

}
}
});

if (!is_array($options['alt'])) {
$options['alt'] = array($options['alt']);
Expand Down
60 changes: 59 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Tests/HttpKernelTest.php
Expand Up @@ -220,6 +220,64 @@ public function testExceptionInSubRequestsDoesNotMangleOutputBuffers()
$this->assertEquals('Foo', ob_get_clean());
}

public function testRenderAttributes()
{
$dispatcher = new EventDispatcher();
$resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');

$phpunit = $this;
$esi = $this->getMock('Symfony\\Component\\HttpKernel\\HttpCache\\Esi');
$esi
->expects($this->once())
->method('hasSurrogateEsiCapability')
->will($this->returnValue(true))
;
$esi
->expects($this->once())
->method('renderIncludeTag')
->will($this->returnCallback(function ($uri) use ($phpunit) {
$phpunit->assertEquals('foo[bar]=foo', urldecode($uri));

return 'foo';
}))
;

$router = $this->getMock('Symfony\\Component\\Routing\\Generator\\UrlGeneratorInterface');
$router
->expects($this->once())
->method('generate')
->will($this->returnCallback(function ($name, $options) {
return $options['path'];
}))
;

$request = new Request();

$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
$container->expects($this->at(2))->method('get')->with($this->equalTo('esi'))->will($this->returnValue($esi));
$container->expects($this->at(3))->method('get')->with($this->equalTo('request'))->will($this->returnValue($request));
$container->expects($this->at(4))->method('get')->with($this->equalTo('router'))->will($this->returnValue($router));
$container->expects($this->at(5))->method('get')->with($this->equalTo('request'))->will($this->returnValue($request));
$container->expects($this->at(6))->method('get')->with($this->equalTo('esi'))->will($this->returnValue($esi));

$container
->expects($this->once())
->method('has')
->with($this->equalTo('esi'))
->will($this->returnValue(true))
;

$kernel = new HttpKernel($dispatcher, $container, $resolver);

$content = $kernel->render('foo_controller', array(
'standalone' => true,
'attributes' => array('foo' => array('bar' => 'foo')),
));

// if this assertion fails, it means that the assertion in 'returnCallback' failed
$this->assertEquals('foo', $content);
}

/**
* @expectedException \InvalidArgumentException
*/
Expand All @@ -230,6 +288,6 @@ public function testRenderOnlyAllowScalarAttributes()
$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
$kernel = new HttpKernel($dispatcher, $container, $resolver);

$kernel->render('/', array('attributes' => array('foo' => new \stdClass())));
$kernel->render('/', array('attributes' => array('foo' => array('bar' => new \stdClass()))));
}
}

9 comments on commit 789d5ad

@RapotOR
Copy link
Contributor

@RapotOR RapotOR commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

Rendering an action with an object in attributes from Twig is not working anymore (http://symfony.com/doc/2.0/book/templating.html#embedding-controllers).

 {% render "AcmeArticleBundle:Article:recentArticles" with {'category': category} %}

If it is a normal result, what is the best way to call an action with an object in attributes?

Thanks and regards,

@stof
Copy link
Member

@stof stof commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

@RapotOR this is exactly what this change is about as using objects makes it impossible to switch the render tag to use ESI (the ESI request needs to pass the attributes through an url)

@vicb
Copy link
Contributor

@vicb vicb commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

Should the check be activated only when standalone is true ?

@stof
Copy link
Member

@stof stof commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

no as the goal is to make it behave the same in both cases.

@Seldaek
Copy link
Member

@Seldaek Seldaek commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

It would be cool if param converters could work in reverse there, so that if you pass app.user for example, it is transformed to its identifier, and then transformed again by the param converter. Just throwing the idea out there.

@RapotOR
Copy link
Contributor

@RapotOR RapotOR commented on 789d5ad Jan 4, 2012

Choose a reason for hiding this comment

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

If i am not wrong, the problems happens in WebProfilerBundle. An exception object is given to an action.
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/exception.html.twig#L28

@lsmith77
Copy link
Contributor

Choose a reason for hiding this comment

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

@Seldaek yeah i think it would be a nice touch if we can give some assistance there ..

@mvrhov
Copy link

@mvrhov mvrhov commented on 789d5ad Jan 23, 2012

Choose a reason for hiding this comment

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

Yep, this commit also broke WebProfilerBundle as already found out by @RapotOR.

@fabpot
Copy link
Member Author

@fabpot fabpot commented on 789d5ad Jan 23, 2012

Choose a reason for hiding this comment

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

I'm going to revert the whole thing because there are legitimate uses of passing an object directly like exception management.

Please sign in to comment.