Parse @group tag on method properly - fixes #564, #561

shalvah · shalvah · commit 5647eda35ebb · 2019-08-31T11:43:08.000+01:00
diff --git a/TODO.md b/TODO.md
@@ -1,3 +1,4 @@
 Major
 - Bring `bindings` outside of `response_calls`
 - Should `routes.*.apply.response_calls.headers` be replaced by `routes.*.apply.headers`?
+- Should we move HTML generation from Blade to fully PHP? (L)
diff --git a/resources/views/documentarian.blade.php b/resources/views/documentarian.blade.php
@@ -6,9 +6,7 @@
 <!-- END_INFO -->
 {!! $prependMd !!}
 @foreach($parsedRoutes as $groupName => $routes)
-@if($groupName)
 #{!! $groupName !!}
-@endif
 {{-- We pick the first non-empty description we see. --}}
 {!! array_first($routes, function ($route) { return $route['groupDescription'] !== ''; })['groupDescription'] ?? '' !!}
 @foreach($routes as $parsedRoute)
diff --git a/src/Commands/GenerateDocumentation.php b/src/Commands/GenerateDocumentation.php
@@ -60,8 +60,8 @@ public function __construct(RouteMatcher $routeMatcher)
      */
     public function handle()
     {
-        // Using a global static variable here, so fuck off if you don't like it
-        // Also, the --verbose option is included with all Artisan commands
+        // Using a global static variable here, so fuck off if you don't like it.
+        // Also, the --verbose option is included with all Artisan commands.
         Flags::$shouldBeVerbose = $this->option('verbose');
 
         $this->docConfig = new DocumentationConfig(config('apidoc'));
@@ -82,13 +82,14 @@ public function handle()
 
         $generator = new Generator($this->docConfig);
         $parsedRoutes = $this->processRoutes($generator, $routes);
-        $parsedRoutes = collect($parsedRoutes)->groupBy('groupName')
+        $groupedRoutes = collect($parsedRoutes)
+            ->groupBy('groupName')
             ->sortBy(static function ($group) {
                 /* @var $group Collection */
                 return $group->first()['groupName'];
             }, SORT_NATURAL);
 
-        $this->writeMarkdown($parsedRoutes);
+        $this->writeMarkdown($groupedRoutes);
     }
 
     /**
diff --git a/src/Tools/Generator.php b/src/Tools/Generator.php
@@ -57,9 +57,9 @@ public function processRoute(Route $route, array $rulesToApply = [])
         $controller = new ReflectionClass($class);
         $method = $controller->getMethod($method);
 
-        list($routeGroupName, $routeGroupDescription) = $this->getRouteGroup($controller, $method);
 
         $docBlock = $this->parseDocBlock($method);
+        list($routeGroupName, $routeGroupDescription, $routeTitle) = $this->getRouteGroup($controller, $docBlock);
         $bodyParameters = $this->getBodyParameters($method, $docBlock['tags']);
         $queryParameters = $this->getQueryParameters($method, $docBlock['tags']);
         $content = ResponseResolver::getResponse($route, $docBlock['tags'], [
@@ -72,7 +72,7 @@ public function processRoute(Route $route, array $rulesToApply = [])
             'id' => md5($this->getUri($route).':'.implode($this->getMethods($route))),
             'groupName' => $routeGroupName,
             'groupDescription' => $routeGroupDescription,
-            'title' => $docBlock['short'],
+            'title' => $routeTitle ?: $docBlock['short'],
             'description' => $docBlock['long'],
             'methods' => $this->getMethods($route),
             'uri' => $this->getUri($route),
@@ -271,24 +271,40 @@ protected function parseDocBlock(ReflectionMethod $method)
 
     /**
      * @param ReflectionClass $controller
-     * @param ReflectionMethod $method
+     * @param array $methodDocBlock
      *
-     * @return array The route group name and description
+     * @return array The route group name, the group description, ad the route title
      */
