Rework PR

Describer/ParameterRefMergeDescriber.php

@@ -1,79 +0,0 @@
-<?php
-
-/*
- * This file is part of the NelmioApiDocBundle package.
- *
- * (c) Nelmio
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Nelmio\ApiDocBundle\Describer;
-
-use EXSyst\Component\Swagger\Collections\Parameters;
-use EXSyst\Component\Swagger\Operation;
-use EXSyst\Component\Swagger\Parameter;
-use EXSyst\Component\Swagger\Path;
-use EXSyst\Component\Swagger\Swagger;
-
-/**
- * Merges parameters that have been added as refs through config files with those that get auto generated based on
- * routes.
- */
-class ParameterRefMergeDescriber implements DescriberInterface
-{
-    /**
-     * {@inheritdoc}
-     */
-    public function describe(Swagger $api)
-    {
-        /** @var Path $path */
-        foreach ($api->getPaths() as $path) {
-            /** @var Operation $operation */
-            foreach ($path->getOperations() as $operation) {
-                $this->checkOperation($api, $operation);
-            }
-        }
-    }
-
-    /**
-     * This method removes parameters that also have a ref version as they will be duplicated otherwise.
-     */
-    private function checkOperation(Swagger $api, Operation $operation)
-    {
-        $parametersToRemove = [];
-
-        /** @var Parameter[] $globalParams */
-        $globalParams = $api->getParameters();
-        /** @var Parameters|Parameter[] $currentParams */
-        $currentParams = $operation->getParameters();
-
-        foreach ($currentParams as $parameter) {
-            $ref = $parameter->getRef();
-
-            if (null === $ref) {
-                // we only concern ourselves with '$ref' parameters
-                continue;
-            }
-
-            $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref
-            if (!isset($globalParams[$ref])) {
-                // this really shouldn't happen, if it does there will be other failures elsewhere, so just ignore here
-                continue;
-            }
-
-            $refParameter = $globalParams[$ref];
-            // param ids are in form {name}/{in}
-            $refParameterId = \sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn());
-            if ($currentParams->has($refParameterId)) {
-                // if we got here it means a duplicate parameter is directly defined, schedule it for removal
-                $parametersToRemove[] = $currentParams->get($refParameterId);
-            }
-        }
-
-        foreach ($parametersToRemove as $parameterToRemove) {
-            $currentParams->remove($parameterToRemove);
-        }
-    }
-}

Resources/config/fos_rest.xml

@@ -7,7 +7,7 @@
         <service id="nelmio_api_doc.route_describers.fos_rest" class="Nelmio\ApiDocBundle\RouteDescriber\FosRestDescriber" public="false">
             <argument type="service" id="annotation_reader" />
 
-            <tag name="nelmio_api_doc.route_describer" priority="-300" />
+            <tag name="nelmio_api_doc.route_describer" priority="-250" />
         </service>
     </services>
 

Resources/config/php_doc.xml

@@ -5,7 +5,7 @@
 
     <services>
         <service id="nelmio_api_doc.route_describers.php_doc" class="Nelmio\ApiDocBundle\RouteDescriber\PhpDocDescriber" public="false">
-            <tag name="nelmio_api_doc.route_describer" priority="-1000" />
+            <tag name="nelmio_api_doc.route_describer" priority="-275" />
         </service>
     </services>
 

Resources/config/services.xml

@@ -32,10 +32,6 @@
             <tag name="nelmio_api_doc.describer" priority="-1000" />
         </service>
 
-        <service id="nelmio_api_doc.describers.param_ref_merge" class="Nelmio\ApiDocBundle\Describer\ParameterRefMergeDescriber" public="false">
-            <tag name="nelmio_api_doc.describer" priority="-999" />
-        </service>
-
         <!-- Routing Describers -->
         <service id="nelmio_api_doc.route_describers.route_metadata" class="Nelmio\ApiDocBundle\RouteDescriber\RouteMetadataDescriber" public="false">
             <tag name="nelmio_api_doc.route_describer" priority="-300" />

RouteDescriber/RouteMetadataDescriber.php

@@ -14,6 +14,9 @@ namespace Nelmio\ApiDocBundle\RouteDescriber;
 use EXSyst\Component\Swagger\Swagger;
 use Symfony\Component\Routing\Route;
 
