-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle][HttpKernel] Let RequestPayloadValueResolver
consider mapped argument type
#57577
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
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.
Thanks! minor comments
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
Outdated
Show resolved
Hide resolved
To help others understand the context of this PR, it would be great if you update the title as a new proposal, rather than focusing on it as an issue. Also, a good description is very appreciated, as it will help document the feature later on. |
src/Symfony/Bundle/FrameworkBundle/Tests/Functional/ApiAttributesTest.php
Show resolved
Hide resolved
I think what you have achieved here is a better error message when an empty query string or an empty request payload is sent, and the targeted parameter is not nullable and doesn’t have a default value. In this case, the Serializer denormalizes the empty input as an empty object instead of just returning null, thus triggering the validation with a detailed error message. This is the critical code: class Controller
{
public function __invoke(#[MapQueryString] FooDto $foo) {}
} Before -> Exception without message |
@yceruto your last comment made me realize I may not achieved what I was hoping for. I have to clear out test cases and get back to you next week. |
Hi @yceruto, sorry for delay and possible confusion. I had to review test cases if they cover expected scenarios. |
a62833f
to
87c323f
Compare
@yceruto how do you guys deal with random test fails (8.4) or tests that fails only with one pipeline (8.2, low-deps)? |
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.
The symfony/framework-bundle
package currently requires "symfony/http-kernel": "^6.4|^7.0"
, which causes issues when the FWB runs tests with an older symfony/http-kernel
version (v6.4).
In these cases, we update the FWB composer.json
file to require the newest symfony/http-kernel
version instead, i.e. ^7.2
, so the CI will use that version in the low-deps
job.
However, I'm not sure it makes sense in this case where the only changes in FWB here are about tests. Actually, the FWB is still compatible with symfony/http-kernel: 6.4
. Let's ask @symfony/mergers to be sure.
src/Symfony/Bundle/FrameworkBundle/Tests/Functional/ApiAttributesTest.php
Show resolved
Hide resolved
64f4449
to
8a6c852
Compare
RequestPayloadValueResolver
consider mapped argument type
src/Symfony/Bundle/FrameworkBundle/Tests/Functional/ApiAttributesTest.php
Outdated
Show resolved
Hide resolved
…ion when empty request is sent resolves symfony#54617
RequestPayloadValueResolver
consider mapped argument typeRequestPayloadValueResolver
consider mapped argument type
Thank you @unixslayer. |
resolves #54617
In case of sending empty request,
RequestPayloadValueResolver
should consider argument type. In case empty request is considered invalid, having attribute as non-nullable without default value will cause to perform denormalization using empty input resulting with proper error message in response.