Fix jsonRequest in AbstractBrowser for GET methods
#39281
Conversation
cc1747f to
4d90a87
Compare
4d90a87 to
589d06f
Compare
|
@nicolas-grekas anything missing here from my side? |
| break; | ||
| default: | ||
| $content = null; | ||
| $query = $parameters; |
There was a problem hiding this comment.
wait, I'm not sure anymore that this makes sense: $parameters doesn't go through the processing of json_encode().
That means objects (that do or don't implement JsonSerializable) are going to be passed differently when using GET vs other verbs.
There was a problem hiding this comment.
Yeah based on the method we set it a request body or not. This is based on the linked logic which symfony currently use here: https://github.com/symfony/symfony/blob/v5.2.0/src/Symfony/Component/HttpFoundation/Request.php#L388-L404
It does not make sense to convert query parameters to JSON as nobody will converted it back. Which is in the FosRestBundle and ApiPlatform converted into the $request->request attributes by the BodyListener.
|
@nicolas-grekas any update to this I think this still should match the same logic as the http foundation |
AbstractBrowser for GET methods
|
@nicolas-grekas I still think this change is valid because of: symfony/src/Symfony/Component/HttpFoundation/Request.php Lines 375 to 391 in d303433 And because of GET, HEAD, OPTION parameters are query strings and the content has no meaning: https://stackoverflow.com/questions/978061/http-get-with-request-body But think this will not being merged as it is now open a long time? |
AbstractBrowser for GET methodsAbstractBrowser for GET methods
|
I think nobody understands the change :) |
When implementing #38596 I was not aware of that $parameters are also used as queryParameters and are converted based on the Method here into query or request data. This I think should be handled the same way for the jsonRequest also instead of just unset the server variables we should to reset to the predefined server variables.