[HttpKernel] RequestPayloadValueResolver Add support for custom http status code #50767
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 |
|
Please modify tests to check it's a status code from the attribute that is thrown. |
Two tests added, for query and request payload. |
...y/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php
Outdated
Show resolved
Hide resolved
| } elseif ($argument instanceof MapRequestPayload) { | ||
| $payloadMapper = 'mapRequestPayload'; | ||
| $validationFailedCode = Response::HTTP_UNPROCESSABLE_ENTITY; | ||
| $validationFailedCode = $argument->validationFailedStatusCode; |
There was a problem hiding this comment.
I think it would be great to be able to define another error by default (from the constructor of this class). Otherwise it means that for every attribute we would have to do validationFailedStatusCode: Response::HTTP_SOMETHING.
There was a problem hiding this comment.
Agree. But maybe it will be better to do this by another MR?
There was a problem hiding this comment.
Don't see why that can't be added to this PR? Seems easy enough to add two constructor arguments and rely on those for the default value. This means in the attribute those arguments should be nullable and null by default. This way we can override it with the argument on the attribute but don't rely on the attribute for the default value of those arguments.
If you want, I can help with that?
There was a problem hiding this comment.
Well this is another feature amd it is always better to separate features into several MR otherwise MR can become quite messy
There was a problem hiding this comment.
I'm not sure this is a good idea. Descriptive info (attributes here) should be self-descriptive. Making them rely on some external configuration blurs the line and readers can't expect a stable outcome anymore.
The solution to your concern is to create and use a child class that sets the code to what you want.
...y/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php
Outdated
Show resolved
Hide resolved
daff0e8 to
45ce17a
Compare
45ce17a to
4d118c0
Compare
|
Thank you @zim32. |
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.