fix route generator not ignoring body param converters #1934
Conversation
|
I was not able to run the php-cs fixer. Somehow it fails on my machine :/ And there is no doc on how it should be executed. It would be cool to have a docker config for that to avoid this kind of situations ;) |
|
ping @GuilhemN |
tonivdv
force-pushed
the
fix/paramconverter
branch
from
266617d to
cde9656
Compare
|
Fixed the CS issue with the provided diff by styleci 👍 |
When fetching a list of function parameters from a controller action for use as URL parameters, check for the @ParamConverter annotation. If a parameter has the annotation with the "fos_rest.request_body" converter, then ignore it and do not include it in the list of parameters (thus removing it from the URL). This allows automatic route generation and ParamConverter to co-exist, as otherwise a parameter that is expected to be derived from the body would also be included as a URL parameter, requiring the developer to manually specify the route. fixes FriendsOfSymfony#1198
tonivdv
force-pushed
the
fix/paramconverter
branch
from
cde9656 to
d9e9d2e
Compare
| @@ -475,6 +475,17 @@ private function getMethodArguments(\ReflectionMethod $method) | |||
| // ignore all query params | |||
| $params = $this->paramReader->getParamsFromMethod($method); | |||
|
|
|||
| // check if a parameter is coming from the request body | |||
| $ignoreParameters = []; | |||
| if (class_exists('Sensio\\Bundle\\FrameworkExtraBundle\\Configuration\\ParamConverter')) { | |||
There was a problem hiding this comment.
let's use the class constant here
| $this->assertNotNull($collection->get('post_something'), 'route for "post_something" does not exist'); | ||
| $this->assertEquals('/somethings.{_format}', $collection->get('post_something')->getPath()); | ||
|
|
||
| $this->assertTrue(true); |
| $collection = $this->loadFromControllerFixture('ParamConverterController'); | ||
|
|
||
| $this->assertNotNull($collection->get('post_something'), 'route for "post_something" does not exist'); | ||
| $this->assertEquals('/somethings.{_format}', $collection->get('post_something')->getPath()); |
There was a problem hiding this comment.
Ok. Just copied from previous tests.
| @@ -475,6 +475,17 @@ private function getMethodArguments(\ReflectionMethod $method) | |||
| // ignore all query params | |||
| $params = $this->paramReader->getParamsFromMethod($method); | |||
|
|
|||
| // check if a parameter is coming from the request body | |||
| $ignoreParameters = []; | |||
| if (class_exists(\Sensio\Bundle\FrameworkExtraBundle\Configuration\ParamConverter::class)) { | |||
There was a problem hiding this comment.
but please also add use case statements for the classes :)
There was a problem hiding this comment.
Not sure I understand what you mean, can you elaborate.
There was a problem hiding this comment.
Alright, I prefer that too, but since a lot in the code is with FQN I thought it was some standard or something. Will do that simplification.
There was a problem hiding this comment.
I changed all FQN in that method so it's cleaned up at the same time.
| @@ -500,6 +511,10 @@ private function getMethodArguments(\ReflectionMethod $method) | |||
| } | |||
| } | |||
|
|
|||
| if (in_array($argument->getName(), $ignoreParameters)) { | |||
There was a problem hiding this comment.
we should also pass true as the third argument
| \FOS\RestBundle\Request\ParamFetcherInterface::class, | ||
| \Symfony\Component\Validator\ConstraintViolationListInterface::class, | ||
| \Sensio\Bundle\FrameworkExtraBundle\Configuration\ParamConverter::class, | ||
| Request::class, |
| */ | ||
| private function getMethodArguments(\ReflectionMethod $method) | ||
| private function getMethodArguments(ReflectionMethod $method) |
There was a problem hiding this comment.
In fact, for classes from the global namespace we follow the same approach as the Symfony code style and keep the namespaced variant. Sorry for the confusion.
There was a problem hiding this comment.
Ah lol. Alright no problem.
|
Thank you @tonivdv for taking care of this! |
|
We usually release when there are a significant number of bug fixes/features awaiting (see 2.3.1...master). I guess we can release 2.4 though, important fixes are awaiting right now. Is it ok for you @xabbuh? |
|
👍 let's go for it |
This is an update on PR #1771 with the missing test case. All credits should go to @Parent5446
When fetching a list of function parameters from a controller action
for use as URL parameters, check for the @ParamConverter
annotation. If a parameter has the annotation with the
"fos_rest.request_body" converter, then ignore it and do not include
it in the list of parameters (thus removing it from the URL).
This allows automatic route generation and ParamConverter to co-exist,
as otherwise a parameter that is expected to be derived from the body
would also be included as a URL parameter, requiring the developer to
manually specify the route.
fixes #1198