[PHP][LARAVEL] initial PHP-laravel integration #574

renepardon · 2018-07-16T09:44:43Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I've created the server stub generation for PHP laravel as requested in this ticket: swagger-api/swagger-codegen#8417

And wrote with @wing328 about this. Hopefully everything is implemented as expected. In the future I will maintain the template and also add some more laravel specific stuff like Request-Classes and models.

wing328 · 2018-07-16T10:00:37Z

@renepardon thanks for the PR.

I've cc'd the technical committee below for review:

@jebentier (2017/07) @dkarlovi (2017/07) @mandrean (2017/08) @jfastnacht (2017/09) @ackintosh (2017/09)

ackintosh · 2018-07-16T10:04:10Z

👀

ackintosh · 2018-07-16T10:33:23Z

.gitignore

@@ -96,6 +96,9 @@ samples/client/petstore/silex/SwaggerServer/venodr/
 samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/cache/
 samples/server/petstore/php-symfony/SymfonyBundle-php/Tests/logs/

+#PHP-laravel
+samples/server/petstore/php-laravel/node_modules
+samples/server/petstore/php-laravel/vendor


This line is unnecessary as 'vendor' is ignored here.

ackintosh · 2018-07-16T10:38:12Z

...napi-generator/src/main/java/org/openapitools/codegen/languages/PhpLaravelServerCodegen.java

+import org.slf4j.LoggerFactory;
+
+
+public class PhpLaravelServerCodegen extends AbstractPhpCodegen implements CodegenConfig {


implements CodegenConfig is unnecessary as AbstractPhpCodegen implements CodegenConfig.

ackintosh · 2018-07-16T11:03:17Z

I got the error below:

$ pwd
~/openapi-generator/samples/server/petstore/php-laravel/lib
$ php composer.phar install

Am I doing something wrong? (I'm new to laravel 💦 )

renepardon · 2018-07-16T11:05:46Z

@ackintosh ok, have to add more laravel specific files. Will do so. The additional vendor/* will be removed within .gitignore and CodegenConfig implementation is also removed with next commit.

ackintosh · 2018-07-16T11:09:47Z

As adding a new generator isn't breaking changes, let's change the target branch 4.0.x -> master. 💡

renepardon · 2018-07-16T11:16:15Z

Ok, will solve the conflict as well with the new push. Changed PR to master already.

renepardon · 2018-07-16T11:30:51Z

@ackintosh i've encountered a problem. Maybe you had this one before with other generators for PHP:

This is part of a generated *.php file:

/**
 * post fakeOuterBooleanSerialize
 * Summary: 
 * Notes: Test serialization of outer boolean types
 * Output-Formats: [*/*]
 */

Problem is, that the output format seems to be undefined and therefore it breaks the PHP comment.

ackintosh · 2018-07-16T11:43:47Z

Oh, I noticed that your PR is based on 4.0.x branch. so the PR includes some enhancements(contains breaking changes) which should not merge into master yet. 💦 (e.g. #212)

Quick idea: Could you create a new PR based on master, and cherry pick 44f34cc ?

ackintosh · 2018-07-16T11:43:52Z

/**
 * post fakeOuterBooleanSerialize
 * Summary: 
 * Notes: Test serialization of outer boolean types
 * Output-Formats: [*/*]
 */

Please refer to here (PhpSlimServerCodegen) to resolve the problem. 💡

jmini · 2018-07-16T11:54:46Z

As adding a new generator isn't breaking changes, let's change the target branch 4.0.x -> master

This PR was started from 4.0.x, so it can not be merged to master.

Do you need help with sorting out the commits?
Here is how I would to it:

Create a new branch origin/master
Cherry pick 44f34cc in your new branch
Force push your new branch to override your php-laravel branch on your GitHub fork.

This will work, unless you need some changes from 05f177b

renepardon · 2018-07-16T12:23:57Z

@jmini thank you for your information. I've updated the source, created a new branch called php-laravel-master out of the master branch, cherry-picked my two commits and pushed the changes. A force-push resulted in a build failure (as you can see above) and a message telling me:

$ git push -f fork php-laravel
Everything up-to-date

…for those unfamilar with laravel to get started quickly

renepardon · 2018-07-16T12:59:08Z

~/Projects/openapi-generator (php-laravel-master) $ git push -f fork php-laravel
Total 0 (delta 0), reused 0 (delta 0)
To https://github.com/renepardon/openapi-generator.git

5e20451...ac8e584 php-laravel -> php-laravel (forced update)

jmini · 2018-07-16T13:03:49Z

I have fetched your two branches locally:

$ git remote add renepardon git@github.com:renepardon/openapi-generator.git
$ git fetch renepardon php-laravel-master
$ git fetch renepardon php-laravel

(the name of the my remote pointing to your fork is called renepardon in my local copy of the git repo)

I would say that your commit 16a47f1 is a good candidate for this pr.
Your last commit on the php-laravel-master branch looks strange to me. My guess is that locally, your master branch is no longer in sync with origin/master (with origin beeing the name of the remote that points to OpenAPITools/openapi-generator on GitHub).

Please force push it to your php-laravel branch (because this PR is opened for this branch).

git push -f fork 16a47f1366a628058f5a6bb54318f2f419fcabf8:php-laravel

(assuming that the name of the remote pointing to renepardon/openapi-generator on GitHub is called fork)

I have the feeling that your are really close to the goal. If you want me to do it, please tell me. If you have the option "Allow edits from maintainers" is set in GitHub, then I might be able to do it for you. But it might not be necessary, since your are so close.

For the moment you still have 4.0.0 stuff in the branch, this can not be committed to master.

renepardon · 2018-07-16T13:16:29Z

I have "Allow edits from maintainers" enabled from the very beginning :)
Thank you for all those information!

