[HttpKernel] FromQuery, FromBody, FromRoute, etc. attributes #58709

klkvsk · 2024-10-29T20:31:24Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	#58662
License	MIT

This is an attempt to separate how and from where the controller arguments are resolved.

Proposed new attributes #[FromQuery] #[FromBody] #[FromRoute] and #[FromHeader] (ValueBagResolvers) can be used in conjunction with existing #[MapDateTime] or #[MapEntity].

Most ValueResolvers that use data from request currently use only $request->attributes bag. They are easily converted to work with another bag specified by attribute by using ValueBagResolverTrait. BC is kept: when no argument attributes specified, $request->attributes is used.

It can deprecate #[MapQueryParameter].

It may deprecate #[MapQueryString] and #[MapRequestPayload] in the future, but it will need a something like #[MapDto] to replace them.

Currently I'm not quite done with handling variadic parameters. I want BackedEnum, Uid, and DateTime values to be supported, but I don't feel like their respective ValueResovlers should handle both variadic and non-variadic logic. On the other side, VariadicValueResolver has no ability to subsequently resolve values.

Feedback much appreciated.

Example:

#[Route("/foo/list", methods: ['GET'])]
public function listFoo(
    #[FromQuery] 
    int $page = 1,
    #[FromQuery('sort')] 
    string $sortingField = 'id',
    #[FromQuery('asc')] 
    bool $sortingAsc = true,
) {}

#[Route('/foo/{fooId}/status', methods: ['POST'])]
public function updateFoo(
    #[MapEntity(Foo::class, id: 'fooId')] // 
    Foo $foo,                             // 
                                          // -- same as --
    #[FromRoute('fooId')]                 // 
    #[MapEntity(Foo::class)]              // 
    Foo $foo,                             // 
    
    #[FromBody]
    FooStatus $status, // enum
) {}

derrabus · 2024-10-30T10:01:08Z

Thank you. Please apply the patch from fabbot to fix all code-style issues.

derrabus · 2024-10-30T10:06:46Z

src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

@@ -31,6 +33,8 @@
 */
 final class EntityValueResolver implements ValueResolverInterface
 {
+    use ValueBagResolverTrait;


This change will render the Doctrine Bridge incompatible with older versions of the HttpKernel component. That's fine, but we need to tell Composer about that.

Please adjust this conflict rule:

symfony/src/Symfony/Bridge/Doctrine/composer.json

Line 61 in 9b0ca99

"symfony/http-kernel": "<6.4",

Since you're currently targeting 7.2 for this change, declaring incompatibility with versions before 7.2 would solve the problem.

derrabus · 2024-10-30T10:12:03Z

src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

@@ -122,7 +128,7 @@ private function find(ObjectManager $manager, Request $request, MapEntity $optio
        }
    }

-    private function getIdentifier(Request $request, MapEntity $options, ArgumentMetadata $argument): mixed
+    private function getIdentifier(Request $request, MapEntity $options, ArgumentMetadata $argument, ParameterBag $valueBag): mixed


As far as I can tell, the $valueBag parameter has effectively replaced the $request in most private methods you've touched here. Let's remove the now-unused parameter where we can.

Suggested change

private function getIdentifier(Request $request, MapEntity $options, ArgumentMetadata $argument, ParameterBag $valueBag): mixed

private function getIdentifier(ParameterBag $valueBag, MapEntity $options, ArgumentMetadata $argument): mixed

derrabus · 2024-10-30T10:19:06Z

src/Symfony/Component/HttpKernel/Tests/Controller/ValueBagResolverTraitTest.php

+    public function testVariadicEnum()
+    {
+        // TODO
+    }
+
+    public function testVariadicUid()
+    {
+        // TODO
+    }
+
+    public function testVariadicDateTime()
+    {
+        // TODO
+    }


yceruto · 2024-10-30T13:46:12Z

I'd prefer FromPath instead of FromRoute.

klkvsk · 2024-11-11T17:02:26Z

Thank you all for comments.

@ro0NL