+/**
+ * Should be last route describer executed to make sure all params are set.
+ */
 final class RouteMetadataDescriber implements RouteDescriberInterface
 {
     use RouteDescriberTrait;
@@ -26,12 +29,39 @@ final class RouteMetadataDescriber implements RouteDescriberInterface
             $requirements = $route->getRequirements();
             $compiledRoute = $route->compile();
 
+            $globalParams = $api->getParameters();
+            $existingParams = [];
+            foreach ($operation->getParameters() as $id => $parameter) {
+                $ref = $parameter->getRef();
+                if (null === $ref) {
+                    $existingParams[$id] = true;
+
+                    // we only concern ourselves with '$ref' parameters
+                    continue;
+                }
+
+                $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref
+                if (!isset($globalParams[$ref])) {
+                    // this shouldn't happen, so just ignore here
+                    continue;
+                }
+
+                $refParameter = $globalParams[$ref];
+
+                // param ids are in form {name}/{in}
+                $existingParams[\sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn())] = true;
+            }
+
             // Don't include host requirements
             foreach ($compiledRoute->getPathVariables() as $pathVariable) {
                 if ('_format' === $pathVariable) {
                     continue;
                 }
 
+                if (isset($existingParams[$pathVariable.'/path'])) {
+                    continue; // ignore this param, it is already defined
+                }
+
                 $parameter = $operation->getParameters()->get($pathVariable, 'path');
                 $parameter->setRequired(true);
 
@@ -39,7 +69,7 @@ final class RouteMetadataDescriber implements RouteDescriberInterface
                     $parameter->setType('string');
                 }
 
-                if (isset($requirements[$pathVariable])) {
+                if (isset($requirements[$pathVariable]) && null === $parameter->getPattern()) {
                     $parameter->setPattern($requirements[$pathVariable]);
                 }
             }

Tests/Describer/ParameterRefMergeDescriberTest.php

@@ -1,64 +0,0 @@
-<?php
-
-/*
- * This file is part of the NelmioApiDocBundle package.
- *
- * (c) Nelmio
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Nelmio\ApiDocBundle\Tests\Describer;
-
-use EXSyst\Component\Swagger\Swagger;
-use Nelmio\ApiDocBundle\Describer\ParameterRefMergeDescriber;
-
-class ParameterRefMergeDescriberTest extends AbstractDescriberTest
-{
-    public function testDescribe()
-    {
-        // this sample config was taken from a json generated by swagger without ParameterRefMergeDescriber
-        $apiDef = [
-            'swagger' => '2.0',
-            'info' => ['title' => 'Ref Test'],
-            'paths' => [
-                '/api/{version}/product' => [
-                    'get' => [
-                        'parameters' => [
-                            [
-                                '$ref' => '#/parameters/versionParam',
-                            ],
-                            [
-                                'name' => 'version',
-                                'in' => 'path',
-                                'required' => true,
-                                'type' => 'string',
-                                'pattern' => 'v\\d+',
-                            ],
-                        ],
-                    ],
-                ],
-            ],
-            'parameters' => [
-                'versionParam' => [
-                    'name' => 'version',
-                    'in' => 'path',
-                    'required' => true,
-                    'type' => 'string',
-                ],
-            ],
-        ];
-        $api = new Swagger($apiDef);
-        $this->describer->describe($api);
-
-        $describedData = $api->toArray();
-        // only one parameter should remain as they were duplicates
-        $this->assertCount(1, $describedData['paths']['/api/{version}/product']['get']['parameters']);
-    }
-
-    protected function setUp()
-    {
-        $this->describer = new ParameterRefMergeDescriber();
-    }
-}

Tests/Functional/FunctionalTest.php

@@ -433,4 +433,9 @@ class FunctionalTest extends WebTestCase
         $this->assertSame('This is post', $postOperation->getDescription());
         $this->assertSame('Worked well!', $postOperation->getResponses()->get(200)->getDescription());
     }
+
+    public function testNoDuplicatedParameters()
+    {
+        $this->assertFalse($this->getOperation('/api/article/{id}', 'get')->getParameters()->has('id', 'path'));
+    }
 }