[HttpClient] Various cleanups after recent changes #58969
Merged
+48
−84
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicolas-grekas
commented
Nov 22, 2024
| @@ -117,11 +117,20 @@ public function getHeaders(bool $throw = true): array | |||
|
|
|||
| public function getInfo(?string $type = null) | |||
| { | |||
| if ('debug' === ($type ?? 'debug')) { | |||
| $debug = implode('', array_column($this->info['previous_info'] ?? [], 'debug')); | |||
There was a problem hiding this comment.
the debug info for AsyncResponse should contain all debug statements from previous requests, otherwise it's difficult to debug (happened to me)
| @@ -291,6 +301,9 @@ public static function stream(iterable $responses, ?float $timeout = null, ?stri | |||
| } | |||
| } | |||
|
|
|||
| if (null === $chunk) { | |||
| throw new \LogicException(\sprintf('"%s" is not compliant with HttpClientInterface: its "stream()" method didn\'t yield any chunks when it should have.', get_debug_type($client))); | |||
There was a problem hiding this comment.
this can happen when a phpunit mock is used instead of a MockHttpClient
| if (null !== $ip = $multi->dnsCache->hostnames[$url['host'] = strtolower($url['host'])] ?? null) { | ||
| // Populate DNS cache for redirects if needed | ||
| $port = $url['port'] ?? ('http' === $url['scheme'] ? 80 : 443); | ||
| curl_setopt($ch, \CURLOPT_RESOLVE, ["{$url['host']}:$port:$ip"]); |
There was a problem hiding this comment.
CURLOPT_RESOLVE cannot be changed in the middle of a curl transaction so this is confusing dead code
| }); | ||
|
|
||
| return $previousHttpClient; | ||
| return new MockHttpClient(new MockResponse($content, ['primary_ip' => $ipAddr])); |
| // the DNS cache respectively | ||
| 'on_progress' => null, // callable(int $dlNow, int $dlSize, array $info) - throwing any exceptions MUST abort the | ||
| // request; it MUST be called on connection, on headers and on completion; it SHOULD be | ||
| // called on upload/download of data and at least 1/s |
There was a problem hiding this comment.
Reverting to the previous (before the latest CVE) signature for the progress function: it's way more seamless to expose the resolve function as an implementation detail using the info array.
12adc67 to
f2efa4d
Compare
|
Failures on Windows are new to branch 5.4, I don't have them in v5.4.47 |
|
#58915 introduced the issue on Windows 🔍 |
6b415a1 to
c444d7c
Compare
c444d7c to
01153f2
Compare
nicolas-grekas
added a commit
that referenced
this pull request
Nov 26, 2024
This PR was merged into the 7.2 branch. Discussion ---------- [HttpClient] fix amphp/http-client 5 support | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | following #58969 | License | MIT Commits ------- 06e56f8 fix amphp/http-client 5 support
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recent changes motivated by security issues made me deep dive into the code of HttpClient again.
Here are some cleanups I'd like to do on branch 5.4