Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[HttpKernel] Add#[MapQueryParameter] to map and validate individual query parameters to controller arguments#49134

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

ruudk
Copy link
Contributor

@ruudkruudk commentedJan 27, 2023
edited by nicolas-grekas
Loading

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PR

We increased the PHPStan level from 8 to 9. This lead to some problems when working with query or request parameters.

For example:

$firstName =$request->get('firstName');

Because Symfony typesRequest::get() asmixed there is no type safety and you have to assert everything manually.

We then learned that we shouldn't useRequest::get() but use the explicit parameter bag like:

$request->query->get('firstName');

ThisParameterBag is used forrequest andquery. It contains interesting methods like:

$request->query->getAlpha('firstName') : string;$request->query->getInt('age') : int;

This has the benefit that now the returned value is of a correct type.

Why aren't we being explicit by requiring the parameters and their types in the controller action instead?

Luckily Symfony has a concept calledValueResolver.

It allows you to do dynamically alter what is injected into a controller action.

So in this PR, we introduces a new attribute:#[MapQueryParameter] that can be used in controller arguments.

It allows you to define which parameters your controller is using and which type they should be. For example:

#[Route(path:'/', name:'admin_dashboard')]publicfunction indexAction(  #[MapQueryParameter]array$ids,  #[MapQueryParameter]string$firstName,  #[MapQueryParameter]bool$required,  #[MapQueryParameter]int$age,  #[MapQueryParameter]string$category ='',  #[MapQueryParameter]  ?string$theme =null,)

When requesting/?ids[]=1&ids[]=2&firstName=Ruud&required=3&age=123 you'll get:

$ids = ['1', '2']$firstName = "Ruud"$required = false$age = 123$category = ''$theme = null

It even supports variadic arguments like this:

#[Route(path:'/', name:'admin_dashboard')]publicfunction indexAction(  #[MapQueryParameter]string ...$ids,)

When requesting/?ids[]=111&ids[]=222 the$ids argument will have an array with values ['111','222'].

Unit testing the controller now also becomes a bit easier, as you only have to pass the required parameters instead of constructing theRequest object.

It's possible to useFILTER_VALIDATE_* consts for more precise type descriptions.

For example, this ensures that$ids is an array of integers:

#[MapQueryParameter(filter: \FILTER_VALIDATE_INT)]array$ids

And this declares a string that must match a regexp:

#[MapQueryParameter(filter: \FILTER_VALIDATE_REGEXP, options: ['regexp' =>'/^\w++$/'])]string$name

When a filter fails, a 404 is returned.

alamirault, alexislefebvre, derrabus, sheeep, rvanlaak, qdequippe, webda2l, Romaixn, wkania, korolev-d-l, and juuuuuu reacted with thumbs up emojigabor-jonas-lnrs and mbuliard reacted with heart emojiwillemverspyck, derrabus, rvanlaak, and gabor-jonas-lnrs reacted with rocket emoji
@carsonbotcarsonbot added this to the6.3 milestoneJan 27, 2023
@ruudkruudk changed the titleAllow injecting query and request parameters in controllers by typing them with#[QueryParameter] or#[RequestParameter] attribute[HttpKernel] Allow injecting query and request parameters in controllers by typing them with#[QueryParameter] or#[RequestParameter] attributeJan 27, 2023
@ruudkruudkforce-pushed thequery-request-value-resolver branch 3 times, most recently from427874e to878ddd5CompareJanuary 27, 2023 15:42
@alamirault
Copy link
Contributor

A like the idea, deporting some basic checks and use type hint

@ruudk
Copy link
ContributorAuthor

@carsonbot what's needed to get some reviews ?

@yceruto
Copy link
Member

I guess this one is competing with a similar proposal#49138, right?

@ruudk
Copy link
ContributorAuthor

I think this is a very different approach and not similar. This provides scalar primitives and the use case is to allow some query parameters in the controller action.

While the other PR tries to deserialize request content into typed objects.

@y4roc
Copy link

@ruudk the other PR have two attributes. One for the Query and one for the Content.

@ruudk
Copy link
ContributorAuthor

I see but it serializes to an object, while this PR serializes a query string parameter to a typed property, for examplestring.

And for the RequestParameter is used the form data (equivalent to request->request->get).

If the other PR can also support these cases then this is a obsolete indeed. But AFAIK the other works on request content (json/xml)?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I likeMapQueryParameter but I don't see the use case for MapRequestParameter.

