Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenApi v3 refactor #3407

Merged
merged 17 commits into from
Jul 23, 2020
Merged

OpenApi v3 refactor #3407

merged 17 commits into from
Jul 23, 2020

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Feb 21, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tickets na
License MIT
Doc PR todo

With this rework, I want to improve the developer experience in enhancing our OpenAPI support. By using explicit classes that matches the OpenAPI spec, it should be easier to:

Note that there are some missing features (for example pagination parameters and components) and I wanted to get back some feedbacks before continuing this work as it's a heavy refactor.
I also want to add two new objects (PaginationOptions, OpenApiAuthentificationOptions) to reduce the number of arguments in the constructor.

WDYT ? Should I continue working on this?

@teohhanhui
Copy link
Contributor

I think we should make all the classes immutable. Mutability makes the code very hard to reason about, when there's so much going on.

@soyuka
Copy link
Member Author

soyuka commented Feb 21, 2020

I think we should make all the classes immutable. Mutability makes the code very hard to reason about, when there's so much going on.

Indeed, many classes aren't good yet there are really at the POC state ^^. I think that the Paths object has to be mutable: indeed it's a unique collection of paths and needs to be query-able by path.

@dunglas
Copy link
Member

dunglas commented Feb 21, 2020

Should I continue working on this?

Definitely, it sounds like a great (and needed) design improvement. At some point we'll even maybe be able to provide a standalone OpenAPI component (a subtree split of this repo).

src/DataProvider/PaginationOptions.php Outdated Show resolved Hide resolved
src/DataProvider/PaginationOptions.php Outdated Show resolved Hide resolved
src/DataProvider/PaginationOptions.php Outdated Show resolved Hide resolved
src/DataProvider/PaginationOptions.php Outdated Show resolved Hide resolved
src/DataProvider/PaginationOptions.php Outdated Show resolved Hide resolved
src/OpenApi/Options.php Outdated Show resolved Hide resolved
src/OpenApi/Options.php Outdated Show resolved Hide resolved
src/OpenApi/Options.php Outdated Show resolved Hide resolved
src/OpenApi/Serializer/OpenApiNormalizer.php Outdated Show resolved Hide resolved
src/OpenApi/Serializer/OpenApiNormalizer.php Outdated Show resolved Hide resolved
@soyuka soyuka force-pushed the openapi branch 5 times, most recently from e3035ba to 1c3c2dd Compare April 29, 2020 09:21
@soyuka soyuka changed the title [WIP] OpenApi v3 refactor OpenApi v3 refactor Jun 12, 2020
@soyuka soyuka mentioned this pull request Jun 26, 2020
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job! 👏

src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Bundle/Command/OpenApiCommand.php Outdated Show resolved Hide resolved
src/OpenApi/Model/License.php Outdated Show resolved Hide resolved
src/OpenApi/Model/Link.php Outdated Show resolved Hide resolved
src/OpenApi/Model/Link.php Outdated Show resolved Hide resolved
src/OpenApi/Model/Parameter.php Show resolved Hide resolved
src/OpenApi/OpenApi.php Outdated Show resolved Hide resolved
@@ -105,6 +105,9 @@ public function create(array $context = []): OpenApi
return new OpenApi($info, $servers, $paths, new Model\Components(new \ArrayObject($schemas), new \ArrayObject(), new \ArrayObject(), new \ArrayObject(), new \ArrayObject(), new \ArrayObject(), new \ArrayObject($securitySchemes)), $securityRequirements);
}

/**
* @return array | array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array | array doesn't look right... maybe array[] was intended? or even array{array, array} (https://phpstan.org/writing-php-code/phpdoc-types#array-shapes)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed it should be array[]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait, I see that since then, $schemas has been changed from array to \ArrayObject... so that would give array{array, \ArrayObject} for PHPStan (or at this point maybe just remove this phpDoc?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the return has become useless, see #4199

return 0 === strpos($operation['path'], '/') ? $operation['path'] : '/'.$operation['path'];
}
$path = $this->operationPathResolver->resolveOperationPath($resourceShortName, $operation, $operationType, $operationName);
if ('.{_format}' === substr($path, -10)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @soyuka Does there any way to enable defining '.{_format}' per resource, even ApiPlatform\Core\OpenApi\Model\Paths does not contain a setter function, in case we decide to decorate the OpenapiFactory ? I have a resource that should export pdf and docx.
Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants