8000 [HttpKernel] Handle multi-attribute controller arguments by chalasr · Pull Request #40307 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Handle multi-attribute controller arguments #40307

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 26, 2021
Merged

[HttpKernel] Handle multi-attribute controller arguments #40307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions UPGRADE-5.3.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ HttpF 10000 oundation
HttpKernel
----------

* Deprecate `ArgumentInterface`
* Deprecate `ArgumentMetadata::getAttribute()`, use `getAttributes()` instead
* Marked the class `Symfony\Component\HttpKernel\EventListener\DebugHandlersListener` as internal

Messenger
Expand Down
2 changes: 2 additions & 0 deletions UPGRADE-6.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ HttpFoundation
HttpKernel
----------

* Remove `ArgumentInterface`
* Remove `ArgumentMetadata::getAttribute()`, use `getAttributes()` instead
* Made `WarmableInterface::warmUp()` return a list of classes or files to preload on PHP 7.4+
* Removed support for `service:action` syntax to reference controllers. Use `serviceOrFqcn::method` instead.

Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Component/HttpKernel/Attribute/ArgumentInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@

namespace Symfony\Component\HttpKernel\Attribute;

trigger_deprecation('symfony/http-kernel', '5.3', 'The "%s" interface is deprecated.', ArgumentInterface::class);

