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

[PHP-Lumen] Lumen 5.6 support #212

Merged
merged 7 commits into from
Jun 16, 2018
Merged

[PHP-Lumen] Lumen 5.6 support #212

merged 7 commits into from
Jun 16, 2018

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 3, 2018

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: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Cherry-picking #114

Credits: @Lv-Yi

I'll update the samples tomorrow and do some tests.

@wing328 wing328 added this to the 4.0.0 milestone Jun 3, 2018
@wing328 wing328 mentioned this pull request Jun 3, 2018
4 tasks
@jimschubert jimschubert added the WIP Work in Progress label Jun 10, 2018
@jimschubert
Copy link
Member

I've added WIP label

@ackintosh
Copy link
Contributor

I'll review this PR later today. 👀

@ackintosh
Copy link
Contributor

Ran the sample locally and saw a following error:

$ php artisan serve

In web.php line 77:

  syntax error, unexpected '*', expecting end of file

web.php line 77 -> https://github.com/OpenAPITools/openapi-generator/pull/212/files#diff-e19e515da10035a7c599677e0774c708R77

It caused by Output-Formats: [*/*] in doc comment as */ means end of comment. 🤔

@ackintosh
Copy link
Contributor

The error occurs also in latest master.

git:(master*) $ php -l samples/server/petstore/php-lumen/lib/app/Http/routes.php

Parse error: syntax error, unexpected '*', expecting end of file in samples/server/petstore/php-lumen/lib/app/Http/routes.php on line 77
Errors parsing samples/server/petstore/php-lumen/lib/app/Http/routes.php

@wing328
Copy link
Member Author

wing328 commented Jun 13, 2018

@ackintosh thanks for testing it.

I would suggest a quick fix by removing that line for the time being to make the output functional first and then we'll deal with it later with escaping or other approaches.

@ackintosh
Copy link
Contributor

@wing328 Totally agree with you. I'll work for the quick fix on master branch later.

@wing328
Copy link
Member Author

wing328 commented Jun 13, 2018

@ackintosh I've just removed Output-Format from route.mustache and update the samples. Please pull the latest and run another test to see if the output looks good from your perspective.

@wing328 wing328 changed the title [WIP][PHP-Lumen] Lumen 5.6 support #114 [PHP-Lumen] Lumen 5.6 support Jun 13, 2018
@ackintosh
Copy link
Contributor

@wing328 I've added some commits and now the samples seems work fine.

@ackintosh ackintosh removed the WIP Work in Progress label Jun 13, 2018
@ackintosh ackintosh changed the base branch from master to 4.0.x June 16, 2018 05:15
@ackintosh
Copy link
Contributor

I've changed the base branch as this PR includes breaking changes.

@Lv-Yi
Copy link

Lv-Yi commented Jun 18, 2018

just fyi. in pr#343 I'm trying to add back output format by skip * / injection.

TiFu pushed a commit to TiFu/openapi-generator that referenced this pull request Aug 13, 2018
* Lumen 5.6 support

* recall headlines

* Update composer.mustache

* regenerate lumne php petstore samples

* remove output format from lumen routes

* Fix: "A facade root has not been set"

* Ignore log folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants