zircote / swagger-php

A php swagger annotation and parsing library
http://zircote.github.io/swagger-php/
Apache License 2.0
5.03k stars 933 forks source link

Explicitly defining operation properties with 3rd party already defining them now causes errors #1580

Closed emhovis closed 2 months ago

emhovis commented 2 months ago

When using FOS Rest to define routes, explicitly defining the properties in the operation now causes errors as of commit https://github.com/zircote/swagger-php/commit/c84d201338eaa1f095d7edff828299215db96c5c

The errors are from mergeProperties function of AbstractAnnotation: "Multiple definitions for {path/method/etc}".

An example:

/**
     * @Operation(
     *      tags={"example"},
     *      summary="Get a list of all examples",
     *      method="GET"
     *      path="/path_here"
     * )
*/
#[Annotations\Get(path: '/path_here}', name: 'route_name')] 

Prior to the specified commit, the above had no issues. I either have to restrict the bundle to <4.7.16 or remove the method="GET" & path="/path_here" to resolve the errors.

Is this behavior intentional? I'd think that any of the explicitly defined properties within the Operation would "overwrite" previously defined ones rather than just complain that it already exists. My biggest concern is that if we have to remove all of those properties, if we move away from fos/rest, we'll have to add them right back. To me, it makes more sense to allow users to abstract their documentation from 3rd party bundles if they so wish- this allows easy migration from 3rd party packages such as fos/rest without needing to change the documentation to support whatever alternative is being used.

DerManoMann commented 2 months ago

I suppose you are using swagger-php not standalone? In that case it might be an issue with whatever bundle/plugin you are using. Swagger-php on its own will ignore all annotations it does not recognise.

emhovis commented 2 months ago

I suppose you are using swagger-php not standalone? In that case it might be an issue with whatever bundle/plugin you are using. Swagger-php on its own will ignore all annotations it does not recognise.

The issue comes up when upgrading swagger-php to 4.7.16, more specifically the exact commit https://github.com/zircote/swagger-php/commit/c84d201338eaa1f095d7edff828299215db96c5c

It does seem that Nelmio/ApiDocBundle is the one "mixing" the two sets of annotations and calling mergeProperties in which swagger-php now throws a fit because method/path are being defined twice, even though they're the same value.

emhovis commented 2 months ago

Perhaps, in mergeProperties, instead of directly throwing an error when two properties with the same name exist, what if we compare the values of the two properties and only throw the error if they're different? That'd let people use abstracted documentation (so long as it matches the values that 3rd-party bundles/plugins are setting).

emhovis commented 2 months ago

Perhaps, in mergeProperties, instead of directly throwing an error when two properties with the same name exist, what if we compare the values of the two properties and only throw the error if they're different? That'd let people use abstracted documentation (so long as it matches the values that 3rd-party bundles/plugins are setting).

Well now I feel dumb. After looking into it, mergeProperties already does do that... the problem I have is we'd been using method="GET" rather than method="get", which seemed to work before but now, (thanks to the bugfix in the commit mentioned) actually reaches the point where it ends up comparing "get" and "GET".