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

If docs are disabled, do not try to add a link header to the docs #1731

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

Taluu
Copy link
Contributor

@Taluu Taluu commented Feb 23, 2018

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

Even if the docs are disabled, the AddLinkHeader listener tries to add a Link header to the api_doc route. But the generator fails if the route does not exists, triggers a 500 instead of having an expected result.

Checking for the parameter %api_platform.enable_docs% solves the problem.

@Taluu Taluu force-pushed the fix-api-doc-deactivated-header branch 2 times, most recently from e9c22a2 to a15f7fa Compare February 23, 2018 16:56
@Taluu Taluu changed the title Ignore RouteNotFoundException if api_docs is disabled If docs are disabled, do not try to add a link header to the docs Feb 23, 2018
@dunglas
Copy link
Member

dunglas commented Feb 23, 2018

Hydra cannot work if the docs aren't available. It doesn't make sense to disable the docs without disabling Hydra.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 23, 2018

Not if you have your own docs (like something built with slate, postmann, confluence, ...). It's just that the swagger docs are disabled.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2018

Maybe what could be done would be to add a parameter or something that points to the doc, which defaults to the generation of the api_doc url ?

@Nek-
Copy link
Contributor

Nek- commented Feb 26, 2018

Well the documentation is a feature of hydra. If you decide to remove that feature it should be supported everywhere in api-platform. Or it should throw an exception because of incoherent configuration.

@dunglas
Copy link
Member

dunglas commented Mar 2, 2018

IMO we must never disable the Hydra docs (or throw) when JSON-LD is enabled. The hydra "doc" is mandatory for JSON-LD (because we use Hydra types everywhere in JSON-LD documents) and Hydra compliance.

@@ -22,6 +22,7 @@

<service id="api_platform.hydra.listener.response.add_link_header" class="ApiPlatform\Core\Hydra\EventListener\AddLinkHeaderListener">
Copy link
Member

Choose a reason for hiding this comment

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

It would be cleaner to remove the service if this flag is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, but wouldn't this be in opposition to the fact that hydra needs this header ? It's the same thing as what I proposed, but still...

@Taluu Taluu force-pushed the fix-api-doc-deactivated-header branch from a15f7fa to 42d2efd Compare March 2, 2018 10:54
@Taluu
Copy link
Contributor Author

Taluu commented Mar 2, 2018

@dunglas But is it obligatory to use swagger for that ? Can't we use something else, and thus not depend on an api_doc route ?

@Nek-
Copy link
Contributor

Nek- commented Mar 2, 2018

@dunglas if you disable the documentation it looks pretty normal to disable it from everywhere. A notice may be added to the documentation. For example, I have a fully private application and I don't want to expose any documentation of the service.

Of course, if the documentation would be managed with rights you may make a point to us (we actually have developed a similar system so the front can discover some services). But as it's dangerous, adding the possibility to fully delete any feature related to the doc is relevant.

@dunglas
Copy link
Member

dunglas commented Apr 3, 2018

After a second look, it looks ok to have partial Hydra support, so 👍 on my side, but can you please add a test?

@Taluu Taluu force-pushed the fix-api-doc-deactivated-header branch from 42d2efd to 862db7e Compare April 4, 2018 11:32
@Taluu
Copy link
Contributor Author

Taluu commented Apr 4, 2018

test added @dunglas (and I fixed the ContainerBuilder parameter I added that wasn't called on registerJsonLdConfiguration)

@dunglas dunglas merged commit d808c6b into api-platform:2.2 Apr 4, 2018
@dunglas
Copy link
Member

dunglas commented Apr 4, 2018

Thanks @Taluu

@Taluu Taluu deleted the fix-api-doc-deactivated-header branch April 4, 2018 12:01
@insekticid
Copy link
Contributor

Found this bug today too. I have Symfony web page and I do not want to expose any info about api platform outside the world via header Link so I disabled docs and bug appeared.

Waiting for new Api Platform Release @dunglas + have a look at my pull in Symfony PropertyInfo\DoctrineExtractor

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