Skip to content

Conversation

@Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Nov 10, 2016

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

This PR add a parameter to disable swagger ui

@Simperfit Simperfit force-pushed the feature/add-possibility-to-disable-swagger-ui branch from 92caa74 to 4435235 Compare November 10, 2016 10:56
@soyuka
Copy link
Member

soyuka commented Nov 10, 2016

+1 this can be needed in correlation with #768

@dunglas
Copy link
Member

dunglas commented Nov 10, 2016

I suggest to not register SwaggerUiListener at all if Swagger UI is deactivated instead of passing a flag in the listener's constructor. Cleaner code, and better performance.

@Simperfit Simperfit force-pushed the feature/add-possibility-to-disable-swagger-ui branch 2 times, most recently from 134f1fc to c75cc10 Compare November 10, 2016 16:32
@Simperfit
Copy link
Contributor Author

@dunglas I've made the change

@Simperfit Simperfit force-pushed the feature/add-possibility-to-disable-swagger-ui branch from c75cc10 to 6c42e66 Compare November 10, 2016 16:33

if ($config['enable_swagger_ui']) {
$loader->load('swagger-ui.xml');
$container->setParameter('api_platform.enable_swagger_ui', $config['enable_swagger_ui']);
Copy link
Member

Choose a reason for hiding this comment

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

And if $config['enable_swagger_ui'] is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not load swagger-ui, I don't understand what you mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH: We don't net to set it for the container if it's false anyway. Like we have done with enable_swagger.

@dunglas dunglas merged commit 87e2f4c into api-platform:master Nov 12, 2016
@dunglas
Copy link
Member

dunglas commented Nov 12, 2016

Thank you @Simperfit

@teohhanhui
Copy link
Contributor

teohhanhui commented Nov 15, 2016

Does this address the use case of having Swagger UI at /docs, but disabling the "hijacking" of other paths?

@dunglas
Copy link
Member

dunglas commented Nov 15, 2016

@teohhanhui yes as long as you register yourself the Swagger UI controller: #804 (comment)

@teohhanhui
Copy link
Contributor

That's not very nice for DX 😞

@Simperfit
Copy link
Contributor Author

@teohhanhui I think it's okay, not alot of people use the html encoders and if they do we will have to add a doc part about this.

What would be better ?

@Simperfit Simperfit deleted the feature/add-possibility-to-disable-swagger-ui branch November 15, 2016 07:49
@teohhanhui
Copy link
Contributor

teohhanhui commented Nov 15, 2016

Not many people use a HTML encoder, yes. But many people like myself would not want the hijacking (mostly just a marketing feature to me).

It'd be much better if the hijacking can be disabled by configuration.

I propose something like the following:

swagger_ui:
    # enabled: true
    intercept_operations: true

@soyuka
Copy link
Member

soyuka commented Nov 15, 2016

I made a swagger configuration node in #768 :

swagger:
  enabled: true
  ui_enabled: true
  ui_path: '%kernel.root_dir%/../web/swagger-ui'

I'd propose:

swagger:
  enabled: true
  ui:
    enabled: true
    path: '%kernel.root_dir%/../web/swagger-ui'
    intercept_operations: true 

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
…ility-to-disable-swagger-ui

fix api-platform#804 - add a parameter to disable swagger ui when not needed
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.

5 participants