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

Jump to bottom
Merged
merged 1 commit into from
Feb 26, 2021
Merged

[HttpKernel] Handle multi-attribute controller arguments #40307

merged 1 commit into from
Feb 26, 2021

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Feb 25, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? yes
Tickets -
License MIT
Doc PR todo

Currently, the ArgumentMetadata class used for controller argument value resolution can only hold one attribute per controller argument, while a method argument can take multiple attributes.

This allows accessing all attributes for a given argument, and deprecates the ArgumentInterface because it is not needed.
Spotted by @nicolas-grekas.

@chalasr chalasr force-pushed the arg-resolver-multi-attr branch from 05ba4cb to bbef5bd Compare February 25, 2021 16:18
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Feb 25, 2021
nicolas-grekas
nicolas-grekas approved these changes Feb 25, 2021
View reviewed changes
@nicolas-grekas
Copy link
Member

This PR improves on #37829
/cc @jvasseur

@chalasr chalasr force-pushed the arg-resolver-multi-attr branch from bbef5bd to 9331628 Compare February 25, 2021 16:47
@jvasseur
Copy link
Contributor

I though of allowing multiple attributes when originally designing it but couldn't see the use-case. I see this feature as a potential replacement for the ParamConvertor annotation and as a result the use case is to define a unique attribute that completely define how the argument will be resolved leaving no space for another attribute.

It's true that allowing multiple attributes doesn't limit the feature while allowing a potential new use case. The only things we will lose are:

  • No more error when defining multiple attributes : this could worsen the DX of the feature
  • Controllers attributes will always be instantiated even if not related to argument resolving as spotted by @nicolas-grekas (not sure how much of a problem this could be)

Overall I would say I am slightly in disfavor of this PR since I see no use case for it while it has a few drawbacks but that's not a strong opinion.

@jvasseur
Copy link
Contributor

BTW, this PR is missing the update to the UserValueResolver to no longer use the deprecated API.

@chalasr chalasr force-pushed the arg-resolver-multi-attr branch from 9331628 to cf03bcb Compare February 25, 2021 21:18
@chalasr chalasr requested a review from wouterj as a code owner February 25, 2021 21:18
@chalasr chalasr force-pushed the arg-resolver-multi-attr branch 2 times, most recently from 07c36bb to e55c7a8 Compare February 25, 2021 22:41
@chalasr
Copy link
Member Author

chalasr commented Feb 25, 2021 •
edited
Loading

Thank you for reviewing.

No more error when defining multiple attributes : this could worsen the DX of the feature

I can imagine use cases other than value resolution (e.g. decoration, introspection, validation...). Making the assumption that only one attribute will be needed feels wrong to me.

Controllers attributes will always be instantiated even if not related to argument resolving as spotted by @nicolas-grekas (not sure how much of a problem this could be)

Fixed via hasAttribute() and getAttributes() now returning attribute reflectors. now by ignoring attributes with undefined class.

@chalasr chalasr force-pushed the arg-resolver-multi-attr branch 3 times, most recently from f14e5c8 to de855a9 Compare February 25, 2021 23:01
@chalasr chalasr force-pushed the arg-resolver-multi-attr branch 2 times, most recently from c13d68b to 9c0b580 Compare February 25, 2021 23:38
nicolas-grekas
nicolas-grekas reviewed Feb 25, 2021
View reviewed changes
@chalasr chalasr force-pushed the arg-resolver-multi-attr branch from 9c0b580 to 921d638 Compare February 26, 2021 00:12
nicolas-grekas
nicolas-grekas approved these changes Feb 26, 2021
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Feb 26, 2021
View reviewed changes
@chalasr chalasr force-pushed the arg-resolver-multi-attr branch 4 times, most recently from 871adb3 to c2c54e6 Compare February 26, 2021 00:22
@chalasr chalasr force-pushed the arg-resolver-multi-attr branch from c2c54e6 to d771e44 Compare February 26, 2021 00:25
derrabus
derrabus reviewed Feb 26, 2021
View reviewed changes
src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
$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.

@derrabus
Copy link
Member

Thanks Robin for working on this feature, this is much appreciated.

@derrabus derrabus mentioned this pull request Feb 26, 2021
Closed
@derrabus derrabus merged commit f0e076a into symfony:5.x Feb 26, 2021
@chalasr chalasr deleted the arg-resolver-multi-attr branch February 27, 2021 01:15
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
chalasr referenced this pull request in symfony/http-kernel Jun 7, 2021
@chalasr
[HttpKernel] Handle multi-attribute controller arguments
d162d45
@jvasseur jvasseur mentioned this pull request Feb 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jvasseur jvasseur jvasseur left review comments

@derrabus derrabus derrabus approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Deprecation Feature HttpKernel Ready Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @nicolas-grekas @jvasseur @derrabus @carsonbot