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

Fix swagger basePath #725

Closed
wants to merge 1 commit into from
Closed

Fix swagger basePath #725

wants to merge 1 commit into from

Conversation

GuilhemN
Copy link
Contributor

@GuilhemN GuilhemN commented Sep 5, 2016

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


public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceClassResolverInterface $resourceClassResolver, OperationMethodResolverInterface $operationMethodResolver, OperationPathResolverInterface $operationPathResolver)
public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceClassResolverInterface $resourceClassResolver, OperationMethodResolverInterface $operationMethodResolver, OperationPathResolverInterface $operationPathResolver, RequestContext $requestContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you prefer, we can provide easily a bc layer by instantiating the request context with its default values.

Copy link
Member

Choose a reason for hiding this comment

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

We don't care about BC for now, so it's ok :)

Copy link
Member

Choose a reason for hiding this comment

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

However adding a dependency to the Symfony Router in this component is not ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arf indeed... we could have something like a proxy to the RequestContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can do this in the RouterOperationPathResolver but then resolvers result won't be consistent anymore.

Copy link
Contributor Author

@GuilhemN GuilhemN Sep 5, 2016

Choose a reason for hiding this comment

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

Or maybe better, in the normalizer:

if ($this->operationPathResolver instanceof RouterOperationPathResolver) {
    $doc['basePath'] = $this->operationPathResolver->getContext()->getBaseUrl();
}

What do you prefer?

@dunglas
Copy link
Member

dunglas commented Sep 6, 2016

An alternative (maybe cleaner) would be to allow to inject the basePath as a string in the constructor (can be useful in component context too) and introduce a new BasePathFactory in the Symfony Routing Bridge to return the base path using the router's context.
Then use this factory in the DI to inject the base path when using Symfony.

WDYT @Ener-Getick?

@dunglas dunglas mentioned this pull request Sep 6, 2016
@dunglas dunglas closed this Sep 7, 2016
@dunglas
Copy link
Member

dunglas commented Sep 7, 2016

Thanks for working on this @Ener-Getick!

@GuilhemN GuilhemN deleted the SWAGGERFIX branch September 7, 2016 17:21
@GuilhemN
Copy link
Contributor Author

GuilhemN commented Sep 7, 2016

You're welcome :)

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.

2 participants