Skip to content

Ability to disable entrypoint and /api/docs #1580

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

Merged

Conversation

athos7933
Copy link

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1568
License MIT
Doc PR api-platform/docs#365

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

It currently doesn't disable the entrypoint. The route for the entrypoint is defined here:

<route id="api_entrypoint" path="/{index}.{_format}">

@@ -87,6 +87,7 @@ public function getConfigTreeBuilder()
->end()
->booleanNode('enable_swagger')->defaultValue(true)->info('Enable the Swagger documentation and export.')->end()
->booleanNode('enable_swagger_ui')->defaultValue(class_exists(TwigBundle::class))->info('Enable Swagger ui.')->end()
->booleanNode('enable_entrypoint')->defaultTrue()->info('Enable/disable the entry-point')->end()
Copy link
Member

Choose a reason for hiding this comment

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

Enable the entrypoint for the shake of consistency

Copy link
Member

Choose a reason for hiding this comment

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

I would also create a separate flag for entrypoint and docs, (you may want the docs, but not the entrypoint for instance).

@@ -0,0 +1,14 @@
<?xml version="1.0" encoding="UTF-8" ?>
Copy link
Member

@dunglas dunglas Dec 20, 2017

Choose a reason for hiding this comment

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

Should be docs.xml.

@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch from 7583dfc to 2e30b6f Compare December 20, 2017 14:01
@athos7933
Copy link
Author

athos7933 commented Dec 20, 2017

I dont't know why some behat failed ! My changes do not impact the tests.
@dunglas I did the changes.

@soyuka
Copy link
Member

soyuka commented Dec 20, 2017

We need to merge 2.1, it's because of #1578

@Nek-
Copy link
Contributor

Nek- commented Dec 21, 2017

@athos7933 you need to rebase your work on master :)

@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch from 2e30b6f to 25b27f1 Compare December 21, 2017 10:19
@soyuka
Copy link
Member

soyuka commented Dec 21, 2017

Just merged 2.1, please rebase onn master :)

@athos7933
Copy link
Author

@soyuka Yes, i'ts already done

@Nek-
Copy link
Contributor

Nek- commented Dec 22, 2017

@athos7933 I don't know what you did wrong but what you did is not a rebase but a merge.

@@ -146,6 +146,14 @@ private function loadExternalFiles(RouteCollection $routeCollection)
{
$routeCollection->addCollection($this->fileLoader->load('api.xml'));

if ($this->container->getParameter('api_platform.enable_entrypoint')) {
Copy link
Member

Choose a reason for hiding this comment

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

Should not we inject these parameters in the constructor instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

The constructor is already really huge. Also the container is already use at many places. This seems ok.

Copy link
Contributor

@greg0ire greg0ire Jan 8, 2018

Choose a reason for hiding this comment

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

Another option is to use… options, along with the option resolver. $graphqlEnabled should go in there.

Copy link
Contributor

@Nek- Nek- Jan 8, 2018

Choose a reason for hiding this comment

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

@greg0ire it's adding a dependency. Which is not awesome.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a simple array, that's possible too

@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch 3 times, most recently from 80959ad to 2b059e8 Compare December 29, 2017 10:24
@athos7933
Copy link
Author

@dunglas I did the change you want.

@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch 2 times, most recently from db1504a to 6fb92ca Compare January 8, 2018 09:30
@athos7933 athos7933 changed the title Ability to disable entrypoint /api/docs Ability to disable entrypoint and /api/docs Jan 8, 2018
@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch 2 times, most recently from 8171c0c to 75ec721 Compare January 8, 2018 09:53
@Nek-
Copy link
Contributor

Nek- commented Jan 8, 2018

Failure is not related to this patch, it's about Symfony4 and this new line of code: https://github.com/symfony/symfony/blame/master/src/Symfony/Component/HttpFoundation/Request.php#L1855

cc @vincentchalamon

@vincentchalamon vincentchalamon mentioned this pull request Jan 8, 2018
@vincentchalamon
Copy link
Contributor

Hello @Nek- @athos7933, unit tests will be fixed in this PR: #1631

@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch 2 times, most recently from e824a56 to f77ae7c Compare January 9, 2018 11:13
@athos7933 athos7933 force-pushed the feature/ability-to-disable-entrypoint branch from f77ae7c to 7181269 Compare January 10, 2018 08:02
@athos7933
Copy link
Author

@dunglas Hi ! Waiting for review.

@dunglas dunglas merged commit 4ea214d into api-platform:master Jan 10, 2018
@dunglas
Copy link
Member

dunglas commented Jan 10, 2018

Thanks you very much @athos7933! Don't forget to open a docs PR against the master branch please :)

@athos7933 athos7933 deleted the feature/ability-to-disable-entrypoint branch January 16, 2018 12:53
@@ -146,6 +150,14 @@ private function loadExternalFiles(RouteCollection $routeCollection)
{
$routeCollection->addCollection($this->fileLoader->load('api.xml'));

if ($this->entrypointEnabled) {
$routeCollection->addCollection($this->fileLoader->load('api.xml'));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't this add 2 times the api.xml file ? :o

Copy link
Contributor

Choose a reason for hiding this comment

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

(looking at the line above)

Copy link
Member

Choose a reason for hiding this comment

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

thanks

soyuka pushed a commit to soyuka/core that referenced this pull request Feb 7, 2018
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
…o-disable-entrypoint

Ability to disable entrypoint and /api/docs
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.

8 participants