Webhooks #1462
Webhooks #1462
Conversation
|
It seems too easy |
|
Nice one. It would be great if you could add some tests. Allowing a union of native types (string, etc) and |
|
Also, the serialize code might need to be conditional to suppress the property for 3.0.0 |
|
@DerManoMann I added test and changed types also for webhooks (only PathItem finally).
|
There was a problem hiding this comment.
Looking good so far.
Still some way to go to make all existing tests pass, tho. Also, it would be good to ensure that webhook cannot be used with 3.0.0 - something like https://github.com/zircote/swagger-php/blob/master/src/Annotations/License.php#L77
|
Sorry for ignoring this - I am quite busy ATM with paid work :/ |
No problem it's normal ;) I am quite busy too, it's not critical |
src/Annotations/OpenApi.php
Outdated
| /** | ||
| * The available webhooks for the API. | ||
| * | ||
| * @var array<string,PathItem> |
There was a problem hiding this comment.
It might actually be nice to have a new Webhook annotation/attribute for this rather than using raw maps on top level elements.
There was a problem hiding this comment.
Agree with that, I got this idea at first but I didn't know if I could add a custom annotation/attribute
There was a problem hiding this comment.
Bye the way, how do you think to handle this new annotation/attribute? a new Processor BuildWebhooks?
src/Annotations/OpenApi.php
Outdated
| @@ -143,6 +150,18 @@ public function validate(array $stack = null, array $skip = null, string $ref = | |||
| return false; | |||
| } | |||
|
|
|||
| if ($this->openapi === self::VERSION_3_0_0 && Generator::isDefault($this->paths)) { | |||
| $this->_context->logger->warning('The OpenAPI document must contain paths field'); | |||
There was a problem hiding this comment.
nit: I'd prefer if the error messag ecould be kept the same, even if it means manually reproducing the format.
There was a problem hiding this comment.
i.e. 'Required @OA\Info() not found'
src/Annotations/OpenApi.php
Outdated
| } | ||
|
|
||
| if ($this->openapi === self::VERSION_3_1_0 && Generator::isDefault($this->paths) && Generator::isDefault($this->webhooks) && Generator::isDefault($this->components)) { | ||
| $this->_context->logger->warning('The OpenAPI document must contain at least one paths field, a components field or a webhooks field'); |
There was a problem hiding this comment.
A message in similar style as above wouold be great.
Perhasps
At least one of 'Required @OA\Info(), @OA\Components() or @OA\Webhook() not found'
There was a problem hiding this comment.
Looks promising.
A few more things:
Webhookneeds to be added to the parent/nested arrays similar to other annotations inOpenApi(annotation)- The attribute version should extend the annotation version. Saves on re-declaring the properties and also allows to simplify the parent/nested relationships (which are based on annotations only)
There still needs to be code to merge the Webhook data into the right place when processing the annotation tree.
Again, this is related to $_nested. This should probably look something like this:
public static $_nested = [
Webhook::class => ['webhooks', 'name'],
];From memory the first element in the array is the property this gets merged in and if there is a second value then that is used as propery lookup in the annotation (Webhook) to find the key to store in under.
Sounds complicated (and it kinda is, at least to understand).
| * version="1.0.0", | ||
| * title="Webhook Example", | ||
| * ), | ||
| * webhooks={ |
There was a problem hiding this comment.
This still needs updating I think?
| @@ -566,6 +566,8 @@ The list of values includes alternative security requirement objects that can be | |||
| Only one of the security requirement objects need to be satisfied to authorize a request.<br /> | |||
| Individual operations can override this definition.<br /> | |||
| To make security optional, an empty security requirement `({})` can be included in the array.</p></dd> | |||
| <dt><strong>webhooks</strong> : <span style="font-family: monospace;">array<string,PathItem|string|class-string|object></span></dt> | |||
There was a problem hiding this comment.
docs need refresh - ideally done right before merge
| @@ -38,7 +38,6 @@ public function testScan(string $sourceDir, iterable $sources): void | |||
| public function testScanInvalidSource(): void | |||
| { | |||
| $this->assertOpenApiLogEntryContains('Skipping invalid source: /tmp/__swagger_php_does_not_exist__'); | |||
| $this->assertOpenApiLogEntryContains('Required @OA\Info() not found'); | |||
There was a problem hiding this comment.
Hmm, this is surprising.
| } | ||
|
|
||
| if ($this->openapi === self::VERSION_3_1_0 && Generator::isDefault($this->paths) && Generator::isDefault($this->webhooks) && Generator::isDefault($this->components)) { | ||
| $this->_context->logger->warning("At least one of 'Required @OA\Info(), @OA\Components() or @OA\Webhook() not found'"); |
There was a problem hiding this comment.
It should be @OA\PathItem instead of @OA\Info
|
Closing in favour of #1511. Thanks @qdequippe for doing most of the hard work. |
|
Thanks @DerManoMann for your feedbacks on this PR, sorry for the lack of time on my side. |
Version 3.1 brings "webhooks" to the specification.