From aa31db648aec10789a337754f314ca7671ce0c15 Mon Sep 17 00:00:00 2001 From: Eli Wood Date: Thu, 6 Aug 2026 06:35:30 -0400 Subject: [PATCH] PMK-2865: stop leaking Guzzle's exception hierarchy processRestRequest() documented @throws GuzzleException and left the request call unguarded, so the HTTP client's exception types were part of this SDK's public contract. That coupling was total rather than partial: because the SDK sets http_errors => false and maps responses to PostmarkException itself, the transport family is the ONLY Guzzle family that can reach a caller. Guzzle 8 reclassified exactly that family, and PMK-2061 widens the constraint to allow it. Composer resolves highest, so upgrading moves callers onto Guzzle 8 whether or not they ask -- and catch (ConnectException) around a send stops matching a timeout, silently, with no code change on their side. Transport failures are now PostmarkTransportException extends PostmarkException, so existing catch (PostmarkException) keeps working and the original is on getPrevious(). isTimeout() and isConnectionFailure() let callers make retry decisions without importing anything from GuzzleHttp. All version-specific knowledge is confined to one private classifier, because the two majors are less similar than they look -- established by inspecting the installed packages rather than the docs: Guzzle 7 ConnectException covers connect AND timeout, and carries the cURL errno on getHandlerContext(), which is what separates them. Guzzle 8 ConnectException is re-parented under NetworkException, DROPS getHandlerContext() entirely, and adds ConnectTimeoutException, NetworkTimeoutException and ResponseTimeoutException instead. So errno-based classification alone would have silently degraded to "unknown" on Guzzle 8 -- caught because the tests run under both majors, not because it was predicted. The classifier matches on short class name first, falls back to errno, then to the message, and never references a class that exists in only one major. Deliberately unchanged: http_errors stays false. Flipping it would bypass the body parsing that produces Postmark's own ErrorCode/Message, and would ADD Guzzle's response-exception family to the caller surface -- the opposite of this change. Verified green under both 8.0.2 and 7.15.2: PHPStan clean, 97 tests, 0 failures. The PMK-2061 CI matrix runs both. --- CHANGELOG.md | 12 ++ .../Models/PostmarkTransportException.php | 74 +++++++++ src/Postmark/PostmarkClientBase.php | 70 ++++++++- tests/TransportExceptionTest.php | 141 ++++++++++++++++++ 4 files changed, 294 insertions(+), 3 deletions(-) create mode 100644 src/Postmark/Models/PostmarkTransportException.php create mode 100644 tests/TransportExceptionTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9634eec..01219b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,18 @@ you were catching the `TypeError` from any of the getters above as a workaround, always permitted; dropping 8.1 is the only real constraint change. ### Changed +- **BREAKING** — **transport failures are now `PostmarkTransportException`, not Guzzle exceptions.** + `processRestRequest()` previously documented `@throws \GuzzleHttp\Exception\GuzzleException`, + making Guzzle's hierarchy part of this SDK's public contract. Because the SDK disables + `http_errors` and maps responses itself, the transport family was the *only* Guzzle family that + could reach your code — so a Guzzle upgrade silently changed what your `catch` blocks matched. + Anything that never reached the API (DNS, connect, TLS, timeout, socket) is now + `Postmark\Models\PostmarkTransportException`, which extends `PostmarkException`, so existing + `catch (PostmarkException $e)` blocks keep working. The original is preserved on `getPrevious()`. + + Replace `catch (\GuzzleHttp\Exception\ConnectException $e)` with + `catch (\Postmark\Models\PostmarkTransportException $e)` and use `$e->isTimeout()` / + `$e->isConnectionFailure()` — the SDK normalises across Guzzle majors so you don't have to. - **BREAKING** — `PostmarkAttachment::fromRawData()`, `::fromBase64EncodedData()` and `::fromFile()` now declare `string` for their first two parameters and a `PostmarkAttachment` return type. Passing `null`, an array, or a non-Stringable object now raises a `TypeError`; previously it diff --git a/src/Postmark/Models/PostmarkTransportException.php b/src/Postmark/Models/PostmarkTransportException.php new file mode 100644 index 0000000..4ce5961 --- /dev/null +++ b/src/Postmark/Models/PostmarkTransportException.php @@ -0,0 +1,74 @@ +message = $message; + $this->kind = $kind; + } + + /** + * The request was not answered within the configured timeout. + * + * Retry is at-least-once, not safe-to-repeat: a timeout means no response was + * seen, which does not prove the message was rejected. + */ + public function isTimeout(): bool + { + return self::KIND_TIMEOUT === $this->kind; + } + + /** + * The connection failed before a response could be produced. + * + * Safe to retry — the request did not complete. + */ + public function isConnectionFailure(): bool + { + return self::KIND_CONNECTION === $this->kind; + } + + /** One of the KIND_* constants. */ + public function getKind(): string + { + return $this->kind; + } +} diff --git a/src/Postmark/PostmarkClientBase.php b/src/Postmark/PostmarkClientBase.php index 9ed5e52..e981f36 100644 --- a/src/Postmark/PostmarkClientBase.php +++ b/src/Postmark/PostmarkClientBase.php @@ -9,8 +9,10 @@ namespace Postmark; use GuzzleHttp\Client; +use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\RequestOptions; use Postmark\Models\PostmarkException; +use Postmark\Models\PostmarkTransportException; /** * This is the core class that interacts with the Postmark API. All clients should @@ -93,6 +95,55 @@ protected function getClient() return $this->client; } + /** + * Normalise a Guzzle transport failure to a PostmarkTransportException kind. + * + * All Guzzle-version knowledge is confined here. The two majors express the + * same failures differently: + * - Guzzle 7 raises ConnectException for connect AND timeout, and carries a + * cURL errno on the handler context, which is what separates them. + * - Guzzle 8 re-parents ConnectException under NetworkException, DROPS + * getHandlerContext(), and adds precise subclasses instead + * (ConnectTimeoutException, NetworkTimeoutException, ResponseTimeoutException). + * Matching on the short class name keeps this working under both without + * referencing classes that only exist in one. + */ + private static function classifyTransportFailure(GuzzleException $e): string + { + $shortName = substr(strrchr('\\' . get_class($e), '\\') ?: '', 1); + + // Guzzle 8: the class already says which it is. + if (in_array($shortName, ['ConnectTimeoutException', 'NetworkTimeoutException', 'ResponseTimeoutException'], true)) { + return PostmarkTransportException::KIND_TIMEOUT; + } + + // Guzzle 7: disambiguate with the cURL errno when the handler supplied one. + if (method_exists($e, 'getHandlerContext')) { + $errno = $e->getHandlerContext()['errno'] ?? null; + + if (28 === $errno) { + return PostmarkTransportException::KIND_TIMEOUT; + } + + // Resolve, connect, TLS, empty reply, send/recv. + if (in_array($errno, [5, 6, 7, 35, 52, 55, 56], true)) { + return PostmarkTransportException::KIND_CONNECTION; + } + } + + $message = strtolower($e->getMessage()); + + if (str_contains($message, 'timed out') || str_contains($message, 'timeout')) { + return PostmarkTransportException::KIND_TIMEOUT; + } + + if (in_array($shortName, ['ConnectException', 'NetworkException'], true)) { + return PostmarkTransportException::KIND_CONNECTION; + } + + return PostmarkTransportException::KIND_UNKNOWN; + } + /** * The base request method for all API access. * @@ -102,8 +153,8 @@ protected function getClient() * * @return mixed * - * @throws PostmarkException - * @throws \GuzzleHttp\Exception\GuzzleException + * @throws PostmarkException if the API returns an error response + * @throws PostmarkTransportException if the API could not be reached at all */ protected function processRestRequest($method = null, $path = null, array $body = []): mixed { @@ -142,7 +193,20 @@ protected function processRestRequest($method = null, $path = null, array $body } } - $response = $client->request($method, self::$BASE_URL . $path, $options); + try { + $response = $client->request($method, self::$BASE_URL . $path, $options); + } catch (GuzzleException $e) { + // The HTTP client's exception hierarchy is not part of this SDK's contract. + // Guzzle 8 reclassified the transport family — a plain timeout stopped being + // a ConnectException — and with HTTP_ERRORS disabled that family is the only + // one that can reach a caller, so leaking it made every consumer's catch + // block version-dependent. + throw new PostmarkTransportException( + sprintf('Could not reach the Postmark API: %s', $e->getMessage()), + $e, + self::classifyTransportFailure($e) + ); + } switch ($response->getStatusCode()) { case 200: diff --git a/tests/TransportExceptionTest.php b/tests/TransportExceptionTest.php new file mode 100644 index 0000000..e64508b --- /dev/null +++ b/tests/TransportExceptionTest.php @@ -0,0 +1,141 @@ +clientThatFailsWith( + new ConnectException('cURL error 7: Failed to connect', new Request('POST', 'test')) + ); + + try { + $client->sendEmail('a@example.com', 'b@example.com', 'Subject', null, 'Body'); + $this->fail('Expected a PostmarkTransportException.'); + } catch (PostmarkTransportException $e) { + $this->assertStringContainsString('Could not reach the Postmark API', $e->getMessage()); + $this->assertInstanceOf(ConnectException::class, $e->getPrevious()); + } + } + + /** Existing `catch (PostmarkException)` blocks must keep working. */ + public function testItIsCatchableAsAPostmarkException(): void + { + $client = $this->clientThatFailsWith( + new ConnectException('cURL error 7', new Request('POST', 'test')) + ); + + $this->expectException(PostmarkException::class); + $client->sendEmail('a@example.com', 'b@example.com', 'Subject', null, 'Body'); + } + + /** + * The point of the change: a caller can classify the failure without + * importing anything from GuzzleHttp, and gets the same answer on either + * major even though the underlying class differs. + */ + public function testCallersCanClassifyWithoutTouchingGuzzle(): void + { + $timeout = $this->captureFrom($this->connectFailure('cURL error 28: Operation timed out', 28)); + + $this->assertTrue($timeout->isTimeout()); + $this->assertFalse($timeout->isConnectionFailure()); + + $refused = $this->captureFrom($this->connectFailure('cURL error 7: Failed to connect', 7)); + + $this->assertTrue($refused->isConnectionFailure()); + $this->assertFalse($refused->isTimeout()); + } + + /** Handlers that report no errno still classify, via the message. */ + public function testClassificationFallsBackToTheMessageWithoutAnErrno(): void + { + $timeout = $this->captureFrom( + new ConnectException('cURL error 28: Operation timed out', new Request('POST', 'test')) + ); + + $this->assertTrue($timeout->isTimeout()); + } + + /** A response that arrives carrying an error is NOT a transport failure. */ + public function testApiErrorsAreStillPlainPostmarkExceptions(): void + { + $mock = new MockHandler([new \GuzzleHttp\Psr7\Response(401, [], '{}')]); + $guzzle = new Client(['handler' => HandlerStack::create($mock)]); + $client = new PostmarkClient('test-token'); + $client->setClient($guzzle); + + try { + $client->sendEmail('a@example.com', 'b@example.com', 'Subject', null, 'Body'); + $this->fail('Expected a PostmarkException.'); + } catch (PostmarkTransportException $e) { + $this->fail('A 401 is a response, not a transport failure.'); + } catch (PostmarkException $e) { + $this->assertSame(401, $e->getHttpStatusCode()); + } + } + + /** + * Guzzle 7's ConnectException accepts a handler context (which is where the + * cURL errno lives); Guzzle 8 removed that parameter and uses precise + * subclasses instead. Build whichever the installed major supports. + */ + private function connectFailure(string $message, int $errno): GuzzleException + { + $class = new \ReflectionClass(ConnectException::class); + $arguments = [$message, new Request('POST', 'test')]; + + // Built reflectively rather than with a literal 4-argument call: PHPStan + // resolves the constructor against whichever major is installed and would + // reject the wider form under Guzzle 8, even guarded. + if ($class->getConstructor()->getNumberOfParameters() >= 4) { + $arguments[] = null; + $arguments[] = ['errno' => $errno]; + } + + return $class->newInstanceArgs($arguments); + } + + private function captureFrom(GuzzleException $failure): PostmarkTransportException + { + try { + $this->clientThatFailsWith($failure) + ->sendEmail('a@example.com', 'b@example.com', 'Subject', null, 'Body'); + } catch (PostmarkTransportException $e) { + return $e; + } + + $this->fail('Expected a PostmarkTransportException.'); + } + + private function clientThatFailsWith(GuzzleException $failure): PostmarkClient + { + $guzzle = new Client(['handler' => HandlerStack::create(new MockHandler([$failure]))]); + $client = new PostmarkClient('test-token'); + $client->setClient($guzzle); + + return $client; + } +}