Add areas support #1169

GuilhemN · 2018-01-03T22:25:25Z

dbu

very interesting!

can you elaborate the documentation a bit, to add some more examples what exactly this does?

for reviewing it would be easier to split this up into at least the code cleanups and the actual feature. ideally, even the thing about refactoring how tagged services are integrated would have been separate, but that would now be a large effort to separate.

dbu · 2018-01-04T08:20:16Z

composer.json

@@ -16,27 +16,26 @@
    ],
    "require": {
        "php": "^7.0",
-        "symfony/framework-bundle": "^3.2.5|^4.0",
-        "symfony/property-info": "^3.1|^4.0",
+        "symfony/framework-bundle": "^3.4|^4.0",


i would do this cleanup in a separate pull request that does not add new features.

dbu · 2018-01-04T08:21:14Z

Tests/Functional/Entity/JMSComplex.php

 */

 namespace Nelmio\ApiDocBundle\Tests\Functional\Entity;

 use JMS\Serializer\Annotation as Serializer;

 /**
- * Class JMSComplex
- * @package Nelmio\ApiDocBundle\Tests\Functional\Entity
+ * Class JMSComplex.


this does not tell anything the code does not already say. i'd remove the text line completely. (and these things part of the cleanup pull request rather than the new feature.)

dbu · 2018-01-04T08:23:18Z

DependencyInjection/NelmioApiDocExtension.php

+                ])
+                ->addTag(sprintf('nelmio_api_doc.describer.%s', $area), ['priority' => -400]);
+
+            $container->register('nelmio_api_doc.describers.swagger_php', SwaggerPhpDescriber::class)


i find it an antipattern to define services in the DI extension instead of in the services xml files. its much harder to read what is going on imho. i would rather have additional services xml files for specific features that are only loaded if that feature is to be enabled. but i guess its a question of taste as well...

I tend to agree but these services are dynamically generated and we can't put them in a file unfortunately... Maybe using the php syntax would be clearer though (see symfony/symfony#23834).

its ok, i can live with it as is, if we can't move it to the xml :-)

dbu · 2018-01-04T08:23:41Z

DependencyInjection/Configuration.php

            ->children()
                ->arrayNode('documentation')
                    ->useAttributeAsKey('key')
                    ->info('The documentation used as base')
                    ->example(['info' => ['title' => 'My App']])
                    ->prototype('variable')->end()
                ->end()
-                ->arrayNode('routes')


this will need to be documented in the changelog

dbu · 2018-01-04T08:24:20Z

DependencyInjection/Configuration.php

+                })
+                ->then(function ($v) {
+                    $v['areas'] = $v['routes'];
+                    unset($v['routes']);


this should also trigger a deprecation warning

dbu · 2018-01-04T08:25:19Z

DependencyInjection/Configuration.php

@@ -21,20 +21,49 @@ public function getConfigTreeBuilder()
        $treeBuilder = new TreeBuilder();
        $treeBuilder
            ->root('nelmio_api_doc')
+            ->beforeNormalization()
+                ->ifTrue(function ($v) {
+                    return !isset($v['areas']) && isset($v['routes']);


if both areas and routes are set, we should throw an invalid configuration exception explaining to use only the new areas section. otherwise the user will just see an error that routes is not known, which will be confusing when before, routes did work.

dbu · 2018-01-04T08:28:58Z

README.md

@@ -77,9 +77,9 @@ routes that are documented by configuring the bundle:
 ```yml
 # app/config/config.yml
 nelmio_api_doc:
-    routes:
+    areas:


does the areas model allow to specify other criteria than path patterns? and how to specify more than one distinct area?

does the areas model allow to specify other criteria than path patterns?

For now it's only based on path patterns but we could easily add more criteria if we want to, I'd rather do this later though to not complicate this PR. The best imo would be to allow to add a custom routes filter.

and how to specify more than one distinct area?

You can define as many areas as you want:

nelmio_api_doc: areas: default: [ path_patterns: [ ^/api ] ] internal: [ path_patterns: [ ^/internal ] ] commercial: [ path_patterns: [ ^/commercial ] ] # ...

can you add a doc section for configuring several areas? so if you specify what we have here, you implicitly create a default area, and if you would explicitly create a list of areas, you get several?

GuilhemN · 2018-01-04T13:30:08Z

I added tests and docs. Much bigger diff :/

dbu

a features is also its test and doc, and this is no tiny thing ;-) great job!

i think there is something unfinished with the cache (that seems to be not tested if travis-ci is green) but otherwise looks good to me!

dbu · 2018-01-05T08:21:54Z

ApiDocGenerator.php

@@ -46,7 +46,7 @@ public function generate(): Swagger
        }

        if ($this->cacheItemPool) {
-            $item = $this->cacheItemPool->getItem('swagger_doc');
+            $item = $this->cacheItemPool->getItem($cacheItemId ?? 'swagger_doc');


i think this will never do anything. ?? is like isset, not telling you that the variable does not exist. but cacheItemId will never exist in this context unless i am missing something. did you mean to store the constructor parameter andin the class and use $this->cacheItemId?

Indeed, I added tests to make sure it works. Will have to use it one day to have better perfs...

dbu · 2018-01-05T08:24:30Z

Controller/DocumentationController.php

+        if (!$generatorLocator instanceof ContainerInterface) {
+            @trigger_error(sprintf('Providing an instance of "%s" to "%s()" is deprecated since version 3.1. Provide it an instance of "%s" instead.', ApiDocGenerator::class, __METHOD__, ContainerInterface::class), E_USER_DEPRECATED);
+            $generatorLocator = new ServiceLocator(['default' => function () use ($generatorLocator) {
+                return $generatorLocator;


this will only work if $generatorLocator is the ApiDocGenerator, right? i would typehint both and explain that the generator is still supported for BC. and do a typecheck before the deprecation to fail loudly if its not something we can work with.

type hint added :)

dbu

great! looks good now, feel free to squash and merge.

GuilhemN · 2018-01-05T12:09:16Z

Thanks for all your reviews! :)

GuilhemN force-pushed the newfeature branch from 1a16cd2 to 64b9e69 Compare January 3, 2018 22:26

GuilhemN mentioned this pull request Jan 3, 2018

Supporting different sets of documentations for different domains #1105

Closed

GuilhemN force-pushed the newfeature branch 2 times, most recently from a58314a to 85fa9f2 Compare January 3, 2018 22:32

dbu reviewed Jan 4, 2018

View reviewed changes

GuilhemN mentioned this pull request Jan 4, 2018

Cleanup #1170

Merged

GuilhemN force-pushed the newfeature branch 4 times, most recently from 47b6464 to f72ce77 Compare January 4, 2018 13:29

GuilhemN mentioned this pull request Jan 4, 2018

Rework the docs #1171

Merged

GuilhemN force-pushed the newfeature branch 3 times, most recently from a67d6e1 to e3d787f Compare January 4, 2018 19:07

GuilhemN added 2 commits January 4, 2018 20:09

Add areas support

702569e

Document the Areas feature

47faeec

GuilhemN force-pushed the newfeature branch from e3d787f to be03a27 Compare January 4, 2018 19:10

Allow to expose swagger area as JSON

1cf7be9

GuilhemN force-pushed the newfeature branch from be03a27 to 1cf7be9 Compare January 4, 2018 19:14

dbu reviewed Jan 5, 2018

View reviewed changes

Fixes

1dcec10

dbu approved these changes Jan 5, 2018

View reviewed changes

last fixes

1bbedfa

GuilhemN merged commit 393a6c0 into master Jan 5, 2018

GuilhemN deleted the newfeature branch January 5, 2018 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add areas support #1169

Add areas support #1169

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add areas support #1169

Add areas support #1169

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!