#[MapEntity(Foo::class, id: new MapQueryParameter('fooId'))]

General idea is to decouple #[From..] attributes from #[Map...] attributes, so in order to do it, I needed to make ValueResolvers for Entity, DateTime, Uid and other unaware of any #[From..] attributes. That is why they just receive a $valueBag ready for them to consume values from.

In latest commit I addressed the compatibility with variadic parameters. Now any ValueResolver work with variadic and non-variadic parameters the same way. They just being called resolveValue() multiple times if it's variadic.

Due to this, we no longer need VariadicValueResolver. It is deprecated.

Also, MapQueryParameter is deprecated, as FromQuery does the same thing, but more flexible, i.e. can be combined with mappers such as MapDateTime.

I think this PR is finalized in my point of view. There are couple things I'm not still very happy about:

had to keep RequestAttributeValueResolver named this way for a better BC (in case someone is disabling it with #[ValueResolver(RequestAttributeValueResolver::class, disabled: true)], but it should better be called RequestParameterValueResolver or SimpleValueResolver or something. Basically, it resolves any values not converted with higher-priority ValueResolvers, so mostly built-ins.
the EntityValueResolver is the only thing that required passing request to resolveValue due to it's use in expression. Otherwise, the resolveValue method signature would be cleaner without $request.

The thing I am happy about is that any custom ValueResolver can be converted with just a few lines of code.

…st parameter bag to use

…verTraitTest

carsonbot added Status: Needs Review Feature HttpKernel labels Oct 29, 2024

carsonbot added this to the 7.2 milestone Oct 29, 2024

klkvsk mentioned this pull request Oct 29, 2024

#[Map...] is a mess (rfc) #58662

Open

derrabus reviewed Oct 30, 2024

View reviewed changes

Koc mentioned this pull request Nov 10, 2024

[HttpKernel] Create #[Serialize] Attribute to serialize Controller Result #49518

Open

1 task

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

klkvsk added 6 commits December 3, 2024 22:52

add controller argument attributes to hint ValueResolvers which reque…

c4a709b

…st parameter bag to use

add ValueBagResolverTrait support to EntityValueResolver

65cc9ab

add tests for MapEntity with expression when using ValueBagResolver

f2825e7

refactor request parameter resolvers using RequestParameterValueResol…

3ea5403

…verTraitTest

apply fabbot patch, fix psalm notices and ci tests

c30df5c

target symfony 7.3

9e5b4ff

klkvsk force-pushed the request-value-from branch from ef7e5e4 to 9e5b4ff Compare December 3, 2024 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] FromQuery, FromBody, FromRoute, etc. attributes #58709

[HttpKernel] FromQuery, FromBody, FromRoute, etc. attributes #58709

klkvsk commented Oct 29, 2024 •

edited by OskarStark

Loading

derrabus commented Oct 30, 2024

derrabus Oct 30, 2024

derrabus Oct 30, 2024

derrabus Oct 30, 2024

yceruto commented Oct 30, 2024

klkvsk commented Nov 11, 2024

	private function getIdentifier(Request $request, MapEntity $options, ArgumentMetadata $argument, ParameterBag $valueBag): mixed
	private function getIdentifier(ParameterBag $valueBag, MapEntity $options, ArgumentMetadata $argument): mixed

[HttpKernel] FromQuery, FromBody, FromRoute, etc. attributes #58709

Are you sure you want to change the base?

[HttpKernel] FromQuery, FromBody, FromRoute, etc. attributes #58709

Conversation

klkvsk commented Oct 29, 2024 • edited by OskarStark Loading

derrabus commented Oct 30, 2024

derrabus Oct 30, 2024

Choose a reason for hiding this comment

derrabus Oct 30, 2024

Choose a reason for hiding this comment

derrabus Oct 30, 2024

Choose a reason for hiding this comment

yceruto commented Oct 30, 2024

klkvsk commented Nov 11, 2024

klkvsk commented Oct 29, 2024 •

edited by OskarStark

Loading