Ignore unsupported HTTP methods #1294
Conversation
|
i just noticed a similar problem with an end point that supports COPY. are you sure that the javascript part can not handle this? if i understand correctly, this PR will simply drop those methods without reporting that there is a problem. is there anything better we can do? |
notFloran
changed the title
Swagger/OpenAPI only supports : get, put, post, delete, options, head, patch. So this end points will never display in the javascript parts. See:
Yes, methods that are not in I don't see how to improve that, maybe a log ? |
Describer/SwaggerPhpDescriber.php
Outdated
| @@ -206,6 +206,7 @@ private function getMethodsToParse(): \Generator | |||
| $path = $this->normalizePath($route->getPath()); | |||
| $httpMethods = $route->getMethods() ?: Swagger::$METHODS; | |||
| $httpMethods = array_map('strtolower', $httpMethods); | |||
| $httpMethods = array_intersect($httpMethods, Swagger::$METHODS); | |||
There was a problem hiding this comment.
what happens if the methods become an empty array here, because only unsupported methods are in the list?
also, is it invalid for open api spec to specify this methods, or is it "just" a bug of the swagger-ui library? if its a problem of that library, we should see if we can do something later in the process, or have a configuration option, so that people that care about the specification of their api can get a full spec that does not drop methods.
There was a problem hiding this comment.
what happens if the methods become an empty array here, because only unsupported methods are in the list?
Nothing, in the rest of the code we go through the methods to consolidate the annotations and add them to the analysis so if a path does not have methods it is ignored.
I have changed the code, if there are no valid methods we go to the next route.
also, is it invalid for open api spec to specify this methods, or is it "just" a bug of the swagger-ui library? if its a problem of that library, we should see if we can do something later in the process, or have a configuration option, so that people that care about the specification of their api can get a full spec that does not drop methods.
It's invalid for OpenAPI spec, the spec only supports : get, put, post, delete, options, head, patch.
See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#pathItemObject for the spec and this issue OAI/OpenAPI-Specification#480
|
Could you add a test to ensure there won't be a regression please ? (Adding a method to one of the existing routes should be enough) And I have no strong opinion about logging, that could indeed help people understand the situation and it isn't complicated to implement so if you feel so I think we should add it :) |
Describer/SwaggerPhpDescriber.php
Outdated
|
|
||
| yield $method => [$path, $httpMethods]; | ||
| if (empty($validHttpMethods)) { | ||
| $this->logger->warning('No valid HTTP method for path', [ |
There was a problem hiding this comment.
What about replacing no valid by no supported? It is less alarming imo :)
There was a problem hiding this comment.
to help the user some more, we could even say: "None of the HTTP methods specified for path {path} are supported by swagger-ui, skipping this path"
|
Thank you @notFloran :) |
If an action use a LINK, UNLINK or COPY methods the bundle fails with:
Notice: Undefined index: link in vendor/nelmio/api-doc-bundle/Describer/SwaggerPhpDescriber.php (line 174)OpenAPI doesn't supports the LINK and UNLINK methods so the bundle must ignore this methods.
Fix #1290