Skip to content

Conversation

@dragonmantank
Copy link
Contributor

@dragonmantank dragonmantank commented May 18, 2020

This upgrade Laravel from 5.6 to 7.x. 5.6 is no longer supported. I updated the source code from the main laravel/laravel repository, and updated the supported files for new/missing files. I also updated the pet store examples.

See also #2562

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@dragonmantank dragonmantank changed the title Upgrade php laravel 7 Upgrade php-laravel to 7.x May 18, 2020
@dragonmantank
Copy link
Contributor Author

Do I notify the technical committee by just tagging them in the PR request directly? I don't seem to be able to actually assign formal reviewers.

@wing328
Copy link
Member

wing328 commented May 18, 2020

Yes please tag them

@dragonmantank
Copy link
Contributor Author

Pinging @jebentier, @mandrean, @jfastnacht, @ackintosh, @ybelenko, @renepardon for a review please!

Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

Sorry for a late response. PR branch is pretty outdated, please rebase to latest master and resolve merge conflicts.
Also please update required PHP version there

and there:

Everything else looks pretty good! 👍

@ybelenko
Copy link
Contributor

Related issue #6522

@ybelenko ybelenko changed the title Upgrade php-laravel to 7.x [php-laravel] Upgrade php-laravel to 7.x Jun 11, 2020
@wing328
Copy link
Member

wing328 commented Jun 15, 2020

@dragonmantank can you please resolve the merge conflicts when you've time?

@dragonmantank
Copy link
Contributor Author

@wangzw @ybelenko I rebased on the newest master, but I'm not sure exactly how to regenerate the sample application. I've created a yaml file, but the old petstore generators look like they've been replaced with generate-samples.sh, and the contrib docs don't tell how to run it properly.

@dragonmantank dragonmantank force-pushed the upgrade-php-laravel-7 branch from 727bb54 to 940660a Compare June 28, 2020 01:22
@ybelenko
Copy link
Contributor

@dragonmantank I've added generation config, you can launch it with:

$ bin/generate-samples.sh bin/configs/php-laravel*

@ybelenko
Copy link
Contributor

@dragonmantank One more thing, don't forget to delete samples folder samples/server/petstore/php-laravel completely before new samples generation or make sure you deleted tests folder. By default generator doesn't overwrite tests, so you need to erase them manually, generate again and commit changes to pass CI.

@dragonmantank dragonmantank force-pushed the upgrade-php-laravel-7 branch from 9ad8ce4 to fcec819 Compare June 29, 2020 00:47
@dragonmantank
Copy link
Contributor Author

@ybelenko Most of the CI things should be cleared up now, except for CircleCI. It's complaining about one of the auto-generated files from the sample, but I'm not 100% sure what the appropriate fix is for it. Any help would be appreciated.

@ybelenko
Copy link
Contributor

Frankly, I don't know why bin/generate-samples.sh and bin/generate-samples.sh bin/configs/php-laravel* gives different output, but I hope that my latest commit will pass CI and we can move on with Laravel upgrade.

@ybelenko
Copy link
Contributor

I didn't check how generated Laravel server works, but I hope that @dragonmantank checked it locally with at least few API calls. Now it's time to finally merge it.

@ybelenko ybelenko merged commit d433c2d into OpenAPITools:master Jun 30, 2020
@dragonmantank
Copy link
Contributor Author

Thank you! If there's anything else I can do to help make this smoother next time, let me know!

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.

3 participants