-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpFoundation] Use InputBag for POST requests too #40315
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
PS: Also not sure if this should be considered a bugfix and merged into 5.2.x instead of 5.x |
Thinking a bit more about this, there might still be an issue when using JSON payloads: the |
@nicolas-grekas that's only because of the phpdoc typehint, there's nothing preventing an |
Can you update the type hints so that we can discuss the full change that would be needed? |
(and add related tests btw) |
06bb7e3
to
74bdd51
Compare
74bdd51
to
381a0a1
Compare
Added the missing scalar type hints and some testing |
$this->assertNull($bag->get('null', 'default'), '->get() returns null if null is set'); | ||
$this->assertSame(1, $bag->get('int'), '->get() gets the value of an int parameter'); | ||
$this->assertSame(1.0, $bag->get('float'), '->get() gets the value of a float parameter'); | ||
$this->assertFalse($bag->get('bool'), '->get() gets the value of a bool parameter'); |
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.
perhaps assert eg. $default=true is never casted to string
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.
Sure, sorry I didn't see your comment!
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.
LGTM thanks.
@ro0NL's suggestion is a good one, can you update the PR if you agree?
Thank you @acasademont. |
Ah! Thx @fabpot ! I was planning to update the tests this weekend, I'll submit another PR for them. |
Why are arrays not allowed anymore ? This means if you got an form with checkboxes it's not allowed anymore. Edit: @acasademont @fabpot Why is the set accepting arrays but the get not could this be fixed asap please? |
|
#37265 was created as a fix for #37100. However, when #37327 was merged, the original bug was also fixed with a different solution (allowing null values on
InputBag::set
) and parts of #37265 are not needed anymore. By using onlyInputBag
as the$request
bag we can tighten the typehint again and make static analysis a bit more useful.