Create an interface to resolve the operations path #666

GuilhemN · 2016-08-03T17:37:21Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

This PR solves an issue of the swagger DocumentationNormalizer which doesn't include resources if they don't have at least one collection operation (that's the case when a resource has only item operations).
I finally choose a different solution than the one I proposed in #648 (comment) and created a new interface OperationPathResolverInterface. It replaces the current ResourcePathNamingStrategyInterface.

GuilhemN · 2016-08-03T17:46:08Z

tests/Bridge/Symfony/Routing/ApiLoaderTest.php

-            $routeCollection->get('api_dummies_get_item'),
-            $this->getRoute('/dummies/{id}.{_format}', 'api_platform.action.get_item', DummyEntity::class, 'get', ['GET'])
+            $this->getRoute('/dummies/{id}.{_format}', 'api_platform.action.get_item', DummyEntity::class, 'get', ['GET']),
+            $routeCollection->get('api_dummies_get_item')


I did this because the expected value must be before the one tested.

dunglas · 2016-08-06T04:44:54Z

ping @api-platform/core-team

Simperfit · 2016-08-06T08:04:46Z

LGTM
ping @theofidry @teohhanhui @sroze

dunglas · 2016-08-07T13:27:30Z

Dirty for the delay, I prefer to review it in depth or have the opinion of another member of the core team before merging it.

GuilhemN · 2016-08-07T13:34:51Z

@theofidry reacted to @Simperfit comment, i guess that means he agrees ?
As long as you don't want to tag a new stable version you have the time to review this :)

theofidry · 2016-08-07T13:39:46Z

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

@@ -35,12 +35,7 @@ public function getConfigTreeBuilder()
                ->scalarNode('title')->defaultValue('')->info('The title of the API.')->end()
                ->scalarNode('description')->defaultValue('')->info('The description of the API.')->end()
                ->scalarNode('version')->defaultValue('0.0.0')->info('The version of the API.')->end()
-                ->arrayNode('naming')


we need to update the doc accordingly as well

theofidry · 2016-08-07T13:42:31Z

👍 from me but I would be more confortable if @teohhanhui or @dunglas have time for a deeper review :)

dunglas · 2016-08-16T08:16:09Z

src/Bridge/Symfony/Bundle/Resources/config/swagger.xml

@@ -13,7 +13,7 @@
            <argument type="service" id="api_platform.metadata.property.metadata_factory" />
            <argument type="service" id="api_platform.resource_class_resolver" />
            <argument type="service" id="api_platform.operation_method_resolver" />
-            <argument type="service" id="api_platform.iri_converter" />
+            <argument type="service" id="api_platform.path_resolver.operation" />


Shouldn't the service be named api_platform.operation_path_resolver for consistency with other services?

Yes indeed, good point 👍

Can you update the PR? I'll merge it after that.

dunglas · 2016-08-16T08:22:23Z

I've just reviewed it in deep. Smart design!

I've just a minor comment regarding the service name. WDYT @Ener-Getick?

GuilhemN · 2016-08-16T08:31:35Z

Thanks !

And i updated the services name :)

dunglas · 2016-08-16T08:37:58Z

Test are broken now.

GuilhemN · 2016-08-16T08:40:12Z

Oops i forgot to change a service name in the tests...

dunglas · 2016-08-16T08:48:56Z

Thank you @Ener-Getick!

dunglas · 2016-08-16T09:24:54Z

@Ener-Getick do you think you'll be able to also update the related docs? https://github.com/api-platform/docs/blob/master/core/path-naming-strategy.md

GuilhemN · 2016-08-16T09:27:13Z

@dunglas yes it shouldn't be too long. I'll do that asap ;)

dunglas · 2016-08-16T09:36:39Z

Thanks!

teohhanhui · 2016-08-16T10:49:18Z

I don't like the name CustomOperationPathResolver. ~~Perhaps we can change it to DelegatedOperationPathResolver (suggestions welcome)? And $deferred is a misnomer, as it should be $delegate.~~

dunglas · 2016-08-16T11:15:15Z

I like DelegatedOperationPathResolver. WDYT @Ener-Getick?

GuilhemN · 2016-08-16T11:21:53Z

The advantage of CustomOperationPathResolver is that we know it resolves the path from a custom field. It's not clear to me what DelegatedOperationPathResolver does.
I don't like this name much as well but i don't have a better idea :/

GuilhemN · 2016-08-16T11:24:58Z

Maybe something relating to metadata? (Custom)MetadataOperationPathResolver
Or ExplicitOperationPathResolver?

teohhanhui · 2016-08-16T11:28:07Z

@Ener-Getick:

The advantage of CustomOperationPathResolver is that we know it resolves the path from a custom field.

