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

WIP: Add "host" and "schema" declaration to Swagger declaration #765

Conversation

dkarlovi
Copy link
Contributor

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

Please update this template with something that matches your PR

@dkarlovi
Copy link
Contributor Author

@dunglas can't fetch context from urlGenerator as the interface does not have it. Do I inject the router context or is there a better way to do it?

@@ -488,9 +488,13 @@ private function getType(string $type, bool $isCollection, string $className = n
*/
private function computeDoc(Documentation $documentation, \ArrayObject $definitions, \ArrayObject $paths) : array
{
/** @var Symfony\Component\Routing\RequestContext $requestContext */
$requestContext = $this->urlGenerator->getContext();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, but is wrong as UrlGenerator doesn't have getContext(). What would be a better way to do it? /cc @dunglas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, anyway, I'll introduce a new interface matching the RequestContext (so, getHost() and getSchene()) and inject it here.

If that sounds wrong, please let me know. /cc @dunglas

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid it will not work in CLI context (using the doc generation command).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would configuring the request context not work here as well? We fetch host and scheme here, if they return null, we throw (maybe with a hint what to do).

Copy link
Contributor

@teohhanhui teohhanhui Sep 27, 2016

Choose a reason for hiding this comment

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

In my view, it'd be undesirable to introduce yet another half-baked interface. Let's make use of Psr\Http\Message\UriInterface from PSR-7 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@teohhanhui I completely agree with this, how do you recommend to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm actually there's no need for any interface, because this is implementation-specific. So you can just inject the Symfony RequestContext.

@dkarlovi dkarlovi changed the title WIP: Add "host" declaration to Swagger declaration WIP: Add "host" and "schema" declaration to Swagger declaration Oct 4, 2016
@Simperfit
Copy link
Contributor

Any update on this ?

* @expectedException \ApiPlatform\Core\Exception\RuntimeException
* @expectedExceptionMessage The property "id" of the resource "ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy" can not be required and read-only at the same time.
*/
public function testNormalizeThrowsExceptionWithAReadOnlyAndRequiredProperty()
Copy link
Contributor

Choose a reason for hiding this comment

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

why ?

@dkarlovi
Copy link
Contributor Author

@Simperfit not currently, I had to decorate the Swagger generator with all the required functionality.

@dkarlovi
Copy link
Contributor Author

This is not going to happen in this PR, closing to avoid confusion.

@dkarlovi dkarlovi closed this Jun 27, 2017
@dkarlovi dkarlovi deleted the feature/swagger-host-declaration branch June 27, 2017 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants