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

Add subresources to hydra #1225

Merged
merged 1 commit into from
Jul 10, 2017
Merged

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Jul 4, 2017

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

ping @pierredup

Update of #1188

public function getParentResourceClass(): string
{
return $this->parentResourceClass;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we need those informations here. Usually when using a SubresourceMetadata we already have those informations. I want to update the PR to try and remove those adds.

@soyuka soyuka force-pushed the swagger-subresource branch 2 times, most recently from 3becd17 to 224afe4 Compare July 4, 2017 12:47
],
'/questions/{id}/answer' => [
'get' => new \ArrayObject([
'tags' => ['Answer'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't there be Answer and Question here?

@meyerbaptiste
Copy link
Member

We have to fix paths of sub-resources too: https://github.com/api-platform/core/pull/1192/files#diff-000bd9bbe078960ebd85ec9cbd642bcdR54

@soyuka
Copy link
Member Author

soyuka commented Jul 4, 2017

@meyerbaptiste could you remind me what needs to be done there?

@meyerbaptiste
Copy link
Member

meyerbaptiste commented Jul 4, 2017

We have to use the router to resolve paths (because of route prefix), so the way to find the route of a sub-resource (not so easy).

@soyuka soyuka force-pushed the swagger-subresource branch 3 times, most recently from 51d8f86 to 58961a3 Compare July 4, 2017 14:17
@@ -44,18 +45,39 @@ private function __construct()
*
* @return string
*/
public static function generate(string $operationName, string $resourceShortName, $operationType): string
public static function generate(string $operationName, string $resourceShortName, $operationType, string $property = null, bool $subresourceCollection = false): string
Copy link
Member Author

Choose a reason for hiding this comment

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

ping @meyerbaptiste I have to add property and collection information everywhere :|.

I'm not satisfied with this, WDYT?

I haven't changed the OperationPathResolvers yet because I'd need to add so many hacks to add those 2 new arguments... Let me know if you've a better idea in mind, I'm kinda stuck in the no-breaking-change-loop here 😄.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's why I said it's not so easy! I had encountered the same problem... 😞

$operation['swagger_context']['tags'] = [$resourceMetadata->getShortName(), $parentResourceMetadata->getShortName()];

$path = $this->getPath($parentResourceMetadata->getShortName(), $operationName, $operation, OperationType::SUBRESOURCE);
$method = 'GET'; // we only support GET for subresources
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: No need to declare variable.

@soyuka soyuka force-pushed the swagger-subresource branch 6 times, most recently from e29b6cc to 7dfc162 Compare July 7, 2017 14:32
@soyuka soyuka changed the title Swagger subresource Add subresources to hydra Jul 7, 2017
@soyuka
Copy link
Member Author

soyuka commented Jul 7, 2017

I've removed subresources on swagger for now. There is more work to be done before we can actually do this. See #1239

@dunglas dunglas merged commit 1d4690b into api-platform:master Jul 10, 2017
@dunglas
Copy link
Member

dunglas commented Jul 10, 2017

Thanks @pierredup @soyuka!

@soyuka soyuka deleted the swagger-subresource branch January 29, 2018 09:38
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
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.

None yet

4 participants