Skip to content

Conversation

Simperfit
Copy link
Contributor

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

@Simperfit Simperfit force-pushed the feature/drop-nelmio-api-doc-bundle branch 2 times, most recently from a323fe3 to 4d45672 Compare July 14, 2016 19:38
@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

I'm -1 to drop the NelmioApiDoc bridge. It works, is stable and easy to maintain. A lot of work has been done on it. I prefer to keep it.

Does this composer trick work on Windows? When installing the lib as a dependency of another project?
What is the interest of doing this (the version is still hardcoded) instead of versioning the vendor?
If we take that path, the cleaner solution is to add a package.json file in the standard edition to retrieve the JS lib, but I prefer to have something working out of the box without requiring npm, node and so on. It is just a small dev dependency.

@Simperfit
Copy link
Contributor Author

Simperfit commented Jul 14, 2016

I'm +1 on dropping the bridge or make it disabled by default.

But I tested to remove and download it, but it will be too much work for windows too. I prefer to keep it inside too.

edit: Exactly, I have worked a long time on that too, let make it false by default and drop it when there will be no support anymore.

@Simperfit Simperfit force-pushed the feature/drop-nelmio-api-doc-bundle branch 3 times, most recently from 587e2b0 to 693cf81 Compare July 14, 2016 20:11
@Simperfit Simperfit force-pushed the feature/drop-nelmio-api-doc-bundle branch from 693cf81 to 09b9e39 Compare July 14, 2016 20:15
@Simperfit Simperfit changed the title feat: drop nelmio api doc bundle feat: deactivate api-platform by default Jul 14, 2016
@Simperfit Simperfit changed the title feat: deactivate api-platform by default feat: deactivate NelmioApiDocBundle by default Jul 14, 2016
@dunglas
Copy link
Member

dunglas commented Jul 14, 2016

The bridge is already disabled by default since api-platform/api-platform#113 was merged.

@soyuka
Copy link
Member

soyuka commented Jul 14, 2016

I'm -1 to drop the NelmioApiDoc bridge. It works, is stable and easy to maintain. A lot of work has been done on it. I prefer to keep it.

+1

It is just a small dev dependency.

But it is embed inside api-platform's source code?

@teohhanhui
Copy link
Contributor

The bridge is already disabled by default since api-platform/api-platform#113 was merged.

You mean the route is not there by default...

Anyway, if we have no interest in maintaining it anymore, we should just remove it altogether. My 2c...

@dunglas
Copy link
Member

dunglas commented Jul 15, 2016

The Swagger UI integration is pretty recent and not as tested as the NelmioApiDoc integration. I propose to keep it but disabled by default and to deprecate it at the same time than NelmioApiDoc.

@soyuka
Copy link
Member

soyuka commented Jul 15, 2016

I'd keep NelmioApiDoc too. Still, it bother's me a lot having swagger-ui sources inside api-platform's source code.

@dunglas
Copy link
Member

dunglas commented Jul 15, 2016

It's just the dist. What we can do is to add a script to download automatically the last version when we do a composer update and check that we use the last version in the CI?

@soyuka
Copy link
Member

soyuka commented Jul 15, 2016

That's the only way I see to get the latest tag:

git clone --depth 1 https://github.com/swagger-api/swagger-ui/
git fetch --tags
git checkout $(git describe --tags `git rev-list --tags --max-count=1`)

Won't it be best to let the user install swagger-ui instead if he wants to? Maybe you want it inside api-platform/api-platform but IMO it's not necessary in the core.

@dunglas
Copy link
Member

dunglas commented Jul 15, 2016

+1 to have it in the standard edition but not in the core.

@dunglas dunglas merged commit 2955a93 into api-platform:master Jul 15, 2016
@dunglas
Copy link
Member

dunglas commented Jul 15, 2016

Thank you @Simperfit

@Simperfit Simperfit deleted the feature/drop-nelmio-api-doc-bundle branch August 13, 2016 06:44
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
…o-api-doc-bundle

feat: deactivate NelmioApiDocBundle by default
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