I pushed the requested commit: https://github.com/renepardon/openapi-generator/commits/php-laravel and also the samples. Everything should be done now i guess.

jmini · 2018-07-16T15:55:30Z

The PR is ready for review, from a Git point of view, it can be merged to master.
(more details about my operations to come in the next message)

jmini · 2018-07-16T16:08:27Z

So here is what I did to help you removing the 4.0.x commits from this PR.

I have fetch both branches of yours:

php-laravel
php-laravel-master

Because I later override your php-laravel branch, I made a backup of it in my personal fork:
php-laravel_original
=> Nothing is lost

This was the state:

I have taken 16a47f1 as root of my work (in order not to have the merge commit 5e20451 that created the problem).

Then I cherry-picked 7622b37 (which is now 2a11fc4)

And I have forced pushed my local HEAD to override your php-laravel (the source of this PR, I am allowed to do it on your fork with the maintainer permission you have granted).

I hope it is clear now.
if you have any questions please tell me.

As I demonstrated, nothing is lost, so I can revert my changes if you do not agree with it.

Because of the force-push, if you want to add additional to the PR branch, I recommend you to fetch the changes from your fork. And then to reset you local php-laravel branch to commit 2a11fc4 (should be fork/php-laravel if your remote is called fork).

Depending on how you have configured your git client, just doing a pull might create a not desired merge commit.

ackintosh · 2018-07-17T01:05:14Z

@jmini Thanks for the follow-up! ✨

ackintosh · 2018-07-17T01:18:19Z

@renepardon Could you fix the path configuration as /v2/pet/123 is "404 Not Found" (/api/v2/pet/123 returns response instead)?

$ php artisan serve
Laravel development server started: <http://127.0.0.1:8000>
...
...

`/v2/pet/123` is 404 👀

$ curl -v http://127.0.0.1:8000/v2/pet/123
...
> GET /v2/pet/123 HTTP/1.1
...
...
< HTTP/1.0 404 Not Found
...

`/api/v2/pet/123` returns resonse