@ruudkruudkforce-pushed thequery-request-value-resolver branch from878ddd5 toa57bf66CompareFebruary 22, 2023 14:13
@ruudkruudk changed the title[HttpKernel] Allow injecting query and request parameters in controllers by typing them with#[QueryParameter] or#[RequestParameter] attribute[HttpKernel] Allow injecting query parameters in controllers by typing them with#[MapQueryParameter] attributeFeb 22, 2023
@ruudkruudkforce-pushed thequery-request-value-resolver branch froma57bf66 to161d283CompareFebruary 22, 2023 14:16
@ruudk
Copy link
ContributorAuthor

@nicolas-grekas Updated the PR. RenamedQueryParameter toMapQueryParameter. Removed the support for request parameters.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Like that yes, but with a try/catch to generate 404
This made me realize: what about adding support for simple filtering rules?
https://www.php.net/manual/en/filter.filters.validate.php

@ruudkruudk requested review fromstof andnicolas-grekas and removed request forstofFebruary 22, 2023 18:43
@ruudk
Copy link
ContributorAuthor

@nicolas-grekas@stof Thanks for your feedback. Applied the changes. I think it's way better now.

@ruudkruudk requested review fromstof and removed request fornicolas-grekas andstofFebruary 22, 2023 18:45
@derrabus
Copy link
Member

?id=123456 when there is no such entity with id 123456 also leads to a 404, not a 400

I don't know if you've read my comment fully because you've picked exactly the case where I agree with you and ignored the rest of it.

@nicolas-grekas
Copy link
Member

I read it, I just don't make any difference between both cases, because there aren't any, from an HTTP spec perspective.

@derrabus
Copy link
Member

@ro0NL Looking athttps://httpwg.org/specs/rfc9110.html#status.422 and the example given there, 422 indeed does not look like a good fix for a malformed URL. The spec clearly talks about the request content, not the resource URL.

@ro0NL
Copy link
Contributor

i stand corrected :) i wouldnt dare betting on 422 v 400 v 404 anymore from a spec POV, i can only imagine it helps API consumers

it looks like we found an RFC that calls it "request content" explicitly 👍

@ro0NL
Copy link
Contributor

About allowing validation constraints, we could see how far this can go. In a separate RFC/PR indeed.

See#48525 for type safety!

im not really convinced adding the attribute without constraint validation adds much value then

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

note$request->query->get() throws 400, here it's 404

Copy link
Contributor

@soyukasoyuka left a comment
edited
Loading

Choose a reason for hiding this comment

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