We should not care whether it's "custom" or otherwise, because such a distinction does not matter at the class level. But yes, the service name should reflect the purpose (i.e. resolving based on operations config in resource metadata).

It's not clear to me what DelegatedOperationPathResolver does.

~~It delegates to OperationPathResolverInterface.~~

teohhanhui · 2016-08-16T11:31:08Z

Ok I've misunderstood...

GuilhemN · 2016-08-16T11:33:31Z

DelegatedOperationPathResolver makes me think it is delegated.
Of course this resolver delegates to another resolver but that's not its main purpose, that's just a fallback. That's why i think its name should reflect the source it uses to resolve a path.

teohhanhui · 2016-08-16T11:36:14Z

src/Bridge/Symfony/Bundle/Resources/config/api.xml

+            <argument type="service" id="api_platform.operation_path_resolver.custom" />
+        </service>
+
+        <service id="api_platform.operation_path_resolver.custom" class="ApiPlatform\Core\PathResolver\CustomOperationPathResolver" public="false">


Shouldn't api_platform.operation_path_resolver.custom decorate api_platform.operation_path_resolver.default?

(Why do we need the "default" part of the name, in the first place? This was already discussed before in #547)

It would be weird to decorate a service defined by the user in case he uses its own path resolver, don't you think ?

About the default part, that's just an alias and i couldnt use .operation_path_resolver as it is already in use. Maybe a better name would be .fallback or .root?

We need to be clear about the decoration priority, but by not using Symfony DI's service decoration (when in fact it's exactly what you're trying to achieve), the current implementation is clearly inflexible.

It would be weird to decorate a service defined by the user in case he uses its own path resolver, don't you think ?

I'll address this separately.

Create an interface to resolve the operations path

GuilhemN mentioned this pull request Aug 3, 2016

Use only ItemOperations but it's does'nt work #647

Closed

GuilhemN reviewed Aug 3, 2016
View reviewed changes

theofidry reviewed Aug 7, 2016
View reviewed changes

dunglas reviewed Aug 16, 2016
View reviewed changes

Create an interface to resolve the operations path

c88bdde

dunglas merged commit 0778893 into api-platform:master Aug 16, 2016

GuilhemN deleted the CACHE branch August 16, 2016 08:52

teohhanhui reviewed Aug 16, 2016
View reviewed changes

GuilhemN mentioned this pull request Aug 17, 2016

Replace Path Naming by Operation Path Resolvers api-platform/docs#101

Merged

dunglas mentioned this pull request Sep 5, 2016

Path generated in the Swagger doc don't take into account app_dev.php #722

Closed

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#666 from Ener-Getick/CACHE

82b70f2

Create an interface to resolve the operations path

This was referenced Jul 19, 2017

Improve the performance of CachedIdentifiersExtractor #1257

Merged

Swagger subresource + OperationPathResolver refactor proposal #1245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create an interface to resolve the operations path #666

Create an interface to resolve the operations path #666

GuilhemN commented Aug 3, 2016 •

edited

GuilhemN Aug 3, 2016 •

edited

dunglas commented Aug 6, 2016 •

edited

Simperfit commented Aug 6, 2016

dunglas commented Aug 7, 2016

GuilhemN commented Aug 7, 2016

theofidry Aug 7, 2016

theofidry commented Aug 7, 2016

dunglas Aug 16, 2016

GuilhemN Aug 16, 2016

dunglas Aug 16, 2016

GuilhemN Aug 16, 2016

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

teohhanhui commented Aug 16, 2016 •

edited

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

GuilhemN commented Aug 16, 2016 •

edited

teohhanhui commented Aug 16, 2016 •

edited

teohhanhui commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

teohhanhui Aug 16, 2016

GuilhemN Aug 16, 2016

teohhanhui Aug 16, 2016

Create an interface to resolve the operations path #666

Create an interface to resolve the operations path #666

Conversation

GuilhemN commented Aug 3, 2016 • edited

GuilhemN Aug 3, 2016 • edited

Choose a reason for hiding this comment

dunglas commented Aug 6, 2016 • edited

Simperfit commented Aug 6, 2016

dunglas commented Aug 7, 2016

GuilhemN commented Aug 7, 2016

Choose a reason for hiding this comment

theofidry commented Aug 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

dunglas commented Aug 16, 2016

teohhanhui commented Aug 16, 2016 • edited

dunglas commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

GuilhemN commented Aug 16, 2016 • edited

teohhanhui commented Aug 16, 2016 • edited

teohhanhui commented Aug 16, 2016

GuilhemN commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN commented Aug 3, 2016 •

edited

GuilhemN Aug 3, 2016 •

edited

dunglas commented Aug 6, 2016 •

edited

teohhanhui commented Aug 16, 2016 •

edited

GuilhemN commented Aug 16, 2016 •

edited

teohhanhui commented Aug 16, 2016 •

edited