$ curl -v http://127.0.0.1:8000/api/v2/pet/123
...
> GET /api/v2/pet/123 HTTP/1.1
...
...
< HTTP/1.1 200 OK
...
How about implementing getPetById as a get method ?

The paths is prefixed with `api`

$ php artisan route:list

+--------+----------+-------------------------------------------------+------+--------------------------------------------------------------------+------------+
| Domain | Method   | URI                                             | Name | Action                                                             | Middleware |
+--------+----------+-------------------------------------------------+------+--------------------------------------------------------------------+------------+
|        | GET|HEAD | /                                               |      | App\Http\Controllers\Controller@welcome                            | web        |
|        | PATCH    | api/v2/another-fake/dummy                       |      | App\Http\Controllers\AnotherFakeController@testSpecialTags         | api        |
|        | POST     | api/v2/fake                                     |      | App\Http\Controllers\FakeController@testEndpointParameters         | api        |
|        | GET|HEAD | api/v2/fake                                     |      | App\Http\Controllers\FakeController@testEnumParameters             | api        |
|        | PATCH    | api/v2/fake                                     |      | App\Http\Controllers\FakeController@testClientModel                | api        |
|        | PUT      | api/v2/fake/body-with-file-schema               |      | App\Http\Controllers\FakeController@testBodyWithFileSchema         | api        |
|        | PUT      | api/v2/fake/body-with-query-params              |      | App\Http\Controllers\FakeController@testBodyWithQueryParams        | api        |
|        | POST     | api/v2/fake/inline-additionalProperties         |      | App\Http\Controllers\FakeController@testInlineAdditionalProperties | api        |
|        | GET|HEAD | api/v2/fake/jsonFormData                        |      | App\Http\Controllers\FakeController@testJsonFormData               | api        |
|        | POST     | api/v2/fake/outer/boolean                       |      | App\Http\Controllers\FakeController@fakeOuterBooleanSerialize      | api        |
|        | POST     | api/v2/fake/outer/composite                     |      | App\Http\Controllers\FakeController@fakeOuterCompositeSerialize    | api        |
|        | POST     | api/v2/fake/outer/number                        |      | App\Http\Controllers\FakeController@fakeOuterNumberSerialize       | api        |
|        | POST     | api/v2/fake/outer/string                        |      | App\Http\Controllers\FakeController@fakeOuterStringSerialize       | api        |
|        | POST     | api/v2/fake/{petId}/uploadImageWithRequiredFile |      | App\Http\Controllers\PetController@uploadFileWithRequiredFile      | api        |
|        | PATCH    | api/v2/fake_classname_test                      |      | App\Http\Controllers\FakeClassnameTags123Controller@testClassname  | api        |
|        | POST     | api/v2/pet                                      |      | App\Http\Controllers\PetController@addPet                          | api        |
|        | PUT      | api/v2/pet                                      |      | App\Http\Controllers\PetController@updatePet                       | api        |
|        | GET|HEAD | api/v2/pet/findByStatus                         |      | App\Http\Controllers\PetController@findPetsByStatus                | api        |
|        | GET|HEAD | api/v2/pet/findByTags                           |      | App\Http\Controllers\PetController@findPetsByTags                  | api        |
|        | GET|HEAD | api/v2/pet/{petId}                              |      | App\Http\Controllers\PetController@getPetById                      | api        |
|        | POST     | api/v2/pet/{petId}                              |      | App\Http\Controllers\PetController@updatePetWithForm               | api        |
|        | DELETE   | api/v2/pet/{petId}                              |      | App\Http\Controllers\PetController@deletePet                       | api        |
|        | POST     | api/v2/pet/{petId}/uploadImage                  |      | App\Http\Controllers\PetController@uploadFile                      | api        |
|        | GET|HEAD | api/v2/store/inventory                          |      | App\Http\Controllers\StoreController@getInventory                  | api        |
|        | POST     | api/v2/store/order                              |      | App\Http\Controllers\StoreController@placeOrder                    | api        |
|        | DELETE   | api/v2/store/order/{order_id}                   |      | App\Http\Controllers\StoreController@deleteOrder                   | api        |
|        | GET|HEAD | api/v2/store/order/{order_id}                   |      | App\Http\Controllers\StoreController@getOrderById                  | api        |
|        | POST     | api/v2/user                                     |      | App\Http\Controllers\UserController@createUser                     | api        |
|        | POST     | api/v2/user/createWithArray                     |      | App\Http\Controllers\UserController@createUsersWithArrayInput      | api        |
|        | POST     | api/v2/user/createWithList                      |      | App\Http\Controllers\UserController@createUsersWithListInput       | api        |
|        | GET|HEAD | api/v2/user/login                               |      | App\Http\Controllers\UserController@loginUser                      | api        |
|        | GET|HEAD | api/v2/user/logout                              |      | App\Http\Controllers\UserController@logoutUser                     | api        |
|        | DELETE   | api/v2/user/{username}                          |      | App\Http\Controllers\UserController@deleteUser                     | api        |
|        | GET|HEAD | api/v2/user/{username}                          |      | App\Http\Controllers\UserController@getUserByName                  | api        |
|        | PUT      | api/v2/user/{username}                          |      | App\Http\Controllers\UserController@updateUser                     | api        |
+--------+----------+-------------------------------------------------+------+--------------------------------------------------------------------+------------+

