Skip to content

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Aug 24, 2016

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

Some refactoring but also fixed $operation['swagger_context'] handling by checking for existing values.

/cc @Simperfit

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 83.512% when pulling 875df00 on teohhanhui:fix-swagger into 0f7ec87 on api-platform:master.

@Simperfit
Copy link
Contributor

LGTM, thanks @teohhanhui

private function getPath(string $resourceShortName, array $operation, bool $collection) : string
{
$path = $this->operationPathResolver->resolveOperationPath($resourceShortName, $operation, $collection);
if (substr($path, -10) === '.{_format}') {
if ('.{_format}' === substr($path, -10)) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks not very solid to rely on such hardcoded values (missed it on previous PRs). Anyway, it's not related with this PR and can be fixed in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there are many other issues I've identified. But this PR is just for refactoring / better code quality.

@dunglas
Copy link
Member

dunglas commented Aug 24, 2016

Looks good to me 👍. By curiosity, why do you use \ArrayObject instead of standard arrays?

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Aug 25, 2016

@dunglas:

why do you use \ArrayObject instead of standard arrays?

Because PHP sucks. ArrayObject gives us an empty JSON object, unlike arrays. This is important in some places, like for the property Schema Object.

Also, I see that we might want to turn these into VOs so this is a good first step?

@dunglas dunglas merged commit e249b33 into api-platform:master Aug 25, 2016
@dunglas
Copy link
Member

dunglas commented Aug 25, 2016

Thanks @teohhanhui

@teohhanhui teohhanhui deleted the fix-swagger branch August 30, 2016 03:35
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
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.

4 participants