-    protected function getRouteGroup(ReflectionClass $controller, ReflectionMethod $method)
+    protected function getRouteGroup(ReflectionClass $controller, array $methodDocBlock)
     {
         // @group tag on the method overrides that on the controller
-        $docBlockComment = $method->getDocComment();
-        if ($docBlockComment) {
-            $phpdoc = new DocBlock($docBlockComment);
-            foreach ($phpdoc->getTags() as $tag) {
+        if (!empty($methodDocBlock['tags'])) {
+            foreach ($methodDocBlock['tags'] as $tag) {
                 if ($tag->getName() === 'group') {
-                    $routeGroup = trim($tag->getContent());
-                    $routeGroupParts = explode("\n", $tag->getContent());
+                    $routeGroupParts = explode("\n", trim($tag->getContent()));
                     $routeGroupName = array_shift($routeGroupParts);
-                    $routeGroupDescription = implode("\n", $routeGroupParts);
-
-                    return [$routeGroupName, $routeGroupDescription];
+                    $routeGroupDescription = trim(implode("\n", $routeGroupParts));
+
+                    // If the route has no title (aka "short"),
+                    // we'll assume the routeGroupDescription is actually the title
+                    // Something like this:
+                    // /**
+                    //   * Fetch cars. <-- This is route title.
+                    //   * @group Cars <-- This is group name.
+                    //   * APIs for cars. <-- This is group description (not required).
+                    //   **/
+                    // VS
+                    // /**
+                    //   * @group Cars <-- This is group name.
+                    //   * Fetch cars. <-- This is route title, NOT group description.
+                    //   **/
+
+                    // BTW, this is a spaghetti way of doing this.
+                    // It shall be refactored soon. Deus vult!💪
+                    if (empty($methodDocBlock['short'])) {
+                        return [$routeGroupName, '', $routeGroupDescription];
+                    }
+                    return [$routeGroupName, $routeGroupDescription, $methodDocBlock['short']];
                 }
             }
         }
@@ -298,16 +314,16 @@ protected function getRouteGroup(ReflectionClass $controller, ReflectionMethod $
             $phpdoc = new DocBlock($docBlockComment);
             foreach ($phpdoc->getTags() as $tag) {
                 if ($tag->getName() === 'group') {
-                    $routeGroupParts = explode("\n", $tag->getContent());
+                    $routeGroupParts = explode("\n", trim($tag->getContent()));
                     $routeGroupName = array_shift($routeGroupParts);
                     $routeGroupDescription = implode("\n", $routeGroupParts);
 
-                    return [$routeGroupName, $routeGroupDescription];
+                    return [$routeGroupName, $routeGroupDescription, $methodDocBlock['short']];
                 }
             }
         }
 
-        return [$this->config->get(('default_group')), ''];
+        return [$this->config->get(('default_group')), '', $methodDocBlock['short']];
     }
 
     private function normalizeParameterType($type)
diff --git a/src/Tools/RouteMatcher.php b/src/Tools/RouteMatcher.php
@@ -48,6 +48,9 @@ public function getRoutesToBeDocumented(array $routeRules, bool $usingDingoRoute
         return $matchedRoutes;
     }
 
+    // TODO we should cache the results of this, for Laravel routes at least,
+    // to improve performance, since this method gets called
+    // for each ruleset in the config file. Not a high priority, though.
     private function getAllRoutes(bool $usingDingoRouter, array $versions = [])
     {
         if (! $usingDingoRouter) {
diff --git a/tests/Fixtures/TestController.php b/tests/Fixtures/TestController.php
@@ -33,6 +33,39 @@ public function withGroupOverride()
         return '';
     }
 
+    /**
+     * This is also in Group B. No route description. Route title before gropp.
+     *
+     * @group Group B
+     */
+    public function withGroupOverride2()
+    {
+        return '';
+    }
+
+    /**
+     * @group Group B
+     *
+     * This is also in Group B. Route title after group.
+     */
+    public function withGroupOverride3()
+    {
+        return '';
+    }
+
+    /**
+     * This is in Group C. Route title before group.
+     *
+     * @group Group C
+     *
+     * Group description after group.
+     *
+     */
+    public function withGroupOverride4()
+    {
+        return '';
+    }
+
     /**
      * @bodyParam user_id int required The id of the user. Example: 9
      * @bodyParam room_id string The id of the room.
diff --git a/tests/Unit/GeneratorTestCase.php b/tests/Unit/GeneratorTestCase.php
@@ -4,7 +4,6 @@
 
 use Orchestra\Testbench\TestCase;
 use Mpociot\ApiDoc\Tools\Generator;
-use Illuminate\Support\Facades\Storage;
 use Mpociot\ApiDoc\Tools\DocumentationConfig;
 use Mpociot\ApiDoc\ApiDocGeneratorServiceProvider;
 
@@ -222,10 +221,28 @@ public function can_parse_route_group()
     /** @test */
     public function method_can_override_controller_group()
     {
-        $route = $this->createRoute('GET', '/api/test', 'withGroupOverride');
-        $routeGroup = $this->generator->processRoute($route)['groupName'];
-
-        $this->assertSame('Group B', $routeGroup);
+        $route = $this->createRoute('GET', '/group/1', 'withGroupOverride');
+        $parsedRoute = $this->generator->processRoute($route);
+        $this->assertSame('Group B', $parsedRoute['groupName']);
+        $this->assertSame('', $parsedRoute['groupDescription']);
+
+        $route = $this->createRoute('GET', '/group/2', 'withGroupOverride2');
+        $parsedRoute = $this->generator->processRoute($route);
+        $this->assertSame('Group B', $parsedRoute['groupName']);
+        $this->assertSame('', $parsedRoute['groupDescription']);
+        $this->assertSame('This is also in Group B. No route description. Route title before gropp.', $parsedRoute['title']);
+
+        $route = $this->createRoute('GET', '/group/3', 'withGroupOverride3');
+        $parsedRoute = $this->generator->processRoute($route);
+        $this->assertSame('Group B', $parsedRoute['groupName']);
+        $this->assertSame('', $parsedRoute['groupDescription']);
+        $this->assertSame('This is also in Group B. Route title after group.', $parsedRoute['title']);
+
+        $route = $this->createRoute('GET', '/group/4', 'withGroupOverride4');
+        $parsedRoute = $this->generator->processRoute($route);
+        $this->assertSame('Group C', $parsedRoute['groupName']);
+        $this->assertSame('Group description after group.', $parsedRoute['groupDescription']);
+        $this->assertSame('This is in Group C. Route title before group.', $parsedRoute['title']);
     }
 
     /** @test */

Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,9 @@ public function getRoutesToBeDocumented(array $routeRules, bool $usingDingoRoute`
`48`	`48`	`return $matchedRoutes;`
`49`	`49`	`}`
`50`	`50`
	`51`	`+ // TODO we should cache the results of this, for Laravel routes at least,`
	`52`	`+ // to improve performance, since this method gets called`
	`53`	`+ // for each ruleset in the config file. Not a high priority, though.`
`51`	`54`	`private function getAllRoutes(bool $usingDingoRouter, array $versions = [])`
`52`	`55`	`{`
`53`	`56`	`if (! $usingDingoRouter) {`