renepardon · 2018-07-17T06:07:03Z

@jmini wow, very impressive! Thank you very much for removing this stuff.

@ackintosh this is by intention of the laravel framework. Have a look at the corresponding service provider: https://github.com/renepardon/openapi-generator/blob/2a11fc491364971480a03304cfb7090b8d81a324/modules/openapi-generator/src/main/resources/php-laravel/app/Providers/RouteServiceProvider.php#L68

The only thing we could do is, to create another routes file (e.g. generated.php) and use this for the routes generated by the codegen. But API routes are expected to have the api prefix within a laravel application. So what do you suggest to do here? @taylorotwell what do you think?

renepardon · 2018-07-17T06:13:54Z

And is there an easier way to copy a complete folder of "static" assets/files recursively from resources folder to the output folder rather than copying all the individual files?

ackintosh · 2018-07-17T09:01:28Z

this is by intention of the laravel framework.

Oh... 👀 💦

How about modifying the RouteServiceProvider ?

https://readouble.com/laravel/5.6/en/routing.html

Within this group, the /api URI prefix is automatically applied so you do not need to manually apply it to every route in the file. You may modify the prefix and other route group options by modifying your RouteServiceProvider class.

    protected function mapApiRoutes()
    {
-        Route::prefix('api')
+        Route::prefix('')
             ->middleware('api')
             ->namespace($this->namespace)
             ->group(base_path('routes/api.php'));
    }

renepardon · 2018-07-17T09:33:11Z

Yeah, we can indeed do so. So i will remove the prefix with the next commit!

Do you know why the models aren't generated? (out of model.mustache) @ackintosh

wing328 · 2018-07-18T05:30:55Z

...napi-generator/src/main/java/org/openapitools/codegen/languages/PhpLaravelServerCodegen.java

+        modelPackage = "models";
+
+        // template files want to be ignored
+        modelTemplateFiles.clear();


@renepardon please remove this line to have the model files auto-generated.

cc @ackintosh

…rator into php-laravel

…eration

renepardon · 2018-07-18T12:29:52Z

Ok, changes done. @ackintosh
@wing328 thank you for the hint with this single line which has to be removed. Works :)

Btw.:
Amazing fact: before I did the work on my workplace where I was employed until yesterday and the build took me up to 10 minutes, now on my new Macbook at home:

