Support Guzzle 8 while keeping Guzzle 7 compatibility - #880
Conversation
mokuzon
changed the title
0f6da5c to
74c6695
Compare
a9ae9ca to
55acb61
Compare
| $options = $this->createHttpClientOption(); | ||
| try { | ||
| $response = $this->client->send($request, $options); | ||
| } catch (RequestException $e) { |
There was a problem hiding this comment.
Why this catch structure changed: Guzzle 8 removed RequestException::getResponse(), which the previous code relied on. The response-carrying exceptions were moved to a new ResponseException subclass:
Guzzle 7 Guzzle 8
──────────────────────────────── ────────────────────────────────
TransferException TransferException
├── RequestException ├── RequestException
│ │ getResponse(): ?Response │ │ (getResponse() removed!)
│ └── BadResponseException │ └── ResponseException (new)
│ 4xx/5xx, response │ │ getResponse(): Response
│ guaranteed │ └── BadResponseException
└── ConnectException └── NetworkException (new)
└── ConnectException
There was a problem hiding this comment.
If you want the equivalent exception, BadResponseException is not the right one. You should still catch RequestException and check if it's an instance of ResponseException to see if you can get the response headers or body.
There was a problem hiding this comment.
Thanks, fixed in 22eb61a.
It now catches RequestException again, but guards with method_exists($e, 'getResponse') instead of instanceof ResponseException, since ResponseException doesn't exist in Guzzle 7 and this SDK supports both majors: the guard is true for every RequestException on Guzzle 7 and for the ResponseException subtree on Guzzle 8.
| if (!$exception instanceof BadResponseException) { | ||
| throw new ApiException( | ||
| "[{$exception->getCode()}] {$exception->getMessage()}", | ||
| (int) $exception->getCode(), | ||
| null, | ||
| null | ||
| ); | ||
| } |
There was a problem hiding this comment.
The original callback called $exception->getResponse() unconditionally, but rejections without a response (e.g. ConnectException, which has no getResponse()) made this crash with a fatal Error even on Guzzle 7 as a bug.
This branch converts such failures to a response-less ApiException, exactly as the sync path's catch (ConnectException) has always done.
| $options = $this->createHttpClientOption(); | ||
| try { | ||
| $response = $this->client->send($request, $options); | ||
| } catch (RequestException $e) { |
There was a problem hiding this comment.
If you want the equivalent exception, BadResponseException is not the right one. You should still catch RequestException and check if it's an instance of ResponseException to see if you can get the response headers or body.
2e1ace6 to
41e6f91
Compare
Update the api.pebble template for two Guzzle 8 breaking changes: - Replace GuzzleHttp\Utils::jsonEncode(), removed in Guzzle 8, with native json_encode() using JSON_THROW_ON_ERROR. - Stop calling RequestException::getResponse(), which was moved to ResponseException in Guzzle 8. Catch BadResponseException for error responses instead, which carries the response in both Guzzle 7 and 8, and TransferException for failures without a response such as connection errors (both in sync and async paths). Also widen the guzzlehttp/guzzle constraint to ^7.3 || ^8.0. orchestra/testbench (via laravel/framework, which still requires guzzle ^7.8.2) cannot be installed together with guzzle ^8.0 only, which made composer install unresolvable and broke CI.
Generated by python3 generate-code.py from the updated api.pebble template. No manual edits.
Review feedback from the Guzzle maintainer: catching BadResponseException is not equivalent to the pre-Guzzle-8 behavior. Response-carrying failures that are not 4xx/5xx errors, such as TooManyRedirectsException (Guzzle 7: extends RequestException, Guzzle 8: extends ResponseException), would lose their response headers and body. Go back to catching RequestException as before, and take the response when the exception can provide one. In Guzzle 7 every RequestException has getResponse(); in Guzzle 8 only the ResponseException subtree does, so guard the call with method_exists(), which expresses exactly that availability in both majors. Apply the same rule to the async rejection callback.
Generated by python3 generate-code.py from the updated api.pebble template. No manual edits.
41e6f91 to
e72f9c2
Compare
There was a problem hiding this comment.
Could we add tests to verify that the changed code path works with both v7 and v8?
Reading the v7 and v8 code to confirm the behavior is great, but having tests would give us stronger confidence that the implementation is correct.
3 participants
Supersedes #876 (
guzzlehttp/guzzletov8).Its CI fails because
laravel/framework(via dev dependencyorchestra/testbench) still requires Guzzle^7.8.2, so pinning the root requirement to^8.0makescomposer installunresolvable.Changes
guzzlehttp/guzzleto^7.3 || ^8.0so consumers can use either major version, while dev dependencies (Laravel, still Guzzle 7 only) stay installable.api.pebble+ regenerated clientsUtils::jsonEncode()was removedjson_encode(..., JSON_THROW_ON_ERROR).RequestException::getResponse()was removed