Fix path for custom operation with Swagger UI #1192

meyerbaptiste · 2017-06-21T18:07:53Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	api-platform/api-platform#254
License	MIT
Doc PR	N/A

A fix while waiting for Swagger 3 which will permit overriding the base path at the path item level.
See:

Simperfit

Just a little comment but LGTM

Simperfit · 2017-06-22T12:00:44Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

+{
+    const ROUTE_NAME_PREFIX = 'api_';
+
+    private function __construct()


Static class. Should this class be moved to Util?

I don't think it should be moved to Util, it's a right place here ;)

private constructor does prevent the class from being instantiated no? I think it's the point here :p.

Yep, right! Recipe for static classes:

final class

private constructor

static attributes/methods

soyuka · 2017-06-22T12:15:29Z

src/Bridge/Symfony/Routing/ApiLoader.php

@@ -35,6 +34,10 @@
 */
 final class ApiLoader extends Loader
 {
+    /**
+     * @deprecated since version 2.0.10, to be removed in 3.0.
+     *             Use {@see RouteNameGenerator::ROUTE_NAME_PREFIX} instead.


indentation

I would merge this patch in 2.1, WDYT? I don't like to introduce deprecations in patch releases.

As you want, but this bug seems critic for many people (cf issue).

soyuka · 2017-06-22T12:17:06Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

+     *
+     * @return string
+     */
+    public static function generate(string $operationName, string $resourceShortName, bool $collection): string


Oh okay we're on 2.0 here. We need to change the collection flag to an OperationType when merging this on master IMO.

See my previous comment (we should merge this only in master IMO)

So they will upgrade to 2.1 :P

😄 Okay, I'll update the base branch, and my PR.

Oh okay @dunglas didn't saw your comment, and I'm 👍 on merging this in master! So yes, please drop the bool $collection for an OperationType :D.

Not so easy with sub resources 😢

I can take care of it if you want, anyway I want to refactor some parts and introduce Subresource metadata which should make things easier. For now, you can just support Item and Collection!

Are sub resources not exposed by Swagger UI? 😮

Not yet but we've an open pr here: #1188

dunglas · 2017-06-22T12:22:19Z

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

@@ -13,7 +13,7 @@
            <argument type="service" id="api_platform.resource_class_resolver" />
            <argument type="service" id="api_platform.operation_method_resolver" />
            <argument type="service" id="api_platform.operation_path_resolver" />
-            <argument type="service" id="api_platform.router" />
+            <argument>null</argument>


<argument /> is enough IIRC

An empty string is dumped with <argument />...

dunglas · 2017-06-22T12:23:12Z

src/Bridge/Symfony/Routing/ApiLoader.php

@@ -35,6 +34,10 @@
 */
 final class ApiLoader extends Loader
 {
+    /**
+     * @deprecated since version 2.0.10, to be removed in 3.0.
+     *             Use {@see RouteNameGenerator::ROUTE_NAME_PREFIX} instead.


I would merge this patch in 2.1, WDYT? I don't like to introduce deprecations in patch releases.

dunglas · 2017-06-22T12:24:44Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

+ *
+ * @author Baptiste Meyer <baptiste.meyer@gmail.com>
+ */
+class RouteNameGenerator


Can we mark this class @internal, I don't like public static classes.

dunglas · 2017-06-22T12:25:11Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

+     *
+     * @return string
+     */
+    public static function generate(string $operationName, string $resourceShortName, bool $collection): string


See my previous comment (we should merge this only in master IMO)

meyerbaptiste · 2017-06-23T19:50:59Z

Okay, the PR is ready with the master branch and ready for review.

I didn't take care of sub-resources. IMO we can add the support of these ones when the DocumentationNormalizer will support them.

dunglas · 2017-06-26T19:51:55Z

Amazing work! Thank you very much for this fix and this cleanup @meyerbaptiste!!

soyuka · 2017-07-04T13:43:52Z

src/Bridge/Symfony/Routing/RouteNameGenerator.php

+    {
+        if (OperationType::SUBRESOURCE === $operationType = OperationTypeDeprecationHelper::getOperationType($operationType)) {
+            throw new InvalidArgumentException(sprintf('%s::SUBRESOURCE is not supported as operation type by %s().', OperationType::class, __METHOD__));
+        }


@meyerbaptiste I have to remove this basically and use this class in the RouterOperationPathResolver?

Yes, ideally...

…ustom_operation_path Fix path for custom operation with Swagger UI

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch from 537a3b2 to 1b31a5c Compare June 21, 2017 18:08

meyerbaptiste mentioned this pull request Jun 21, 2017

Route prefix duplicates in custom operations api-platform/api-platform#254

Closed

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch 2 times, most recently from a6755f1 to 806deaa Compare June 22, 2017 08:34

Simperfit approved these changes Jun 22, 2017

View reviewed changes

soyuka approved these changes Jun 22, 2017

View reviewed changes

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch from 806deaa to 568ab59 Compare June 22, 2017 12:26

dunglas requested changes Jun 22, 2017

View reviewed changes

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch from 568ab59 to c1c2e6d Compare June 23, 2017 19:25

meyerbaptiste changed the base branch from 2.0 to master June 23, 2017 19:26

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch 2 times, most recently from 5fc0e5e to daa672e Compare June 23, 2017 19:42

Fix path for custom operation with Swagger UI

7114ebb

meyerbaptiste force-pushed the fix_swagger_custom_operation_path branch from daa672e to 7114ebb Compare June 23, 2017 20:03

soyuka approved these changes Jun 24, 2017

View reviewed changes

dunglas approved these changes Jun 26, 2017

View reviewed changes

dunglas merged commit fd1a553 into api-platform:master Jun 26, 2017

meyerbaptiste deleted the fix_swagger_custom_operation_path branch June 26, 2017 20:03

meyerbaptiste mentioned this pull request Jun 30, 2017

Prefix route api-platform/api-platform#227

Closed

soyuka reviewed Jul 4, 2017

View reviewed changes

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1192 from meyerbaptiste/fix_swagger_c…

6b1d4d9

…ustom_operation_path Fix path for custom operation with Swagger UI

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix path for custom operation with Swagger UI #1192

Fix path for custom operation with Swagger UI #1192

meyerbaptiste commented Jun 21, 2017

Simperfit left a comment

Simperfit Jun 22, 2017

meyerbaptiste Jun 22, 2017

Simperfit Jun 22, 2017

soyuka Jun 22, 2017

meyerbaptiste Jun 22, 2017

soyuka Jun 22, 2017

dunglas Jun 22, 2017

meyerbaptiste Jun 22, 2017

soyuka Jun 22, 2017

dunglas Jun 22, 2017

dunglas Jun 22, 2017

meyerbaptiste Jun 22, 2017

soyuka Jun 22, 2017

meyerbaptiste Jun 22, 2017

soyuka Jun 22, 2017

meyerbaptiste Jun 23, 2017

soyuka Jun 23, 2017

dunglas Jun 22, 2017

meyerbaptiste Jun 22, 2017

dunglas Jun 22, 2017

dunglas Jun 22, 2017

dunglas Jun 22, 2017

meyerbaptiste commented Jun 23, 2017

dunglas commented Jun 26, 2017

soyuka Jul 4, 2017

meyerbaptiste Jul 4, 2017

Fix path for custom operation with Swagger UI #1192

Fix path for custom operation with Swagger UI #1192

Conversation

meyerbaptiste commented Jun 21, 2017

Simperfit left a comment

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meyerbaptiste commented Jun 23, 2017

dunglas commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment