Use composer for swagger ui #768

soyuka · 2016-09-27T06:39:28Z

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

soyuka · 2016-09-27T06:47:55Z

We were unable to access the repo or commit to analyze it.

It is possible that the commit was deleted from GitHub before we had time to analyze it.

Hmmm

teohhanhui · 2016-09-27T09:28:10Z

composer.json

@@ -21,7 +21,8 @@
        "symfony/http-kernel": "^2.7 || ^3.0",
        "symfony/property-info": "^3.1",
        "symfony/serializer": "^3.1",
-        "willdurand/negotiation": "^2.0.3"
+        "willdurand/negotiation": "^2.0.3",
+        "swagger-api/swagger-ui": "^2.2"


Notice that our dependencies are sorted... 😄

teohhanhui · 2016-09-27T09:34:05Z

composer.json

@@ -67,5 +68,10 @@
        "branch-alias": {
            "dev-master": "2.0.x-dev"
        }
+    },
+    "scripts": {


scripts is root-only: https://getcomposer.org/doc/04-schema.md#scripts

So this needs to be in the Standard Edition.

Also, not everyone can make use of symlinks, so we should probably do it in a similar fashion to https://github.com/sensiolabs/SensioDistributionBundle/blob/master/Composer/ScriptHandler.php + https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

Seems a bit overkill but yeah might be better, we need to find and validate a neat solution before I can implement it. ping @api-platform/core-team

~~Btw this works on both my osx and my debian without being root.~~
Oh ok root-only where the root is the first composer.json. Still, I'm using directly api-platform/core, so it is my root :p.

I'd add that copying isn't the best solution in term of space needed as vendor files are then useless and take 5.4M for nothing.

Unless we want to use git submodules, I don't think there's any more elegant solution.

@soyuka It will not work for my Docker Compose set-up. I need to use copy, not symlink because of all the volumes... And then of course there's Windows. 😄

Still, I'm using directly api-platform/core, so it is my root :p

And of course it's not going to work for any actual projects using API Platform.

I'd add that copying isn't the best solution in term of space needed as vendor files are then useless and take 5.4M for nothing.

That's already the case for bundle resources. I don't think it's a concern since the user is doing it for good reasons. The default should be relative symlink, just like in Symfony.

So basically we need a Composer Script which does a cp -r vendor/swagger-api/swagger-ui/dist in src/Bridge/Symfony/Bundle/Resources/public/swagger-ui.

I'm not sure to understand how this script runs (is it magically hooked to composer?).

May I skip the Command part and directly put this in the Composer script?

No magic involved. Take a look at https://github.com/symfony/symfony-standard/blob/6832c7baae2d757b12bbd5342aea2993c21ee9eb/composer.json#L30-L45

Yes, we probably do not need a Command.

Oh there it is :), thanks!

teohhanhui · 2016-09-27T09:37:00Z

.gitignore

@@ -5,3 +5,4 @@ phpunit.xml
 /vendor/
 /tests/Fixtures/app/cache/*
 /tests/Fixtures/app/logs/*
+/src/Bridge/Symfony/Bundle/Resources/public


I don't think we should use the public directory itself directly, but rather a subdirectory public/swagger-ui.

woops my bad was in a hurry 😋

teohhanhui · 2016-09-27T10:05:37Z

composer.json

+    },
+    "scripts": {
+      "post-autoload-dump": [
+        "php -r \"symlink('../../../../../../vendor/swagger-api/swagger-ui', './src/Bridge/Symfony/Bundle/Resources/public/swagger-ui');\""


I think you meant to symlink the dist directory?

https://github.com/swagger-api/swagger-ui/tree/master/dist

Why do they publish everything?

Why do they publish everything?

Composer does not have a way to specify otherwise. 😄

You know, when you think about it, it's perfectly understandable because Composer simply clones the repository or download the archive from GitHub.

Yeah, like bower (they still have an ignore option in the bower.json)... I think what npm achieves is better in term of deployments but it's another debate. For example, I never received a correct answer on this part of a recent bash deployment code of mine:

is having a tag that refers to an overridden commit a good idea?

soyuka · 2016-09-28T07:59:31Z

ping @teohhanhui

The composer.json update should be in the api-platform/api-platform repository if I understand correctly.

dunglas · 2016-09-28T09:43:47Z

src/Composer/ScriptHandler.php

+
+class ScriptHandler
+{
+    protected static $destination = '/src/Bridge/Symfony/Bundle/Resources/public/swagger-ui/dist';


why not a using a const for those parameters?

The logic looks bad here. Swagger UI must be installed:

when downloading the standard edition

when installing the bundle in an existing Symfony project

Currently, it doesn't address these use cases. IMO we should take inspiration from https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php and copy directly files in the web directory (not in the bundle).

Not sure to follow here. It was in public so I put it back in public so that it works seamlessly.

From what I understand the assets command copies content from public to web. So, you want files to be copied to web/?

What are you calling standard edition? The api-platform/api-platform package? composer create-package will run composer install and will run this if we have the same scripts lines in it's composer.json.

when installing the bundle in an existing Symfony project

How should this work? A composer event? Related to an existing symfony command?

Please give me more informations about your needs:

Where should the dist directory content be copied to?

How is it supposed to be triggered (Symfony Command, Composer Event etc.)

@dunglas Waiting on an answer to this one, I'm not sure what should be done. Please give me strict instructions and it'll be done by the end of the week ;).

The way I see it, we should not run the composer script in api-platform/core. It should only be run in api-platform/api-platform (and any Symfony project that uses API Platform).

The files should be copied to the web directory of the Symfony project, i.e. web/swagger-ui.

So, that means all asset('bundles/apiplatform/swagger-ui/ calls in the template would be changed to asset('swagger-ui/.

What about people using directly api-platform/core who would like to benefit from swagger-ui then?

I would left the script in api-platform/core, and then I'd set the composer post-install-cmd inside api-platform/api-platform. This is how it's done with symfony I think. For example in my project's composer.json I have:

"scripts": { "post-install-cmd": [ "Incenteev\\ParameterHandler\\ScriptHandler::buildParameters", "Sensio\\Bundle\\DistributionBundle\\Composer\\ScriptHandler::buildBootstrap", ... ], "post-update-cmd": [ ... ] },

Then ofc, +1 about the web directory.

Ok, I read again what you wrote and I think we said the same thing. I'm doing just that asap.

dunglas · 2016-09-28T09:47:40Z

src/Composer/ScriptHandler.php

+    public static function installSwaggerUi(Event $event)
+    {
+        $fs = new Filesystem();
+        $finder = new Finder();


This variable is used only one time, it can be inlined.

Inline a constructor?

foreach ((new Finder())->in(...

oh didn't knew about this, thanks.

teohhanhui

Also, we can remove update-js.sh

teohhanhui · 2016-09-28T09:56:33Z

composer.json

@@ -21,6 +21,7 @@
        "symfony/http-kernel": "^2.7 || ^3.0",
        "symfony/property-info": "^3.1",
        "symfony/serializer": "^3.1",
+        "swagger-api/swagger-ui": "^2.2",


sw goes before sy 😝

teohhanhui · 2016-09-28T10:09:53Z

src/Composer/ScriptHandler.php

+class ScriptHandler
+{
+    protected static $destination = '/src/Bridge/Symfony/Bundle/Resources/public/swagger-ui/dist';
+    protected static $source = '/vendor/swagger-api/swagger-ui/dist';


$swaggerUiSource?

teohhanhui · 2016-09-28T10:10:17Z

src/Composer/ScriptHandler.php

+
+class ScriptHandler
+{
+    protected static $destination = '/src/Bridge/Symfony/Bundle/Resources/public/swagger-ui/dist';


$swaggerUiDestination?

Also, we do not want the extraneous dist here.

teohhanhui · 2016-09-29T05:43:20Z

src/Composer/ScriptHandler.php

+
+class ScriptHandler
+{
+    const DESTINATION = '/src/Bridge/Symfony/Bundle/Resources/public/swagger-ui';


So this ends up as ScriptHandler::DESTINATION... The const name is not descriptive enough.

dunglas · 2016-10-12T18:03:43Z

What is the status of this PR?

teohhanhui · 2016-10-20T09:51:12Z

This needs a rebase.

soyuka · 2016-10-20T13:47:28Z

rebase broke everything \o/ (because of Fosuserbundle update?).

teohhanhui · 2016-10-24T10:02:02Z

@soyuka So you mean the test failures are unrelated? If that's the case I don't see why we shouldn't proceed with this.

Edit: already fixed in #813

teohhanhui · 2016-10-24T10:12:07Z

composer.json

@@ -17,6 +17,7 @@

        "doctrine/inflector": "^1.0",
        "psr/cache": "^1.0",
+        "swagger-api/swagger-ui": "^2.2",


Is this still a dependency? I think not. Can it go under suggest?

Is it a dependency of api-platform/api-platform then?

teohhanhui · 2016-10-24T10:15:45Z

src/Bridge/Symfony/Bundle/Resources/views/SwaggerUi/index.html.twig

@@ -81,7 +84,7 @@
 <body class="swagger-section">
 <div id="header" style="background-color: #253032; height: 35px">
    <div class="swagger-ui-wrap">
-        <a id="logo" href="https://api-platform.com"><img height="48" src="{{ asset('bundles/apiplatform/logo-header.svg') }}" alt="API Platform" style="position: relative; right: 10px"></a>
+        <a id="logo" href="https://api-platform.com"><img height="48" src="{{ asset('logo-header.svg') }}" alt="API Platform" style="position: relative; right: 10px"></a>


Accidentally changed...

teohhanhui · 2016-10-24T10:16:55Z

src/Composer/ScriptHandler.php

+
+class ScriptHandler
+{
+    const SWAGGER_UI_DESTINATION = '/web/swagger-ui';


The path separator (leading slash) should not be in these constants.

Hmm, to me it's cleaner then adding DIRECTORY_SEPARATOR below:

$cwd.DIRECTORY_SEPARATOR.self::SWAGGER_UI_SOURCE

meyerbaptiste · 2016-10-25T23:16:06Z

src/Composer/ScriptHandler.php

+        $fs = new Filesystem();
+        $cwd = getcwd();
+
+        foreach ((new Finder())->in(implode('/', [$cwd, self::SWAGGER_UI_SOURCE])) as $file) {


What do you think about something like that?

class ScriptHandler { protected static $symfonyWebDir = 'web'; public static function installSwaggerUi(Event $event) { $vendorDir = $event->getComposer()->getConfig()->get('vendor-dir'); $webDir = $event->getComposer()->getPackage()->getExtra()['symfony-web-dir'] ?? static::$symfonyWebDir; $event->getIO()->write('Trying to install assets for Swagger-UI...'); (new Filesystem())->mirror( $webDir.'/swagger-api/swagger-ui/dist', $vendorDir.'/swagger-ui', null, ['override' => true, 'delete' => true] ); } }

It should work, isn't it?

cc @dunglas @teohhanhui

We should pass 'copyonwindows' => true to the options.

soyuka · 2016-11-15T08:57:14Z

Made a new commit for the throw thing, the diff is a mess otherwise.

soyuka · 2016-11-15T09:10:36Z

tests/Bridge/Symfony/Bundle/Action/SwaggerUiActionTest.php

@@ -52,9 +54,6 @@ public function testInvoke(Request $request, ProphecyInterface $twigProphecy)

    public function getInvokeParameters()
    {
-        $postRequest = new Request([], [], ['_api_resource_class' => 'Foo', '_api_item_operation_name' => 'post']);
-        $postRequest->setMethod('POST');


This was not used

soyuka · 2016-11-15T09:11:15Z

src/Bridge/Symfony/Bundle/Action/SwaggerUiAction.php

@@ -50,6 +51,10 @@ public function __invoke(Request $request)
    {
        $documentation = new Documentation($this->resourceNameCollectionFactory->create(), $this->title, $this->description, $this->version, $this->formats);

+        if (!file_exists($request->server->get('DOCUMENT_ROOT').'/swagger-ui')) {
+            throw new RuntimeException('Swagger-ui was not found, either you forgot to require swagger-ui or you might be missing the composer scripts. Please refer to https://api-platform.com/docs/core/swagger-ui for instructions.');
+        }


I think DOCUMENT_ROOT is the safest way to get the web path, or we might add a configuration for the swagger-ui path.

It should be in the config.

okay, so same goes for the Composer\ScriptHandler where we use $event->getComposer()->getPackage()->getExtra()['symfony-web-dir']?

Composer script using Symfony project config seems wrong to me... I think we should stick to the "extra" in the ScriptHandler.

okay that's what I've done so far.

soyuka · 2016-11-15T11:34:46Z

Okay, so I did a cleaner commit with the modifications, the other one is just deleting stuff.

soyuka · 2016-11-15T14:25:32Z

Waiting for insights on #841 (comment) and I'm adding it here. I think that this is close to be good to go (ie: we will finally be able to grep without the swagger mess \o/).

GuilhemN · 2017-01-08T10:20:51Z

What is the problem with the current implementation?
This change is bad for DX imo as it complexify the use of Swagger-Ui; currently you only have to add the swagger routes, but with this change you'll have to:

Register the ScriptHandler in your composer.json file
Run composer require swagger-api/swagger-ui
Then add the routes

dunglas · 2017-01-08T10:33:41Z

I tend to agree with @GuilhemN. The only problem I see with the current implementation is that we need to upgrade the bundle when a the Swagger team releases a new version of Swagger UI. It's not a big deal IMO.

soyuka · 2017-01-08T11:28:12Z

I see several problems:

a pain to keep up to date
the whole swagger library is committed with their vendors which is painful while issuing grep on the library especially since it's in the src path
it increases the library size
all those files aren't bundled, it looks really messy to me. Why should we keep the swagger mess on this so clean library?

If it were only up to me I would completely move Swagger to another bundle, I personally don't use it and it bother's me when doing greps or when parsing every file with php tools. Those are recurrent DX issues, if you just have to add the ScriptHandler, it's a one time thing. Also if you don't want swagger-ui it's your choice.

Then add the routes

Why? We can add the routes even if the ui isn't available.

GuilhemN · 2017-01-08T11:39:12Z

a pain to keep up to date

It can be automated if it's too painful.

the whole swagger library is committed with their vendors which is painful while issuing grep on the library especially since it's in the src path

it increases the library size

all those files aren't bundled, it looks really messy to me. Why should we keep the swagger mess on this so clean library?

Imo this matters less than an easy installation as it is the first thing that the user sees of the library. If we can have both it would be better but it looks like it will be hardly possible without loosing on one side.

If it were only up to me I would completely move Swagger to another bundle

It would be fine for me.

Why? We can add the routes even if the ui isn't available.

The point is only that there are 3 steps, the worse being adding the ScriptHandler imo as people are not used to this kind of action.

soyuka · 2017-01-08T14:02:30Z

The point is only that there are 3 steps, the worse being adding the ScriptHandler imo as people are not used to this kind of action.

Symfony does add its own hooks to composer when using the bundle. Couldn't we do the same with api-platform/api-platform (there's already a PR for that in the repo)? You'd only need to add the ScriptHandler if you're using directly api-platform/core and when you want swagger-ui.

GuilhemN · 2017-01-08T14:11:59Z

Indeed, I forgot Api-Platform had its own edition. Then the only problem is when installing api-platform/core independantly.
Is it a big deal @dunglas?

dunglas · 2017-01-08T14:27:10Z

It needs to be properly documented. The good thing with the actual solution is that it just works, in all cases.

teohhanhui · 2017-06-08T08:09:35Z

@soyuka We can close this, right? And api-platform/api-platform#150.

soyuka · 2017-06-13T06:16:33Z

Sure, even though I still think swaggerui should not be in api-platform/core 😸

teohhanhui suggested changes Sep 27, 2016

View reviewed changes

soyuka force-pushed the move-swagger branch from 4277a42 to 0185c36 Compare September 27, 2016 09:54

teohhanhui mentioned this pull request Sep 27, 2016

Add Swagger 2.0 support #538

Closed

soyuka force-pushed the move-swagger branch 2 times, most recently from edcc5c9 to e64bb6e Compare September 27, 2016 09:58

teohhanhui reviewed Sep 27, 2016

View reviewed changes

teohhanhui added the enhancement label Sep 28, 2016

soyuka force-pushed the move-swagger branch 4 times, most recently from 3c934c5 to 6185867 Compare September 28, 2016 07:56

dunglas requested changes Sep 28, 2016

View reviewed changes

teohhanhui suggested changes Sep 28, 2016

View reviewed changes

soyuka force-pushed the move-swagger branch 2 times, most recently from 4b462e4 to 28f68e2 Compare September 28, 2016 10:53

teohhanhui reviewed Sep 29, 2016

View reviewed changes

soyuka force-pushed the move-swagger branch from 28f68e2 to 477a98f Compare September 30, 2016 06:29

soyuka force-pushed the move-swagger branch 2 times, most recently from f182602 to 6765dd1 Compare October 13, 2016 14:33

soyuka mentioned this pull request Oct 13, 2016

Add scripts for swagger-ui api-platform/api-platform#150

Closed

soyuka force-pushed the move-swagger branch from 6765dd1 to d6a767c Compare October 17, 2016 07:16

soyuka force-pushed the move-swagger branch 2 times, most recently from 6eec2fc to 578133a Compare October 20, 2016 13:39

teohhanhui suggested changes Oct 24, 2016

View reviewed changes

meyerbaptiste reviewed Oct 25, 2016

View reviewed changes

soyuka force-pushed the move-swagger branch 2 times, most recently from 85df583 to 5b5b3f9 Compare October 27, 2016 06:18

soyuka mentioned this pull request Nov 10, 2016

fix #804 - add a parameter to disable swagger ui when not needed #841

Merged

soyuka force-pushed the move-swagger branch from 5c29c4a to 73990a3 Compare November 15, 2016 08:54

soyuka force-pushed the move-swagger branch from 73990a3 to c6d695d Compare November 15, 2016 09:10

soyuka commented Nov 15, 2016

View reviewed changes

Delete swagger ui

8cd1fc3

soyuka force-pushed the move-swagger branch 2 times, most recently from 8402c6e to 490f84a Compare November 15, 2016 11:24

Swagger composer script and configuration

a235062

soyuka force-pushed the move-swagger branch from 490f84a to a235062 Compare November 15, 2016 14:02

jakesylvestre approved these changes Mar 31, 2017

View reviewed changes

soyuka mentioned this pull request May 23, 2017

Upgrade Swagger UI to v3 (React inside) #1138

Merged

3 tasks

soyuka closed this Jun 13, 2017

soyuka deleted the move-swagger branch January 29, 2018 09:38

Use composer for swagger ui #768

Use composer for swagger ui #768

Conversation

soyuka commented Sep 27, 2016

soyuka commented Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

soyuka commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Oct 12, 2016

teohhanhui commented Oct 20, 2016

soyuka commented Oct 20, 2016 • edited Loading

teohhanhui commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

meyerbaptiste Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Nov 15, 2016 • edited Loading

soyuka commented Nov 15, 2016

GuilhemN commented Jan 8, 2017

dunglas commented Jan 8, 2017

soyuka commented Jan 8, 2017

GuilhemN commented Jan 8, 2017 • edited Loading

soyuka commented Jan 8, 2017

GuilhemN commented Jan 8, 2017

dunglas commented Jan 8, 2017

teohhanhui commented Jun 8, 2017

soyuka commented Jun 13, 2017

soyuka commented Sep 27, 2016 •

edited

Loading

soyuka Sep 27, 2016 •

edited

Loading

teohhanhui Sep 27, 2016 •

edited

Loading

soyuka Sep 27, 2016 •

edited

Loading

teohhanhui Oct 13, 2016 •

edited

Loading

dunglas Sep 28, 2016 •

edited

Loading

soyuka commented Oct 20, 2016 •

edited

Loading

teohhanhui commented Oct 24, 2016 •

edited

Loading

soyuka Oct 24, 2016 •

edited

Loading

meyerbaptiste Oct 25, 2016 •

edited

Loading

soyuka commented Nov 15, 2016 •

edited

Loading

GuilhemN commented Jan 8, 2017 •

edited

Loading