Skip to content

feat(HTTPClient): Provide wrapped access to Guzzle's asyncRequest() - #38613

Merged
nickvergessen merged 1 commit into
masterfrom
feat/35959/async-guzzle-requests
Jun 28, 2023
Merged

feat(HTTPClient): Provide wrapped access to Guzzle's asyncRequest()#38613
nickvergessen merged 1 commit into
masterfrom
feat/35959/async-guzzle-requests

Conversation

@nickvergessen

@nickvergessennickvergessen commented Jun 2, 2023

Copy link
Copy Markdown
Member

Summary

Following the approach discussed in #35959 to allow making async requests, e.g. required for interaction with webhooks in a non-blocking way.

Sample usage

$client = $this->clientService->newClient();
$promise = $client->postAsync($webhook->getUrl(), $data);
$promise->then(function (IResponse$response) use ($webhook) {
if ($response->getStatusCode() !== Http::STATUS_OK && $response->getStatusCode() !== Http::STATUS_ACCEPTED) {
$this->logger->error('Webhook responded with unexpected status code, increasing error count');
}
$this->logger->debug('Webhook successfully executed');
}, function (RequestException$exception) {
$this->logger->error('Webhook error occurred, increasing error count', ['exception' => $exception]);
});

Checklist

@nickvergessennickvergessen added enhancement 2. developing Work in progress pending documentation This pull request needs an associated documentation update labels Jun 2, 2023
@nickvergessennickvergessen added this to the Nextcloud 28 milestone Jun 2, 2023
@nickvergessennickvergessen self-assigned this Jun 2, 2023
@nickvergessen

Copy link
Copy Markdown
MemberAuthor

Tests and docs will follow after my vacation.
But could get the first round of reviews on the approach in the meantime 😎

Comment threadlib/private/Http/Client/ClientService.php
@nickvergessen
nickvergessenforce-pushed the feat/35959/async-guzzle-requests branch from 8a73fc2 to f47135bCompareJune 26, 2023 10:21

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good

Comment threadlib/public/Http/Client/IPromise.php Outdated
Comment threadlib/public/Http/Client/IPromise.php Outdated
Comment threadlib/public/Http/Client/IPromise.php Outdated
@nickvergessen

Copy link
Copy Markdown
MemberAuthor

@come-nc@blizzz second review?

Comment threadlib/private/Http/Client/GuzzlePromiseAdapter.php Fixed
Comment on lines +63 to +69
if ($onFulfilled !== null) {
$wrappedOnFulfilled = static function (ResponseInterface $response) use ($onFulfilled) {
$onFulfilled(new Response($response));
};
} else {
$wrappedOnFulfilled = null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does that do?
And the wrapping around onRejected is even weirder as it seems to just pass on the call?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh it wraps the internal $response object in something from OCP, right? Then I get it, but I do not get the one for onRejected.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yeah it's about only exposing OCP.
Same basically on the rejection, we are removing the Guzzle "type hint" and just require a \Exception eating closure instead of a GuzzleHttp\Exception\RequestException one.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
nickvergessenforce-pushed the feat/35959/async-guzzle-requests branch from 8b13f2e to ec6728dCompareJune 27, 2023 13:54
@nickvergessen

Copy link
Copy Markdown
MemberAuthor

Rebased and squashed

@nickvergessen
nickvergessen merged commit fbc63fe into masterJun 28, 2023
@nickvergessen
nickvergessen deleted the feat/35959/async-guzzle-requests branch June 28, 2023 07:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developingWork in progressenhancementpending documentationThis pull request needs an associated documentation update

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Exposing GuzzleHttp\Client::asyncRequest()

5 participants

@nickvergessen@ChristophWurst@blizzz@github-advanced-security@come-nc