fix(Driver): classify exceptions by SQLSTATE before the message - #270
Merged
Merged
Conversation
fix(MySQL): report the integrity violations mysql files under HY000 fix(SQLServer): classify a failed connection in create() test(Driver): cover exception mapping across all four drivers Every driver read its needle list over the message before looking at the error code, so any value the server echoes back could decide the class: a uuid containing `0800` or a table named `connections` turned a constraint violation into a ConnectionException, which made `Driver::statement()` drop the connection and re-run the statement while application code catching ConstrainException saw nothing. SQLSTATE now decides, and the needles are consulted only for the states PDO invents itself (`HY`, `IM`) — a dropped socket arrives as HY000 with the reason only in the text. This also repairs reconnect on a localized ODBC install, where the English needles matched nothing at all. Comparing the state as a string rather than casting it to an int fixes Postgres `23P01`: `(int) '23P01'` is 23, which fell below the `>= 23000` test, so exclusion violations arrived as the base StatementException. Mysql leaves two integrity violations outside class 23 — a CHECK the row failed and a required column left out of the statement — which Postgres reports as 23514 and 23502 and SQL Server as 23000. They are now matched by error number, listed one by one because their HY000 neighbours are DDL errors and coercion failures. `SQLServerDriver::create()` reaches the server to reject a version it cannot compile for, which makes it the one place a connection failure happens outside `Driver::statement()`; it let the raw PDOException escape instead of classifying it. Closes #268 Assisted-By: Claude Opus 5
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #270 +/- ##
=========================================
Coverage 95.65% 95.66%
- Complexity 2207 2230 +23
=========================================
Files 142 142
Lines 6307 6341 +34
=========================================
+ Hits 6033 6066 +33
- Misses 274 275 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
MySQL still permits HY000 message false positives, PostgreSQL omits 57P04, and SQLSTATE parsing introduces an undeclared extension dependency.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Classifies database exceptions by SQLSTATE before message heuristics, preventing incorrect reconnects.
Changes:
- Adds shared SQLSTATE extraction and classification.
- Handles driver-specific constraint and connection errors.
- Adds unit and functional regression coverage.
File summaries
| File | Description |
|---|---|
src/Driver/Driver.php |
Adds SQLSTATE helpers. |
src/Driver/MySQL/MySQLDriver.php |
Revises MySQL exception mapping. |
src/Driver/Postgres/PostgresDriver.php |
Revises PostgreSQL exception mapping. |
src/Driver/SQLite/SQLiteDriver.php |
Uses SQLSTATE class 23. |
src/Driver/SQLServer/SQLServerDriver.php |
Revises mapping and wraps connection failures. |
tests/Database/Unit/Driver/MapExceptionTest.php |
Adds cross-driver mapping tests. |
tests/Database/Functional/Driver/Common/Query/ExceptionsTest.php |
Tests false-positive prevention. |
tests/Database/Functional/Driver/MySQL/Query/ExceptionsTest.php |
Tests MySQL-specific constraints. |
tests/Database/Functional/Driver/Postgres/Connection/ConnectionExceptionTest.php |
Updates PostgreSQL connection cases. |
tests/Database/Functional/Driver/Postgres/Query/ExceptionsTest.php |
Tests exclusion constraints. |
tests/Database/Functional/Driver/SQLServer/Connection/ConnectionExceptionTest.php |
Updates SQL Server connection cases. |
tests/Database/Functional/Driver/SQLServer/Driver/DriverTest.php |
Tests connection failure wrapping. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fix(Driver): match SQLSTATE with PCRE instead of ext-ctype fix(Postgres): treat 57P04 and 57P05 as connection failures docs(SQLServer): declare the StatementException create() can now throw HY000 is a sound gate for the other drivers, whose servers always classify what they reject, but mysql files plenty of its own errors under it and their text carries table and constraint names. A DDL failure on a constraint named `connections` therefore still reached the needles and came back as a ConnectionException, disconnecting and retrying over a statement the server had already refused — the original defect, surviving inside its own fix. The server numbers its errors from 1000 upwards and leaves 2000-2999 to the client library, which is what lets the two be told apart; 4031 is listed explicitly because the server raises it to close an idle connection, and gating on the number alone would otherwise stop recognising it. `ctype_alnum()` made ext-ctype a hard requirement of every exception mapping, while composer.json asks only for ext-pdo. PCRE is already a dependency of the same method. Assisted-By: Claude Opus 5
Comments that restated the branch they sit above, or retold what the mapping used to do, are gone; what stays is what the code cannot carry — the int cast that truncates `23P01`, the reason the 57 states are listed instead of taken as a class, the three places PDO keeps a SQLSTATE, and the localized ODBC strings no needle can match. Assisted-By: Claude Opus 5
There was a problem hiding this comment.
🟡 Changes recommended
MySQL can still retry server-classified messages and non-connection client errors as connection failures.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Driver/MySQL/MySQLDriver.php:109
- The entire 2001–2099 client-error range is not limited to lost sockets. For example, errno 2014 is
CR_COMMANDS_OUT_OF_SYNCand 2031 isCR_PARAMS_NOT_BOUND; now thatgetErrno()readserrorInfo[1], statement-side occurrences enter this branch, are retried, and disconnect the session even though reconnecting cannot fix the request. Restrict this condition to client errnos that actually indicate a broken connection.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
The CR_* range was widened from getCode() to errorInfo, which let it match numbers a statement raises rather than a connect attempt: CR_COMMANDS_OUT_OF_SYNC and CR_PARAMS_NOT_BOUND became ConnectionException, so a driver holding an unbuffered result disconnected and reran the statement over a mistake the reconnect could only repeat. The range is back on getCode(), where a CR_* number appears only before any statement exists, and the losses that happen under an open session are named individually. The message fallback also has to see the SQLSTATE again: an exception carrying no errorInfo yields errno 0, and without the state from the message prefix a server-classified failure would reach the needles the same way it does in the other drivers. Assisted-By: Claude Opus 5
… connect docs(tests): correct the comments on PDO's code property and on SQLite echoing values Every other driver's create() is a pure factory, and a failed connection reaches the caller through Driver::statement(), where mapException() classifies it. The sqlserver driver connected inside create() to read the server version and so had to classify that one failure by hand, under a made-up 'CONNECT' query. Moving the version check into createPDO() puts the connection back on the shared path and removes the special case. Assisted-By: Claude Fable 5.1
…age fallback test(Driver): cover "Connection is busy" as client misuse on SQL Server The library reaches SQL Server only through pdo_sqlsrv, and the ODBC layer beneath it files every transport failure under class 08 on its own: a dropped link is 08S01, a refused connect is 08001, and even EPIPE arrives as 08S01. The needles therefore matched nothing genuine, and the bare `connection` needle turned "Connection is busy with results for another command" into a ConnectionException. That is client misuse with MARS off, and the reconnect dropped the open result set so the retry succeeded, hiding the mistake. MySQL and Postgres keep their fallback because mysqlnd and libpq report a lost socket under HY000; the ODBC driver does not. Assisted-By: Claude Fable 5.1
There was a problem hiding this comment.
🟡 Changes recommended
Lazy SQL Server version validation currently exposes inconsistent exception types, and the PR description contradicts the implemented factory behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Balanced
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
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.
🔍 What was changed
The SQLSTATE now decides which
StatementExceptionsubclass a driver raises, in all four drivers. The message needles remain only where the client library itself reports a lost socket underHY000: mysqlnd and libpq. The ODBC driver beneath pdo_sqlsrv files every transport failure under class08, so SQL Server has no message fallback at all. On MySQL the fallback is additionally gated on the absence of a server error number, because mysql files plenty of its own errors underHY000and their text carries table and constraint names.ConnectionExceptionbecause of a value the server echoed back. A uuid containing0800or a table namedconnectionswas enough, on Postgres and on SQL Server.23P01(exclusion violation) reachesConstrainException. The state was compared numerically, and(int) '23P01'is23, below the>= 23000test.ConstrainExceptionfor the two integrity violations it files underHY000rather than class 23: a CHECK the row failed (3819) and a required column left out of the statement (1364). Postgres reports the same two as23514and23502, SQL Server as23000.SQLServerDriver::create()no longer reaches the server. The version check moved tocreatePDO(), so a failed connection surfaces on the first statement and is classified bymapException()like on every other driver.HY000"Connection is busy with results for another command" is a plainStatementException. It is client misuse with MARS off; the reconnect dropped the open result set and let the retry succeed, hiding the mistake.Behaviour that changes beyond the reported bug: an error the server did classify is no longer matched against the needles, so a failure like
42P01 relation "no_such_connections_table" does not existis now a plainStatementException. It used to be aConnectionException, which madeDriver::statement()disconnect and re-run the statement.Why?
A misclassified constraint violation is not just a wrong class name. Outside a transaction
Driver::statement()disconnects and retries on aConnectionException, so a data error dropped the connection — losing temp tables, advisory locks and session settings — and application code catching the narrowerConstrainExceptionnever saw it. See #268 for how intermittent that looks in practice.The MySQL and SQL Server parts are the same defect found while verifying the first: #268 reports Postgres, but SQL Server had the identical message-before-code ordering, and MySQL leaves two violations outside class 23 entirely.
Checklist
ConnectionExceptioninside oneReview notes
PDO reports the SQLSTATE in three places that do not agree. At connect time
getCode()holds the driver's own number — libpq's7, mysql's2002— and the SQLSTATE appears only inerrorInfoand theSQLSTATE[...]message prefix; once a statement has been sent, all three agree.Driver::getSqlState()reads them in that order for this reason.The ODBC driver translates its own strings, so on a localized install the English needles matched nothing at all and a dropped SQL Server link was never recognised as one. That path is now covered by SQLSTATE class
08, and a localized08S01is in the test data.The
ConnectionExceptionTestdata sets that were removed (new \Exception('0800'),new \Exception('080P'), andnew \Exception('Bad connection')for SQL Server) encoded the heuristic being fixed. They are replaced by exceptions carrying real states —08006,08P01,57P01,53300,08001,08S01— which is what the needles were reaching for by matching the message prefix.Driver::connect()still exposes the rawPDOException, and a query wraps whatever failed insideDriver::statement()into aStatementExceptionwith the cause ingetPrevious(). That is the existing contract:formatDatetime()already throws aDriverExceptioninside the sametry, and it reaches the caller wrapped. The SQL Server version check now follows the same rule rather than getting its own exit.