Skip to content

Support for normalization groups and filters in the Swagger doc #753

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

Merged

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Sep 12, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #728
License MIT
Doc PR

WIP

It does fixes the issues in #728, now swagger docs output groups correctly.

@@ -127,119 +126,235 @@ private function getPath(string $resourceShortName, array $operation, bool $coll
*
* @return \ArrayObject
*/
private function getPathOperation(string $operationName, array $operation, string $method, bool $collection, ResourceMetadata $resourceMetadata, array $mimeTypes) : \ArrayObject
private function getPathOperation(string $operationName, array $operation, string $method, bool $collection, string $resourceClass, ResourceMetadata $resourceMetadata, array $mimeTypes, \ArrayObject $definitions) : \ArrayObject
Copy link
Member

Choose a reason for hiding this comment

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

The PHPDoc must be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like the mutating of $definitions. This is a step backwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Simperfit If the code structure does not lend itself well to the new requirements, don't try to shoehorn things into the existing structure. 😄

Copy link
Member

Choose a reason for hiding this comment

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

@teohhanhui why? It's an object not an array, so it's by definition a reference and it's ok to mutate it.
I don't get the benefit of using \ArrayObject instead of a raw array if we cannot use such features :)

Copy link
Contributor

@teohhanhui teohhanhui Sep 14, 2016

Choose a reason for hiding this comment

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

No, I only used \ArrayObject for the "empty object" behaviour when serialized to JSON. Anyway, immutability makes for cleaner code. And that's what I focused on in my previous refactoring. Now it's being undone.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I'm 👍 for merging as is. We can always refactor later...

@dunglas dunglas changed the title wip: DocumentationNormalizer Refactor && fix #728 DocumentationNormalizer Refactor. Closes #728. Sep 13, 2016
@dunglas dunglas changed the title DocumentationNormalizer Refactor. Closes #728. DocumentationNormalizer refactoring. Closes #728. Sep 13, 2016
@Simperfit
Copy link
Contributor Author

LGTM ;)

@dunglas dunglas force-pushed the feature/728-show-in-out-in-swagger-doc branch from 7801a53 to ae7ea2f Compare September 14, 2016 12:04
@dunglas dunglas force-pushed the feature/728-show-in-out-in-swagger-doc branch from ae7ea2f to 94119bd Compare September 14, 2016 12:07
@dunglas dunglas changed the title DocumentationNormalizer refactoring. Closes #728. Support for normalization groups and filters in the Swagger doc Sep 14, 2016
@dunglas dunglas merged commit 955d778 into api-platform:master Sep 14, 2016
@dunglas
Copy link
Member

dunglas commented Sep 14, 2016

Thanks @Simperfit

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
…n-out-in-swagger-doc

Support for normalization groups and filters in the Swagger doc
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.

3 participants