Will it be possible to introduce a name converter afterwards? This seems very opinionated regarding query parameters naming and conventions (they're nonexistent anyways). Also,query[key]=value doesn't seem to be tested yet.

@nicolas-grekas
Copy link
Member

Will it be possible to introduce a name converter afterwards? This seems very opinionated regarding query parameters naming and conventions (they're nonexistent anyways).

The names can already be remapped if needed, see argument $name of the attribute. Or did you mean something else?

having validation be part of the controller is like merging the model and the controller on an MVC point of view.

This is not about validation but about the last step of the routing logic. Thus the 404 when the input doesn't match.

nicolas-grekas added a commit that referenced this pull requestApr 14, 2023
…and `#[MapQueryString]` to map Request input to typed objects (Koc)This PR was merged into the 6.3 branch.Discussion----------[HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects| Q             | A| ------------- | ---| Branch?       | 6.3| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       |#36037,#36093,#45628,#47425,#49002,#49134| License       | MIT| Doc PR        | TBDYet another variation of how we can map raw Request data to typed objects with validation. We can even build OpenApi Specification based on this DTO classes using [NelmioApiDocBundle](https://github.com/nelmio/NelmioApiDocBundle).## Usage Example 🔧### `#[MapRequestPayload]````phpclass PostProductReviewPayload{    public function __construct(        #[Assert\NotBlank]        #[Assert\Length(min: 10, max: 500)]        public readonly string $comment,        #[Assert\GreaterThanOrEqual(1)]        #[Assert\LessThanOrEqual(5)]        public readonly int $rating,    ) {    }}class PostJsonApiController{    public function __invoke(        #[MapRequestPayload] PostProductReviewPayload $payload,    ): Response {        // $payload is validated and fully typed representation of the request body $request->getContent()        //  or $request->request->all()    }}```### `#[MapQueryString]````phpclass GetOrdersQuery{    public function __construct(        #[Assert\Valid]        public readonly ?GetOrdersFilter $filter,        #[Assert\LessThanOrEqual(500)]        public readonly int $limit = 25,        #[Assert\LessThanOrEqual(10_000)]        public readonly int $offset = 0,    ) {    }}class GetOrdersFilter{    public function __construct(        #[Assert\Choice(['placed', 'shipped', 'delivered'])]        public readonly ?string $status,        public readonly ?float $total,    ) {    }}class GetApiController{    public function __invoke(        #[MapQueryString] GetOrdersQuery $query,    ): Response {        // $query is validated and fully typed representation of the query string $request->query->all()    }}```### Exception handling 💥- Validation errors will result in an HTTP 422 response, accompanied by a serialized `ConstraintViolationList`.- Malformed data will be responded to with an HTTP 400 error.- Unsupported deserialization formats will be responded to with an HTTP 415 error.## Comparison to another implementations 📑Differences to#49002:- separate Attributes for explicit definition of the used source- no need to define which class use to map because Attributes already associated with typed argument- used ArgumentValueResolver mechanism instead of Subscribers- functional testsDifferences to#49134:- it is possible to map whole query string to object, not per parameter- there is validation of the mapped object- supports both `$request->getContent()` and `$request->request->all()` mapping- functional testsDifferences to#45628:- separate Attributes for explicit definition of the used source- supports `$request->request->all()` and `$request->query->all()` mapping- Exception handling opt-in- functional tests## Bonus part 🎁- Extracted `UnsupportedFormatException` which thrown when there is no decoder for a given formatCommits-------d987093 [HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects
@OskarStark
Copy link
Contributor

@nicolas-grekas
Copy link
Member

Why close? This PR is complementary, not "instead of".

OskarStark and ruudk reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas changed the title[HttpKernel] Add#[MapQueryParameter] to map query parameters to controller arguments[HttpKernel] Add#[MapQueryParameter] to map and validate individual query parameters to controller argumentsApr 14, 2023
@nicolas-grekasnicolas-grekasforce-pushed thequery-request-value-resolver branch fromfa71093 tobd7c669CompareApril 14, 2023 11:29
@ruudk
Copy link
ContributorAuthor

@nicolas-grekas Can we merge this PR?

@nicolas-grekas
Copy link
Member

Thank you@ruudk.

@dotdevio
Copy link

Just imho but #[QueryParameter] would be much readable because what else can this attribute can do except a map attribute to method property, "Map" prefix is just superfluous. I mean no else framework as i know uses Map prefix for such attributes and the attribute itself is also responsible for validation whatever... Like in sesnioextrabundle #[ParamConverter] is not #[MapParamConverter] just beacuse we map this attribute to property it's so clear. Also it's just looks so terrible in code with "Map" prefix. I understand you may say this will help with autocomplete so all such attributes will be grouped with "Map" but other frameworks are fine without map prefix as well as sensio extra bundle we are fine without prefixes cause there is just few attributes which devs are need to learn we are not so lazy)))

soyuka and juuuuuu reacted with thumbs up emojiruudk and b1rdex reacted with thumbs down emojiruudk reacted with confused emoji

nicolas-grekas added a commit that referenced this pull requestAug 1, 2023
…or custom http status code (zim32)This PR was merged into the 6.4 branch.Discussion----------[HttpKernel] RequestPayloadValueResolver Add support for custom http status code| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | no| License       | MIT| Doc PR        | Will do if acceptedReading this discussion#49134 (comment) it looks like a good idea to have ability to customize http status code in case of a validation error.This MR does not create any BC's.**MapQueryString** and **MapRequestPayload** attributes now have additional parameter `validationFailedStatusCode`, which allows to specify which http status code will be used if validation fails.Commits-------4d118c0 [HttpKernel] RequestPayloadValueResolver Add support for custom http status code
@smilesrg
Copy link
Contributor

If some length validation fails, why the response status code should be 404?

Janczur reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@KocKocKoc left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof left review comments

@ro0NLro0NLro0NL left review comments

@soyukasoyukasoyuka left review comments

@94noni94noni94noni left review comments

@MatTheCatMatTheCatMatTheCat left review comments

@GromNaNGromNaNGromNaN approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

17 participants
@ruudk@alamirault@yceruto@y4roc@nicolas-grekas@ro0NL@derrabus@OskarStark@dotdevio@smilesrg@Koc@GromNaN@stof@soyuka@94noni@MatTheCat@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp