-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] [WIP] Validate controller arguments using constraints #47425
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
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
Hey! I think @alexandre-daubois has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Hi @artyuum I just try it out! and it's working great! thanks for your work! 😁 |
$violations = $this->validator->validate($value, $constraint); | ||
|
||
if ($violations->count() > 0) { | ||
throw new ValidationFailedException($value, $violations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm throwing ValidationFailedException
if the validation failed, but I'm not sure that this is the right exception to throw. Should I create a new one instead?
return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 10]]; | ||
} | ||
|
||
private function getReflectionMethod(callable $controller): \ReflectionMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how I'm extracting the controller class + method from the event.
If you have a better way I'll take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the new ControllerEvent::getControllerReflector()
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method seems to be available only in ControllerEvent
and I'm listening on ControllerArgumentsEvent
😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's worth it, it could be easy to expose:
class ControllerArgumentsEvent extends KernelEvent
{
// ...
public function getControllerReflector(): \ReflectionFunctionAbstract
{
return $this->controllerEvent->getControllerReflector();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be useful, yes. Should I create another PR to make that change and merge it into the current PR or can I change that method scope in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @nicolas-grekas
Why it is like this, I would like use the PositiveOrZero on the arguments directly. #[Route(name: 'all', methods: ['GET'])]
public function all(#[PositiveOrZero] int $offset = 0, #[Positive] int $limit = 20): Response
{
...
} Controller argument binding is another topic, such as #[Route(path: '{id}', methods: ['GET'])]
public function getById(
#[PathParam(name="id", defaultValue="xxx")] int blogId, // example: the method argument is different from the path variables.
#[RequestParam(name="offset", defaultValue="0")] #[PositiveOrZero] int $offset = 0,
#[RequestParam(name="limit", defaultValue="50")] #[Positive] int $limit = 20): Response
{
...
} |
@hantsy the reason has been discussed here #47425 (comment) |
Sadly see this decision. |
…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
Can we close this PR? was merged right now. |
Yes, I don't think it's necessary to finish this PR. |
As discussed in #43958, the purpose of this PR is to add a new feature to the Validator component to allow the validation of controller arguments through constraints.
Example:
Todo