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

[HttpClient][WDT] Add missing response content #35317

Closed
wants to merge 3 commits into from

Conversation

@michaljusiega
Copy link
Contributor

michaljusiega commented Jan 12, 2020

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets #33015
License MIT
Doc PR -

DataCollector for HttpClient was added in #33015 but missing important (IMHO) thing during profiling a request to endpoint.

The PR add information about content do we get.

Michał Jusięga and others added 2 commits Jan 12, 2020
@michaljusiega michaljusiega changed the title {HttpClient][WDT] Add missing response content [HttpClient][WDT] Add missing response content Jan 12, 2020
Copy link
Member

nicolas-grekas left a comment

Unfortunately, fetching the response content breaks laziness of responses.
The correct approach is not easy to implement as it requires buffering the content while it is yield - and there is little extension point to do this easily.
I'd suggest closing here, as #33311 already exists to keep track of the need for this.

// Try to convert response to array if possible
try {
$responseContent = $response->toArray(false);
} catch (\Throwable $e) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 12, 2020

Member

never ever do this: catch throwable and silence any errors
check https://twitter.com/nikita_ppv/status/1042676176836341760 also

This comment has been minimized.

Copy link
@michaljusiega

michaljusiega Jan 12, 2020

Author Contributor

Thanks for explain with catching.

@michaljusiega

This comment has been minimized.

Copy link
Contributor Author

michaljusiega commented Jan 12, 2020

So I closing in favor of #33311 and breaking laziness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.