-
Notifications
You must be signed in to change notification settings - Fork 6k
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] Add support for async requests #5771
[PHP] Add support for async requests #5771
Conversation
run './bin/php-petstore.sh'
run './bin/security/php-petstore.sh'
@ackintosh thanks for the enhancement. |
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.
Looks good and works fine. Exceptions are working as expected.
Please just tweak @return
type annotations.
* | ||
* @param string $test_code_inject____end____rn_n_r To test code injection *_/ ' \" =end -- \\r\\n \\n \\r (optional) | ||
* @throws \InvalidArgumentException | ||
* @return void |
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.
PromiseInterface is returned here, annotation is wrong
* | ||
* @param string $test_code_inject____end____rn_n_r To test code injection *_/ ' \" =end -- \\r\\n \\n \\r (optional) | ||
* @throws \InvalidArgumentException | ||
* @return \GuzzleHttp\Promise\Promise |
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.
it returns \GuzzleHttp\Promise\PromiseInterface
@wing328 tag |
@baartosz already labelled as 2.3.0. Can you check again? |
@wing328 Sorry, I was thinking about milestone but wrote |
@baartosz no worry and thanks for reviewing the change. |
xxx(Async|AsyncWithHttpInfo) returns \GuzzleHttp\Promise\PromiseInterface
@baartosz Thanks for your comment ✨ I fixed it. |
@ackintosh I wrote some tests for this feature yesterday, please add them to this PR. Or I'll create another PR later.
|
@ackintosh thanks for the PR, which has been merged into master. @baartosz thanks for reviewing the change and adding the test cases to cover the change. |
closes #5306
PR checklist
./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)2.3.0
branch for breaking (non-backward compatible) changes.Description of the PR
(details of the change, additional tests that have been done, reference to the issue for tracking, etc)
Async requests
xxxAsync
xxxAsyncWithHttpInfo
Usage