[HttpFoundation] use InputBag for Request::$request only if data is coming from a form #37265
Conversation
nicolas-grekas
commented
Jun 13, 2020
nicolas-grekas
commented
| Q | A |
|---|---|
| Branch? | 5.1 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #37100 |
| License | MIT |
| Doc PR | - |
|
Wont this make static analysis go crazy.. ??? |
|
@gmponos you made me realize I missed updating the |
|
Always positive :) at least something good came out from my comment.. check my comment on the related issue Also I tried your changes locally at a project of mine and it seems that PHPStan is not complaining. |
nicolas-grekas
force-pushed
the
hf-req
branch
4 times, most recently
from
86a239a
to
25769b2
Compare
…oming from a form
|
Thank you @nicolas-grekas. |
| @@ -298,7 +298,9 @@ public static function createFromGlobals() | |||
| { | |||
| $request = self::createRequestFromFactory($_GET, $_POST, [], $_COOKIE, $_FILES, $_SERVER); | |||
|
|
|||
| if (0 === strpos($request->headers->get('CONTENT_TYPE'), 'application/x-www-form-urlencoded') | |||
| if ($_POST) { | |||
| $request->request = new InputBag($_POST); | |||
There was a problem hiding this comment.
Most of the developers using symfony that will create a form using form component, let's say a login form, will get the data back as
`login` => [
`username` => `user`,
`password` => `password`,
];
So I don't see this InputBag here serving the majority..
There was a problem hiding this comment.
The form component is already strict about strings vs arrays in values and knows how to deal with InputBag.
There was a problem hiding this comment.
Yes the form component, as a component knows how to deal with the InputBag.. my point is that users that will build a form with the component... they will never use this:
$request->request->get('login');
What use cases/bugs are you trying to cover by using for the request attribute the InputBag?
This means many apps just fail with a 500 when adding [] in the query string.
All this started with the scenario that someone access a query parameter and thinking that it is string etc etc.. ok.. although I disagree with the notion of it.. it is a UseCase. I don't see how developers will be protected by using InputBag on the request since most of the cases are submitting associative arrays...
All I see is that I have to do this:
$login = $request->request->get('login');
return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);to:
$request = $request->request->all();
if (!isset($request['login'])) {
return false;
}
$login = $request['login'];
return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);it's not the code that I have to write.. it's that ParameterBag::get is totally useless..
Hope I am explaining in the best way.. if not.. maybe we should move on.. there might be more important things to care..
There was a problem hiding this comment.
Your "to" should be:
$request = $request->request->all('login');
return isset($login['username']) && isset($login['password']) && isset($login['_token']);Then you'll let a BadRequestException bubble down and become a 400 as it should if you expect login to be an array. This "after" is much better than the "before" actually, with a richer behavior and more semantic code.
There was a problem hiding this comment.
Then you'll let a BadRequestException bubble down and become a 400 as it should if you expect login to be an array.
I can't.. various technical reasons that are out of the topic of this discussion...
Still this remains:
it's not the code that I have to write.. it's that ParameterBag::get is totally useless..
| } | ||
|
|
||
| return $value; | ||
| return parent::all($key); |
There was a problem hiding this comment.
I see that you tried making the behavior between the two bags more similar.. nice.. not sure if you were driven by my comment... but this is indeed nice..
There was a problem hiding this comment.
I don't remember exactly my flow of thoughts, but it might be :)
|
Hi @nicolas-grekas , after merging #37327 should this be reverted? The InputBag|ParameterBag hint is making our phpstan analysis a bit useless due to the Not sure if I should open a PR, happy to do it, thx! |
|
I think using InputBag for accessing form payloads is the correct to provide type-safety at runtime. |
|
Possibly yes. InputBag is more restrictive than ParameterBag, but maybe that's still fine in practice even when the content is json. Feel free to give it a try :) |
…sademont) This PR was merged into the 5.3-dev branch. Discussion ---------- [HttpFoundation] Use InputBag for POST requests too | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Partially revert #37265 | License | MIT | Doc PR | - #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 only `InputBag` as the `$request` bag we can tighten the typehint again and make static analysis a bit more useful. Commits ------- 381a0a1 use InputBag for POST requests too, added missing scalar type hints
