Skip to content

fix: wrap transport failures (DNS/connect/TLS/timeout) in a catchable NetworkException - #20

Merged
wzul merged 2 commits into
mainfrom
fix/network-exception
Oct 23, 2026
Merged

wzul merged 2 commits into
mainfrom
fix/network-exception

Conversation

@wzul

@wzul wzul commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

A request that fails before any HTTP response exists — DNS failure, connection refused, TLS handshake error, timeout — raises the raw Guzzle transport exception:

try {
    $chip->purchases->get($id);          // gateway unreachable
} catch (ChipApiException $e) {          // never runs
    // ...
}
// GuzzleHttp\Exception\ConnectException escapes instead

GuzzleClient::request() only catches GuzzleClientException (4xx) and GuzzleServerException (5xx), which both extend BadResponseException. ConnectException extends TransferException and therefore bypasses the SDK's exception mapping entirely: callers must know the transport library to handle a plainly network-shaped failure, and catch (ChipApiException $e) gives a false sense of coverage.

Verified before the fix (MockHandler feeding a ConnectException): escaped as: GuzzleHttp\Exception\ConnectException (is ChipApiException? NO).

Changes

  • lib/Exception/NetworkException.php (new) — extends ChipApiException; getCode() is 0 (no HTTP status), getResponseBody() is null, and the transport exception is preserved via getPrevious().
  • lib/Http/GuzzleClient.php — a third catch (TransferException $e) after the 4xx/5xx handlers maps it to NetworkException with 'Network error: ' . $e->getMessage() and logs at error level. Ordering matters: ClientException/ServerException are caught first, so 4xx/5xx mapping is unchanged.
  • tests/NetworkExceptionTest.php (new) — three cases: the mapping (class, message, code, body, previous), the no-retry contract (exactly one attempt), and a guard that 4xx still raises its own exception type.
  • README.md — NetworkException added to the error-handling example; CHANGELOG.md — ### Added entry under [Unreleased].

Verification

  • composer test — 130 tests, 382 assertions, OK.
  • composer phpstan — no errors (level 8 + bleedingEdge).
  • composer cs-check — clean.
  • The new tests bite: with lib/Http/GuzzleClient.php reverted, testConnectFailureBecomesCatchableChipApiException and testNetworkFailureIsNotRetried fail with GuzzleHttp\Exception\ConnectException: Connection timed out / Connection refused escaping — exactly the reported leak.

Potential Risks & Review Items

  • Behaviour change for callers who caught the Guzzle exception directly. Anyone with catch (\GuzzleHttp\Exception\ConnectException $e) around an SDK call will no longer catch it, because the SDK now throws Chip\Exception\NetworkException (the Guzzle exception remains reachable through getPrevious()). That is the point of the change, but it is a visible surface change — the reason I would cut this as a minor (2.2.0), not a patch.
  • No new retries. RetryClient::shouldRetry() keys off the status code (5xx/429); NetworkException carries 0, so network failures still fail on the first attempt. This is deliberate: an error raised before a response existed does not prove the request never reached the gateway, so retrying a POST (create purchase, refund, capture) could duplicate it. Making network failures retryable is a separate decision that needs per-method idempotency analysis.
  • TransferException is broader than ConnectException. It also catches TooManyRedirectsException and any future Guzzle transport exception, so those are reported as NetworkException with code 0 rather than leaking. RequestException with a null response lands there too. I think that is the right bucket, but a reviewer may prefer an explicit allow-list.
  • Timeouts surface here as well (Guzzle maps CURLE_OPERATION_TIMEOUTED to ConnectException), so the SDK's timeout config option now produces a catchable NetworkException instead of a raw transport error.
  • No version bump or tag in this PR: the changelog entry sits under [Unreleased].

@wzul
wzul requested a review from LiyanaAmil October 23, 2026 16:37
wzul added 2 commits October 23, 2026 19:03
A DNS failure, refused connection, TLS error or timeout raised the raw
GuzzleHttp exception, so catch (ChipApiException $e) never saw it and the
caller had to know the transport library. GuzzleClient now catches
TransferException and throws Chip\Exception\NetworkException (code 0,
getResponseBody() null, transport exception preserved as the previous).

Network failures are deliberately not retried: an error raised before a
response existed does not prove the request never reached the gateway, so
retrying a POST could duplicate it.

Tests cover the mapping, the no-retry contract (one attempt), and that
4xx/5xx keep their own exception types.
@wzul
wzul force-pushed the fix/network-exception branch from c6e601f to 4242cea Compare October 23, 2026 17:05
@wzul
wzul merged commit 1df9381 into main Oct 23, 2026
7 checks passed
@wzul
wzul deleted the fix/network-exception branch October 23, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants