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

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 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 2 commits into from
Apr 17, 2023

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jan 27, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
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 types Request::get() as mixed there is no type safety and you have to assert everything manually.

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

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

This ParameterBag is used for request and query. 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 called ValueResolver.

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')]
public function 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')]
public function 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 the Request object.

It's possible to use FILTER_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.

@carsonbot carsonbot added this to the 6.3 milestone Jan 27, 2023
@ruudk ruudk changed the title Allow 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] attribute Jan 27, 2023
@ruudk ruudk force-pushed the query-request-value-resolver branch 3 times, most recently from 427874e to 878ddd5 Compare January 27, 2023 15:42
@alamirault
Copy link
Contributor

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

@ruudk
Copy link
Contributor Author

ruudk commented Feb 2, 2023

@carsonbot what's needed to get some reviews ?

@yceruto
Copy link
Member

yceruto commented Feb 3, 2023

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

@ruudk
Copy link
Contributor Author

ruudk commented Feb 3, 2023

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

y4roc commented Feb 3, 2023

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

@ruudk
Copy link
Contributor Author

ruudk commented Feb 3, 2023

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

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-grekas nicolas-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 like MapQueryParameter but I don't see the use case for MapRequestParameter.

@ruudk ruudk force-pushed the query-request-value-resolver branch from 878ddd5 to a57bf66 Compare February 22, 2023 14:13
@ruudk ruudk 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] attribute Feb 22, 2023
@ruudk ruudk force-pushed the query-request-value-resolver branch from a57bf66 to 161d283 Compare February 22, 2023 14:16
@ruudk
Copy link
Contributor Author

ruudk commented Feb 22, 2023

@nicolas-grekas Updated the PR. Renamed QueryParameter to MapQueryParameter. Removed the support for request parameters.

@ruudk ruudk requested a review from nicolas-grekas February 22, 2023 14:21
Copy link
Member

@nicolas-grekas nicolas-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

@ruudk ruudk requested review from stof and nicolas-grekas and removed request for stof February 22, 2023 18:43
@ruudk
Copy link
Contributor Author

ruudk commented Feb 22, 2023

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

@ruudk ruudk requested review from stof and removed request for nicolas-grekas and stof February 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 at https://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

ro0NL commented Mar 20, 2023

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

ro0NL commented Mar 21, 2023

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

@ro0NL ro0NL 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

@soyuka soyuka left a comment

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 request Apr 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        | TBD

Yet 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]`

```php
class 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]`

```php
class 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 tests

Differences 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 tests

Differences 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 format

Commits
-------

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".

@nicolas-grekas nicolas-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 arguments Apr 14, 2023
@nicolas-grekas nicolas-grekas force-pushed the query-request-value-resolver branch from fa71093 to bd7c669 Compare April 14, 2023 11:29
@ruudk
Copy link
Contributor Author

ruudk commented Apr 17, 2023

@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)))

nicolas-grekas added a commit that referenced this pull request Aug 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 accepted

Reading 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Morty Proxy This is a proxified and sanitized view of the page, visit original site.