[INFO] openapi-generator-project 3.1.2-SNAPSHOT ........... SUCCESS [ 1.108 s]
[INFO] openapi-generator (core library) ................... SUCCESS [ 20.000 s]
[INFO] openapi-generator (executable) ..................... SUCCESS [ 5.147 s]
[INFO] openapi-generator (maven-plugin) ................... SUCCESS [ 3.556 s]
[INFO] openapi-generator-gradle-plugin (maven wrapper) .... SUCCESS [ 0.070 s]
[INFO] openapi-generator-online 3.1.2-SNAPSHOT ............ SUCCESS [ 0.985 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 31.352 s
[INFO] Finished at: 2018-07-18T14:12:32+02:00
[INFO] ------------------------------------------------------------------------

ackintosh · 2018-07-18T13:09:54Z

Wow, amazing! 😄✨

Thanks for the updates! I'll have a look.

ackintosh · 2018-07-18T13:25:54Z

The generated server works fine! but there's one thing that bothers me, the generated models have syntax error:

$ pwd
~/openapi-generator-1/samples/server/petstore/php-laravel/lib
$ php -l app/Models/AdditionalPropertiesClass.php

Parse error: syntax error, unexpected '.', expecting '{' in app/Models/AdditionalPropertiesClass.php on line 5
Errors parsing app/Models/AdditionalPropertiesClass.php

It caused by the change: 606cf90#diff-412b73e206d04f5e7da08a3f11ee8038R99 ,
what about modelPackage = "App\\Models"?

wing328 · 2018-07-18T14:04:57Z

@renepardon to shorten the build time, you may try "mvn clean package -DskipTests" and let the CIs (e.g. Travis, CircleCI, etc) run the tests later.

renepardon · 2018-07-18T14:39:44Z

@wing328 good to know. But 30s is ok. Would have helped me before on the old machine.

wing328 · 2018-07-30T07:22:34Z

Hi folks, can this be merged into master for a beta release?

renepardon · 2018-07-30T07:40:10Z

Would be nice to receive feedback here, yes :)

wing328 · 2018-07-30T14:54:13Z

@renepardon 👌

If no further question/feedback on this PR, we'll merge it into master and ask developers for feedback.

wing328 · 2018-07-31T17:07:04Z

@renepardon thanks for contributing the PHP Laravel generator. I'll send out a tweet tomorrow to promote it.

wing328 · 2018-08-01T10:03:39Z

Tweet https://twitter.com/oas_generator/status/1024596121971130369

…nAPITools#574) * feat (PHP LARAVEL) 8417: initial PHP-laravel codegen integration * feat (PHP LARAVEL) 8417: code review adjustments * feat (PHP LARAVEL) 8417: fix typos; add missing files; adjust readme for those unfamilar with laravel to get started quickly * feat (PHP LARAVEL) 8417: add sample petstore server * feat (PHP LARAVEL) 8417: adjust route service provdider and model generation

wing328 added Enhancement: New generator Server: PHP labels Jul 16, 2018

renepardon mentioned this pull request Jul 16, 2018

Create server stub generation for laravel swagger-api/swagger-codegen#8417

Open

ackintosh reviewed Jul 16, 2018

View reviewed changes

renepardon changed the base branch from 4.0.x to master July 16, 2018 11:15

renepardon added 2 commits July 16, 2018 14:18

feat (PHP LARAVEL) 8417: initial PHP-laravel codegen integration

d6226bf

feat (PHP LARAVEL) 8417: code review adjustments

6c4b67a

feat (PHP LARAVEL) 8417: fix typos; add missing files; adjust readme …

16a47f1

…for those unfamilar with laravel to get started quickly

renepardon force-pushed the php-laravel branch from 5e20451 to ac8e584 Compare July 16, 2018 12:48

jmini force-pushed the php-laravel branch from c55c188 to a1ca81f Compare July 16, 2018 15:48

feat (PHP LARAVEL) 8417: add sample petstore server

2a11fc4

jmini force-pushed the php-laravel branch from a1ca81f to 2a11fc4 Compare July 16, 2018 15:54

wing328 reviewed Jul 18, 2018

View reviewed changes

renepardon added 2 commits July 18, 2018 14:02