/**
* Marker interface for controller argument attributes.
*
* @deprecated since Symfony 5.3
*/
interface ArgumentInterface
{
Expand Down
3 changes: 3 additions & 0 deletions src/Symfony/Component/HttpKernel/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ CHANGELOG
5.3
---

* Deprecate `ArgumentInterface`
* Add `ArgumentMetadata::getAttributes()`
* Deprecate `ArgumentMetadata::getAttribute()`, use `getAttributes()` instead
* marked the class `Symfony\Component\HttpKernel\EventListener\DebugHandlersListener` as internal

5.2.0
Expand Down
52 changes: 48 additions & 4 deletions src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,34 @@
*/
class ArgumentMetadata
{
public const IS_INSTANCEOF = 2;

private $name;
private $type;
private $isVariadic;
private $hasDefaultValue;
private $defaultValue;
private $isNullable;
private $attribute;
private $attributes;

public function __construct(string $name, ?string $type, bool $isVariadic, bool $hasDefaultValue, $defaultValue, bool $isNullable = false, ?ArgumentInterface $attribute = null)
/**
* @param object[] $attributes
*/
public function __construct(string $name, ?string $type, bool $isVariadic, bool $hasDefaultValue, $defaultValue, bool $isNullable = false, $attributes = [])
{
$this->name = $name;
$this->type = $type;
$this->isVariadic = $isVariadic;
$this->hasDefaultValue = $hasDefaultValue;
$this->defaultValue = $defaultValue;
$this->isNullable = $isNullable || null === $type || ($hasDefaultValue && null === $defaultValue);
$this->attribute = $attribute;

if (null === $attributes || $attributes instanceof ArgumentInterface) {
trigger_deprecation('symfony/http-kernel', '5.3', 'The "%s" constructor expects an array of PHP attributes as last argument, %s given.', __CLASS__, get_debug_type($attributes));
$attributes = $attributes ? [$attributes] : [];
}

$this->attributes = $attributes;
}

/**
Expand Down Expand Up @@ -114,6 +125,39 @@ public function getDefaultValue()
*/
public function getAttribute(): ?ArgumentInterface
{
return $this->attribute;
trigger_deprecation('symfony/http-kernel', '5.3', 'Method "%s()" is deprecated, use "getAttributes()" instead.', __METHOD__);

if (!$this->attributes) {
return null;
}
629A
return $this->attributes[0] instanceof ArgumentInterface ? $this->attributes[0] : null;
}

/**
* @return object[]
*/
public function getAttributes(string $name = null, int $flags = 0): array
{
if (!$name) {
return $this->attributes;
}

$attributes = [];
if ($flags & self::IS_INSTANCEOF) {
foreach ($this->attributes as $attribute) {
if ($attribute instanceof $name) {
$attributes[] = $attribute;
}
}
} else {
foreach ($this->attributes as $attribute) {
if (\get_class($attribute) === $name) {
$attributes[] = $attribute;
}
}
}

return $attributes;
}
}
24 changes: 4 additions & 20 deletions src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@

namespace Symfony\Component\HttpKernel\ControllerMetadata;

use Symfony\Component\HttpKernel\Attribute\ArgumentInterface;
use Symfony\Component\HttpKernel\Exception\InvalidMetadataException;

/**
* Builds {@see ArgumentMetadata} objects based on the given Controller.
*
Expand All @@ -37,28 +34,15 @@ public function createArgumentMetadata($controller): array
}

foreach ($reflection->getParameters() as $param) {
$attribute = null;
if (\PHP_VERSION_ID >= 80000) {
$reflectionAttributes = $param->getAttributes(ArgumentInterface::class, \ReflectionAttribute::IS_INSTANCEOF);

if (\count($reflectionAttributes) > 1) {
$representative = $controller;

if (\is_array($representative)) {
$representative = sprintf('%s::%s()', \get_class($representative[0]), $representative[1]);
} elseif (\is_object($representative)) {
$representative = \get_class($representative);
foreach ($param->getAttributes() as $reflectionAttribute) {
if (class_exists($reflectionAttribute->getName())) {
$attributes[] = $reflectionAttribute->newInstance();
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 still not convinced we should call newInstance() on arbitrary attributes. Do we have a way to whitelist relevant attributes beforehand? Note that this is more or less what the now-deprecated ArgumentInterface did for us.

Alternatively: Can we make the newInstance() call lazy, so we only perform it if $argumentMetadata->getAttributes(MyAttribute::class) is called?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, we could use some closures for that. But is it really worth it? Instantiation should not be heavy...

Copy link
Member
@derrabus derrabus Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It shouldn't. Still, we trigger autoloading and attribute validation and execute someone else's code here. The reflection API intentionally gives us a way to prevent this. Why shouldn't we use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the autoloading will get triggered anyway when using getAttributes in IS_INSTANCEOF mode since we have an argument resolver in Symfony that does that it will be triggered anyway.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, autoloading might be triggered anyway if we use IS_INSTANCEOF.

Copy link
Member Author

Choose a reason for hiding this comment

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

The scope of the argument value resolver covers Symfony controllers exclusively.
Controllers are auto-registered callables that are executed at runtime, which might use third-party extensions we don't even know about. I mean, we do run someone else's code all the time in this area (take e.g sensio/framework-extra-bundle param converters... reflection-based logic is way more heavy there.)

So, after thinking twice at the possible alternatives, I propose to keep this PR as-is and iterate.
If someone comes up with a problematic case, we can reconsider pretty easily.

Copy link
Member

Choose a reason for hiding this comment

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

I propose to keep this PR as-is and iterate.

👍🏻 We agree on the goal of the PR and discuss an implementation detail here. Let's merge and I'll try to create a follow-up PR to avoid unwanted newInstance() calls.

If someone comes up with a problematic case, we can reconsider pretty easily.

Sorry for being so persistent here. I'd like to avoid the problematic case before the bugfix becomes a BC break.

}

throw new InvalidMetadataException(sprintf('Controller "%s" has more than one attribute for "$%s" argument.', $representative, $param->getName()));
}

if (isset($reflectionAttributes[0])) {
$attribute = $reflectionAttributes[0]->newInstance();
}
}

$arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param, $reflection), $param->isVariadic(), $param->isDefaultValueAvailable(), $param->isDefaultValueAvailable() ? $param->getDefaultValue() : null, $param->allowsNull(), $attribute);
$arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param, $reflection), $param->isVariadic(), $param->isDefaultValueAvailable(), $param->isDefaultValueAvailable() ? $param->getDefaultValue() : null, $param->allowsNull(), $attributes ?? []);
}

return $arguments;
Expand Down
10 changes: 4 additions & 6 deletions src/Symfony/Component/HttpKernel/Tests/ControllerMetadata/ArgumentMetadataFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory;
use Symfony\Component\HttpKernel\Exception\InvalidMetadataException;
use Symfony\Component\HttpKernel\Tests\Fixtures\Attribute\Foo;
use Symfony\Component\HttpKernel\Tests\Fixtures\Controller\AttributeController;
use Symfony\Component\HttpKernel\Tests\Fixtures\Controller\BasicTypesController;
Expand Down Expand Up @@ -128,18 +127,17 @@ public function testAttributeSignature()
$arguments = $this->factory->createArgumentMetadata([new AttributeController(), 'action']);

$this->assertEquals([
new ArgumentMetadata('baz', 'string', false, false, null, false, new Foo('bar')),
new ArgumentMetadata('baz', 'string', false, false, null, false, [new Foo('bar')]),
], $arguments);
}

/**
* @requires PHP 8
*/
public function testAttributeSignatureError()
public function testMultipleAttributes()
{
$this->expectException(InvalidMetadataException::class);

$this->factory->createArgumentMetadata([new AttributeController(), 'invalidAction']);
$this->factory->createArgumentMetadata([new AttributeController(), 'multiAttributeArg']);
$this->assertCount(1, $this->factory->createArgumentMetadata([new AttributeController(), 'multiAttributeArg'])[0]->getAttributes());
}

private function signature1(self $foo, array $bar, callable $baz)
Expand Down
28 changes: 28 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/ControllerMetadata/ArgumentMetadataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@
namespace Symfony\Component\HttpKernel\Tests\ControllerMetadata;

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\HttpKernel\Attribute\ArgumentInterface;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;
use Symfony\Component\HttpKernel\Tests\Fixtures\Attribute\Foo;

class ArgumentMetadataTest extends TestCase
{
use ExpectDeprecationTrait;

public function testWithBcLayerWithDefault()
{
$argument = new ArgumentMetadata('foo', 'string', false, true, 'default value');
Expand All @@ -41,4 +46,27 @@ public function testDefaultValueUnavailable()
$this->assertFalse($argument->hasDefaultValue());
$argument->getDefaultValue();
}

/**
* @group legacy
*/
public function testLegacyAttribute()
{
$attribute = $this->createMock(ArgumentInterface::class);

$this->expectDeprecation('Since symfony/http-kernel 5.3: The "Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata" constructor expects an array of PHP attributes as last argument, %s given.');
$this->expectDeprecation('Since symfony/http-kernel 5.3: Method "Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata::getAttribute()" is deprecated, use "getAttributes()" instead.');

$argument = new ArgumentMetadata('foo', 'string', false, true, 'default value', true, $attribute);
$this->assertSame($attribute, $argument->getAttribute());
}

/**
* @requires PHP 8
*/
public function testGetAttributes()
{
$argument = new ArgumentMetadata('foo', 'string', false, true, 'default value', true, [new Foo('bar')]);
$this->assertEquals([new Foo('bar')], $argument->getAttributes());
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/Tests/Fixtures/Attribute/Foo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
use Symfony\Component\HttpKernel\Attribute\ArgumentInterface;

#[\Attribute(\Attribute::TARGET_PARAMETER)]
class Foo implements ArgumentInterface
class Foo
{
private $foo;

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpKernel/Tests/Fixtures/Controller/AttributeController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ class AttributeController
public function action(#[Foo('bar')] string $baz) {
}

public function invalidAction(#[Foo('bar'), Foo('bar')] string $baz) {
public function multiAttributeArg(#[Foo('bar'), Undefined('bar')] string $baz) {
}
}
4 changes: 1 addition & 3 deletions src/Symfony/Component/Security/Http/Attribute/CurrentUser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,10 @@

namespace Symfony\Component\Security\Http\Attribute;

use Symfony\Component\HttpKernel\Attribute\ArgumentInterface;

/**
* Indicates that a controller argument should receive the current logged user.
*/
#[\Attribute(\Attribute::TARGET_PARAMETER)]
class CurrentUser implements ArgumentInterface
class CurrentUser
{
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/Controller/UserValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public function supports(Request $request, ArgumentMetadata $argument): bool
{
// with the attribute, the type can be any UserInterface implementation
// otherwise, the type must be UserInterface
if (UserInterface::class !== $argument->getType() && !$argument->getAttribute() instanceof CurrentUser) {
if (UserInterface::class !== $argument->getType() && !$argument->getAttributes(CurrentUser::class, ArgumentMetadata::IS_INSTANCEOF)) {
return false;
}

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/Security/Http/Tests/Controller/UserValueResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ public function testResolveWithAttribute()
$tokenStorage->setToken($token);

$resolver = new UserValueResolver($tokenStorage);
$metadata = new ArgumentMetadata('foo', null, false, false, null, false, new CurrentUser());
$metadata = $this->createMock(ArgumentMetadata::class);
$metadata = new ArgumentMetadata('foo', null, false, false, null, false, [new CurrentUser()]);

$this->assertTrue($resolver->supports(Request::create('/'), $metadata));
$this->assertSame([$user], iterator_to_array($resolver->resolve(Request::create('/'), $metadata)));
Expand All @@ -89,7 +90,7 @@ public function testResolveWithAttributeAndNoUser()
$tokenStorage->setToken(new UsernamePasswordToken('username', 'password', 'provider'));

$resolver = new UserValueResolver($tokenStorage);
$metadata = new ArgumentMetadata('foo', null, false, false, null, false, new CurrentUser());
$metadata = new ArgumentMetadata('foo', null, false, false, null, false, [new CurrentUser()]);

$this->assertFalse($resolver->supports(Request::create('/'), $metadata));
}
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"symfony/deprecation-contracts": "^2.1",
"symfony/security-core": "^5.3",
"symfony/http-foundation": "^5.2",
"symfony/http-kernel": "^5.2",
"symfony/http-kernel": "^5.3",
"symfony/polyfill-php80": "^1.15",
"symfony/property-access": "^4.4|^5.0"
},
Expand Down
0