Skip to content

[2.8] Move argument resolving from ControllerResolver #14971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ public function load(array $configs, ContainerBuilder $container)
$definition->replaceArgument(2, E_COMPILE_ERROR | E_PARSE | E_ERROR | E_CORE_ERROR);
}

if (interface_exists('Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface')) {
Copy link
Member

Choose a reason for hiding this comment

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

there should also be a config setting to force disabling it in case you don't want to use the parameter converter, allowing to get rid of its overhead

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, we should have a configure option for all resolvers imo (so one config option in SecurityBundle too). I'm not sure I like that.

$loader->load('psr-http-message.xml');
}

$this->addClassesToCompile(array(
'Symfony\\Component\\Config\\FileLocator',

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
use Symfony\Component\HttpKernel\DependencyInjection\FragmentRendererPass;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Bundle\Bundle;
use Symfony\Component\HttpKernel\DependencyInjection\RegisterArgumentResolversPass;

/**
* Bundle.
Expand Down Expand Up @@ -88,6 +89,7 @@ public function build(ContainerBuilder $container)
$container->addCompilerPass(new TranslationDumperPass());
$container->addCompilerPass(new FragmentRendererPass(), PassConfig::TYPE_AFTER_REMOVING);
$container->addCompilerPass(new SerializerPass());
$container->addCompilerPass(new RegisterArgumentResolversPass());

if ($container->getParameter('kernel.debug')) {
$container->addCompilerPass(new ContainerBuilderDebugDumpPass(), PassConfig::TYPE_AFTER_REMOVING);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version="1.0" ?>

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<services>

<service id="argument_resolver.psr_message" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\PsrServerRequestArgumentResolver">
Copy link
Member

Choose a reason for hiding this comment

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

should be private (and the other argument resolver services too)

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure if the naming is chosen really well. Imagine that someone actually has packages under an ArgumentResolver vendor. Their service names will then conflict with these.

Copy link
Member

Choose a reason for hiding this comment

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

yeah. This services will never be accessed directly, so the service id can be less generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what ID do you propose? Symfony's service IDs are all pretty generic... controller_resolver.arguments.psr_message?

<tag name="kernel.argument_resolver" />
</service>

</services>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
<argument type="service" id="service_container" />
<argument type="service" id="controller_resolver" />
<argument type="service" id="request_stack" />
<argument type="service" id="argument_resolver.manager" />
<argument>false</argument>
</service>

Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,16 @@
<argument type="service" id="logger" on-invalid="ignore" />
</service>

<service id="argument_resolver.manager" class="Symfony\Component\HttpKernel\Controller\ArgumentResolverManager" />

<service id="argument_resolver.request" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestArgumentResolver">
<tag name="kernel.argument_resolver" />
</service>

<service id="argument_resolver.request_attributes" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestAttributesArgumentResolver">
<tag name="kernel.argument_resolver" />
</service>

<service id="response_listener" class="%response_listener.class%">
<tag name="kernel.event_subscriber" />
<argument>%kernel.charset%</argument>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

Copy link
Member

Choose a reason for hiding this comment

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

missing license header

Copy link
Member Author

Choose a reason for hiding this comment

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

added

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\SecurityBundle\ArgumentResolver;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would be better suited in the Component?


use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\ArgumentResolverInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;

/**
* Resolves a typehint for UserInterface in a controller to the current user.
*
* @author Wouter J <wouter@wouterj.nl>
*/
class UserArgumentResolver implements ArgumentResolverInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be confused if we want to resolve User by ParamConverter ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure I understand what you try to say.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, do you mean using the Doctrine Param Converter? That's why I made this resolver only work with UserInterface. When using the Doctrine Param Converter, one would actually need to typehint to a specific implementation.

Btw, this may be a good reason to implement priority for argument resolvers.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I mean Doctrine Param Converter and implement priority sound good to me.

{
/**
* @var TokenStorageInterface
*/
private $tokenStorage;

public function __construct(TokenStorageInterface $tokenStorage)
{
$this->tokenStorage = $tokenStorage;
}

/**
* {@inheritdoc}
*/
public function supports(Request $request, \ReflectionParameter $parameter)
{
$class = $parameter->getClass();
$userInterface = 'Symfony\Component\Security\Core\User\UserInterface';
$userImplementation = 'Symfony\Component\Security\Core\User\User';

return null !== $class && ($userInterface === $class->getName() || $userImplementation === $class->getName());
}

/**
* {@inheritdoc}
*/
public function resolve(Request $request, \ReflectionParameter $parameter)
{
if (null === $token = $this->tokenStorage->getToken()) {
return;
}

if (!is_object($user = $token->getUser())) {
// e.g. anonymous authentication
return;
}

return $user;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@
</parameters>

<services>

<service id="security.argument_resolver.user" class="Symfony\Bundle\SecurityBundle\ArgumentResolver\UserArgumentResolver">
<argument type="service" id="security.token_storage" />
<tag name="kernel.argument_resolver" />
</service>

<service id="security.context" class="%security.context.class%">
<argument type="service" id="security.token_storage" />
<argument type="service" id="security.authorization_checker" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<?php

namespace Symfony\Bundle\SecurityBundle\Tests\ArgumentResolver;

use Symfony\Bundle\SecurityBundle\ArgumentResolver\UserArgumentResolver;
use Symfony\Component\Security\Core\Role\Role;
use Symfony\Component\Security\Core\User\UserInterface;

/**
* @author Wouter J <wouter@wouterj.nl>
*/
class UserArgumentResolverTest extends \PHPUnit_Framework_TestCase
{
private $resolver;
private $tokenStorage;

protected function setUp()
{
$this->tokenStorage = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface');
$this->resolver = new UserArgumentResolver($this->tokenStorage);
}

/**
* @dataProvider provideClasses
*/
public function testSupports($class, $supported = true)
{
$this->assertEquals($supported, $this->resolver->supports($this->getRequestMock(), $this->getReflectionParameterMock($class)));
}

public function provideClasses()
{
return array(
array('Symfony\Component\Security\Core\User\UserInterface'),
array('Symfony\Component\Security\Core\User\User'),
array('Symfony\Bundle\SecurityBundle\Tests\ArgumentResolver\UserFixture', false),
array('\stdClass', false),
);
}

public function testResolvesToNullWhenNoUserIsAvailable()
{
$this->tokenStorage->expects($this->any())->method('getToken')->willReturn(null);

$this->assertNull($this->resolver->resolve($this->getRequestMock(), $this->getReflectionParameterMock()));
}

public function testResolvesToNullWhenUserIsAnonymous()
{
$token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
$token->expects($this->any())->method('getUser')->willReturn('anon.');

$this->tokenStorage->expects($this->any())->method('getToken')->willReturn($token);

$this->assertNull($this->resolver->resolve($this->getRequestMock(), $this->getReflectionParameterMock()));
}

public function testResolvesToUser()
{
$user = $this->getMock('Symfony\component\Security\Core\User\User');

$token = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
$token->expects($this->any())->method('getUser')->willReturn($user);

$this->tokenStorage->expects($this->any())->method('getToken')->willReturn($token);

$this->assertEquals($user, $this->resolver->resolve($this->getRequestMock(), $this->getReflectionParameterMock()));
}

private function getRequestMock()
{
return $this->getMock('Symfony\Component\HttpFoundation\Request');
}

private function getReflectionParameterMock($class = null)
{
$reflectionParameter = $this->getMockBuilder('\ReflectionParameter')->disableOriginalConstructor()->getMock();

if (null !== $class) {
$reflectionClass = $this->getMockBuilder('\ReflectionClass')->disableOriginalConstructor()->getMock();
$reflectionClass->expects($this->any())->method('getName')->willReturn($class);
$reflectionParameter->expects($this->any())->method('getClass')->willReturn($reflectionClass);
}

return $reflectionParameter;
}
}

class UserFixture implements UserInterface
{
public function getRoles() {}
public function getPassword() {}
public function getSalt() {}
public function getUsername() {}
public function eraseCredentials() {}
}
6 changes: 6 additions & 0 deletions src/Symfony/Component/HttpKernel/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
CHANGELOG
=========

2.8.0
-----

* added argument resolvers
* deprecated `Symfony\Component\HttpKernel\Controller\ControllerResolver::getArguments()` and `doGetArguments()` in favor of argument resolvers

2.7.0
-----

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;

/**
* An ArgumentResolverInterface implementation resolves the arguments of
* controllers.
*
* @author Wouter J <wouter@wouterj.nl>
*/
interface ArgumentResolverInterface
{
/**
* Checks if the current pameter can be esolved by this argument
* resolver.
*
* @param Request $request
* @param \ReflectionParameter $parameter
*
* @return bool
*/
public function supports(Request $request, \ReflectionParameter $parameter);

/**
* Resolves the current parameter into an argument.
*
* @param Request $request
* @param \ReflectionParameter $parameter
*
* @return mixed The resolved argument
*/
public function resolve(Request $request, \ReflectionParameter $parameter);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface;
use Symfony\Component\HttpFoundation\Request;

/**
* Converts HttpFoundation Request to PSR-7 ServerRequest using the bridge.
*
* @author Kévin Dunglas <dunglas@gmail.com>
* @author Wouter J <wouter@wouterj.nl>
*/
class PsrServerRequestArgumentResolver implements ArgumentResolverInterface
{
/**
* @var array
*/
private static $supportedTypes = array(
'Psr\Http\Message\ServerRequestInterface' => true,
'Psr\Http\Message\RequestInterface' => true,
'Psr\Http\Message\MessageInterface' => true,
);

/**
* @var HttpMessageFactoryInterface
*/
private $httpMessageFactory;

public function __construct(HttpMessageFactoryInterface $httpMessageFactory)
{
$this->httpMessageFactory = $httpMessageFactory;
}

/**
* {@inheritdoc}
*/
public function supports(Request $request, \ReflectionParameter $parameter)
{
$class = $parameter->getClass();

return null !== $class && isset(self::$supportedTypes[$class->getName()]);
}

/**
* {@inheritdoc}
*/
public function resolve(Request $request, \ReflectionParameter $parameter)
{
return $this->httpMessageFactory->createRequest($request);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;

/**
* Resolves arguments typehinting for the HttpFoundation Request object.
*
* @author Wouter J <wouter@wouterj.nl>
*/
class RequestArgumentResolver implements ArgumentResolverInterface
{
/**
* {@inheritdoc}
*/
public function supports(Request $request, \ReflectionParameter $parameter)
{
$class = $parameter->getClass();

return $class && $class->isInstance($request);
}

/**
* {@inheritdoc}
*/
public function resolve(Request $request, \ReflectionParameter $parameter)
{
return $request;
}
}
Loading