Merge branch 'master' of https://github.com/OpenAPITools/openapi-gene…

7c0aa8c

…rator into php-laravel

feat (PHP LARAVEL) 8417: adjust route service provdider and model gen…

606cf90

…eration

wing328 added this to the 3.2.0 milestone Jul 31, 2018

wing328 merged commit f793ac2 into OpenAPITools:master Jul 31, 2018

wing328 changed the title ~~feat (PHP LARAVEL) 8417: initial PHP-laravel codegen integration~~ [PHP][LARAVEL]: initial PHP-laravel integration Jul 31, 2018

wing328 changed the title ~~[PHP][LARAVEL]: initial PHP-laravel integration~~ [PHP][LARAVEL] initial PHP-laravel integration Jul 31, 2018

jmini mentioned this pull request Aug 12, 2018

[PHP][Laravel] Remove duplicate variable declaration #792

Merged

4 tasks

wing328 mentioned this pull request Sep 6, 2018

[PHP] Yii 2 support #974

Open

matlo607 mentioned this pull request Jan 12, 2023

[BUG][PHP-laravel] Potential security risk due to resources hosted on external CDNs #14440

Open

		import org.slf4j.LoggerFactory;


		public class PhpLaravelServerCodegen extends AbstractPhpCodegen implements CodegenConfig {

[PHP][LARAVEL] initial PHP-laravel integration #574

[PHP][LARAVEL] initial PHP-laravel integration #574

Conversation

renepardon commented Jul 16, 2018 • edited Loading

PR checklist

Description of the PR

wing328 commented Jul 16, 2018

ackintosh commented Jul 16, 2018

ackintosh Jul 16, 2018

Choose a reason for hiding this comment

ackintosh Jul 16, 2018

Choose a reason for hiding this comment

ackintosh commented Jul 16, 2018

renepardon commented Jul 16, 2018

ackintosh commented Jul 16, 2018

renepardon commented Jul 16, 2018

renepardon commented Jul 16, 2018

ackintosh commented Jul 16, 2018

ackintosh commented Jul 16, 2018 • edited Loading

jmini commented Jul 16, 2018

renepardon commented Jul 16, 2018

renepardon commented Jul 16, 2018

jmini commented Jul 16, 2018 • edited Loading

renepardon commented Jul 16, 2018

jmini commented Jul 16, 2018

jmini commented Jul 16, 2018

ackintosh commented Jul 17, 2018

ackintosh commented Jul 17, 2018 • edited Loading

/v2/pet/123 is 404 👀

/api/v2/pet/123 returns resonse

The paths is prefixed with api

renepardon commented Jul 17, 2018 • edited Loading

renepardon commented Jul 17, 2018 • edited Loading

ackintosh commented Jul 17, 2018

renepardon commented Jul 17, 2018 • edited Loading

wing328 Jul 18, 2018

Choose a reason for hiding this comment

renepardon commented Jul 18, 2018

ackintosh commented Jul 18, 2018

ackintosh commented Jul 18, 2018

wing328 commented Jul 18, 2018

renepardon commented Jul 18, 2018

wing328 commented Jul 30, 2018

renepardon commented Jul 30, 2018 • edited Loading

wing328 commented Jul 30, 2018

wing328 commented Jul 31, 2018

wing328 commented Aug 1, 2018

renepardon commented Jul 16, 2018 •

edited

Loading

ackintosh commented Jul 16, 2018 •

edited

Loading

jmini commented Jul 16, 2018 •

edited

Loading

ackintosh commented Jul 17, 2018 •

edited

Loading

`/v2/pet/123` is 404 👀

`/api/v2/pet/123` returns resonse

The paths is prefixed with `api`

renepardon commented Jul 17, 2018 •

edited

Loading

renepardon commented Jul 17, 2018 •

edited

Loading

renepardon commented Jul 17, 2018 •

edited

Loading

renepardon commented Jul 30, 2018 •

edited

Loading