-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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] Update GuzzleHttp version to 7 #10585
[PHP] Update GuzzleHttp version to 7 #10585
Conversation
@@ -31,7 +31,7 @@ paths: | |||
$ref: '#/components/schemas/Foo' | |||
/pet: | |||
servers: | |||
- url: 'http://petstore.swagger.io/v2' | |||
- url: 'https://petstore.swagger.io/v2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please undo changes in this file?
We run a local instance of the petstore server, which doesn't have TLS setup: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests#how-to-add-integration-tests-for-new-petstore-samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I tried running the PHP integrationtests locally, which all failed. So the tests are not run against the actual petstore, but a locally docker instance. Maybe I can even drop the first commit entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to test it locally with a petstore server. Let me know once you've finalized all the changes and I'll do the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wing328, I reverted the change as you asked with the nice benefit of getting rid of most of the changed files as I hoped it would. Thanks for the pointer with running the petstore in a local container, the tests ran stable and without issues. If you like you can rerun them again, but as I was able to, this should not be necessary, especially if all the merge checks are green.
…nction calls by their static counterparts and updated FakeHttpClient interface to Guzzle7 client
9e04071
to
41bf436
Compare
Tested locally and the result is good:
|
@heissb2342 thanks a lot for the PR, which has been merged into master and will be included in the upcoming v5.3.0 release. |
Upgrade performed by looking at OpenAPITools/openapi-generator#10585. Omitting the catch-and-rethrow on ConnectException included in that PR as downstream can just catch a TransferException if necessary, leaving just the composer.json update and switching removed namedspaced functions to their static-method equivalents.
Fix #7869,
Updated composer.mustache to contain
guzzlehttp/guzzle ^7.3
andguzzlehttp/psr7 ^2.0
as methods of these libraries are directly used in api.mustache. AsConnectException
is no longered extendingRequestException
, it is separately caught and converted into anApiException
. References to deprecated/removed function were replaced by their appropriate replacements.Further switching to HTTPS was necessary in the .yaml file used in PHP test code generation, as all PetStore integration tests failed with
405
HTTP status code. Additionally a spin method was implemented to retry getting updated pet data for assertion.PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(5.3.0),6.0.x
@jebentier, @dkarlovi, @mandrean, @jfastnacht, @ackintosh, @ybelenko, @renepardon