FIx: Add opt-in batch error continuation for sqlsrv and PDO next-result APIs

[jahnvi480]

Github issue #1599

This pull request introduces an opt-in feature to allow batch error continuation for the sqlsrv and PDO drivers' next-result APIs, addressing scenarios where multi-statement batches may encounter mid-batch errors (such as a divide-by-zero). By default, legacy error handling is preserved, but users can now explicitly enable continuation past errors, allowing access to subsequent result sets. The implementation includes new driver attributes and connection options, updates to error handling logic, and comprehensive functional tests.

Batch Error Continuation Feature:

• Added a new driver attribute/connection option (SQLSRV_ATTR_BATCH_ERROR_CONTINUE / BatchErrorContinue) to opt in to batch error continuation for both sqlsrv and PDO drivers. This allows users to continue processing result sets after a mid-batch error, instead of aborting as in legacy behavior.
• Updated the core statement result logic to support this opt-in behavior: when enabled, errors encountered during SQLMoreResults are reported but do not abort result set navigation, and exceptions are cleared to allow continued execution.
• Modified the sqlsrv_next_result and PDO nextRowset implementations to respect the new batch error continuation setting.

Documentation and Changelog:

• Updated CHANGELOG.md to document the addition of batch error continuation controls and clarify that legacy error handling remains the default.

Testing:

• Added comprehensive functional tests for both sqlsrv and PDO drivers to verify default and opt-in behaviors, error reporting, and correct navigation of result sets after batch errors.

[codecov]

Codecov Report

❌ Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.80%. Comparing base (3a8d8ee) to head (f9f4bd0).

Files with missing lines	Patch %	Lines
source/pdo_sqlsrv/pdo_dbh.cpp	50.00%	3 Missing ⚠️
source/shared/core_stmt.cpp	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1600      +/-   ##
==========================================
+ Coverage   85.77%   85.80%   +0.02%     
==========================================
  Files          23       23              
  Lines        7213     7247      +34     
==========================================
+ Hits         6187     6218      +31     
- Misses       1026     1029       +3     

Files with missing lines	Coverage Δ
source/pdo_sqlsrv/pdo_init.cpp	89.70% <ø> (ø)
source/pdo_sqlsrv/pdo_stmt.cpp	81.91% <100.00%> (+0.29%)	⬆️
source/pdo_sqlsrv/php_pdo_sqlsrv_int.h	100.00% <ø> (ø)
source/shared/core_sqlsrv.h	90.25% <100.00%> (+0.31%)	⬆️
source/sqlsrv/conn.cpp	83.65% <100.00%> (+0.08%)	⬆️
source/sqlsrv/stmt.cpp	88.30% <100.00%> (ø)
source/shared/core_stmt.cpp	93.51% <96.29%> (+<0.01%)	⬆️
source/pdo_sqlsrv/pdo_dbh.cpp	91.30% <50.00%> (-0.46%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[David-Engel]

Impact Analysis of the current changes

There are 6 call sites for core_sqlsrv_next_result. Two are changed directly; all six are affected by the catch-block change:

Call site	throw_on_errors	Changed by PR?	Impact
sqlsrv_execute() flush loop	false (existing)	No, but catch block changed	Low — SQLMoreResults errors already didn't throw here
sqlsrv_next_result()	true → false	Yes	High — return value semantics change
pdo_sqlsrv_stmt_close_cursor()	true (default)	No, but catch block changed	Medium — see concern # 3 below
pdo_sqlsrv_stmt_execute() flush	true (default)	No, but catch block changed	Medium — see concern # 3 below
pdo_sqlsrv_stmt_next_rowset()	true → false	Yes	High — return value & exception semantics change
pdo_sqlsrv_stmt_param_hook() flush	true (default)	No, but catch block changed	Medium — see concern # 3 below

---

Compatibility & Behavioral Concerns

1. Breaking change: sqlsrv_next_result() return value (HIGH)

	Before	After
Return value on mid-batch error	false	true
Error available via sqlsrv_errors()	Yes	Yes
Can reach subsequent result sets	No	Yes

Applications that check sqlsrv_next_result() === false to detect batch errors will silently miss the error. This is a user-visible behavior change with no opt-out mechanism.

2. Breaking change: PDO::nextRowset() exception behavior (HIGH)

• ERRMODE_EXCEPTION: Previously threw PDOException. Now the PR explicitly calls zend_clear_exception() to suppress it and returns success. Existing catch (PDOException $e) blocks around nextRowset() will stop firing. Error info is still available via errorInfo(), but code structured around exception-based error handling will silently skip errors.

• ERRMODE_WARNING / ERRMODE_SILENT: Previously returned an error indicator; now returns success. Same concern as the SQLSRV driver.

3. SQLCancel removed from shared catch block — affects all callers (MEDIUM-HIGH)

The catch block is shared between throw_on_errors=true (internal flush loops, closeCursor, param binding) and throw_on_errors=false (user-facing). Removing SQLCancel from the catch block affects the internal paths too:

• When throw_on_errors=true, a core::SQLMoreResults() SQL_ERROR still throws CoreException into this catch block.
• Previously, SQLCancel ensured the ODBC handle was cleaned up. Now the exception propagates without canceling the statement.
• For error scenarios unrelated to mid-batch failures (network errors, connection drops, OOM), not calling SQLCancel could leave the ODBC handle in an indeterminate state.

Recommendation: The SQLCancel removal should be conditional — only skip it when throw_on_errors=false. Something like:

catch( core::CoreException& e ) {
    if( throw_on_errors ) {
        SQLCancel( stmt->handle() );
    }
    throw e;
}

4. zend_clear_exception() is overly broad (MEDIUM)

zend_clear_exception() clears any pending Zend exception, not just the one raised by the ODBC error handler. If another exception was already pending (e.g., from user code or a prior operation), this would silently consume it. A more targeted approach would check whether the pending exception matches the expected type before clearing.

5. new_result_set() called after SQL_ERROR (LOW-MEDIUM)

When SQLMoreResults returns SQL_ERROR, the code falls through to stmt->new_result_set(), which resets internal metadata and may call SQLNumResultCols/SQLRowCount. Whether the ODBC handle is in a valid state for these calls after SQL_ERROR depends on the driver implementation.

6. No opt-out to retain old behavior (HIGH)

Applications that relied on the batch-aborting behavior as a safety mechanism — stopping processing after any statement error — have no way to restore the old behavior. A connection or statement attribute to control this would provide a safer migration path.

---

Summary of Recommendations

1. Make SQLCancel removal conditional on throw_on_errors — the internal flush/closeCursor paths should still call SQLCancel on error to properly clean up the ODBC handle.
2. Narrow zend_clear_exception() — check the pending exception type or only clear if it was raised by this specific error handler invocation.
3. Document the behavior change in release notes — callers checking sqlsrv_next_result() === false or catching PDOException from nextRowset() need to update.
4. Add a connection attribute (e.g., SQLSRV_ATTR_BATCH_ERROR_CONTINUE) to let users opt in to new behavior, preserving backward compatibility.

[David-Engel]

Thanks for adding this — the opt-in design is sound, the default is preserved, and the happy path is well tested. I verified a few things while reviewing: pdo_sqlsrv_dbh : public sqlsrv_conn so the PDO attribute set on driver_dbh correctly reaches stmt->conn->batch_error_continue; source/sqlsrv/shared and source/pdo_sqlsrv/shared are symlinks to source/shared so editing the shared file is correct; and the reordered SS_CONN_OPTIONS enum is only matched by value in the option table (not used as an array index), so reordering is safe. A few items below.

Blocking

1. The shared else branch overloads throw_on_errors=false and silently changes the default re-execute path.

core_sqlsrv_next_result(..., throw_on_errors=false) previously meant "drain silently" and was used only by the sqlsrv re-execute flush loop in source/sqlsrv/stmt.cpp (~L297):

while( stmt->past_next_result_end == false ) {
    core_sqlsrv_next_result( stmt, false, false );   // silent flush
}

This PR repurposes that same else branch to call call_error_handler(stmt, SQLSRV_ERROR_ODBC, 0/1) and zend_clear_exception(). Because the flush loop always passes false regardless of the opt-in flag, a user who has not opted in but re-executes a prepared statement whose prior batch had a mid-batch error/warning will now get those errors/warnings pushed into sqlsrv_errors() where they were previously swallowed. That contradicts the stated contract ("legacy error handling remains the default") and is untested — Test 3 only re-executes on the opt-in connection.

Two distinct semantics are being conflated:

• flush loop: don't throw, don't report (silent drain)
• opt-in user: don't throw, do report, continue

Suggest introducing a separate parameter (e.g. bool report_errors / bool continue_on_error) distinct from throw_on_errors, so the flush loop keeps draining silently and only the user-facing opt-in path (sqlsrv_next_result / pdo_sqlsrv_stmt_next_rowset) reports.

Suggestions

2. SQL_ERROR is treated uniformly as "continuable." In the new branch in source/shared/core_stmt.cpp, any SQL_ERROR from SQLMoreResults is reported, the exception cleared, and execution falls through to new_result_set(). That's valid for a recoverable mid-batch error (divide-by-zero), but a genuinely fatal SQL_ERROR (e.g. a communication link failure) would also be reported as success (nextRowset() returns true) and the code would keep trying to advance. Given it's opt-in this is an acceptable trade-off, but please add a code comment and a CHANGELOG/docs note that true can be returned even for an unrecoverable error.

3. Environment-dependent test assertions. Whether divide-by-zero surfaces as SQL_ERROR vs SQL_SUCCESS_WITH_INFO depends on session settings (ANSI_WARNINGS, XACT_ABORT) and the MSODBC driver version. The tests pin SQLSTATE 22012 and exact bool(true)/bool(false) navigation outcomes — this is the most likely source of cross-version flakiness. Please confirm the tests pass against the oldest supported MSODBC 18 and across SQL Server versions in CI.

4. zend_clear_exception() is unconditional. It clears whatever zend exception is pending, not specifically the one this handler set. In practice the only pending exception here is the PDO ERRMODE_EXCEPTION one, but a narrower approach (clear only when in PDO exception mode / only the just-set exception) would be safer against masking an unrelated pending exception.

Nits

5. Attribute is connection-level only. SQLSRV_ATTR_BATCH_ERROR_CONTINUE is read from stmt->conn at nextRowset time and only handled in pdo_sqlsrv_dbh_set_attr/get_attr, so $stmt->setAttribute(...) won't take effect. Fine if intended — worth documenting that it must be set on the connection (or in the PDO constructor driver options).

6. Docs. Only CHANGELOG.md is updated. The new option/attribute should also be documented in the reference docs and cross-referenced from issue #1599.

[David-Engel]

Thanks for the rework — this addresses all of my earlier feedback well.

Resolved

• Overloaded throw_on_errors (blocking): The new dedicated report_errors parameter (default false) cleanly separates the silent internal drain from the user-facing opt-in continuation. Call sites are consistent, and the default sqlsrv re-execute flush loop stays silent. The new sqlsrv Test 3 asserting Errors after re-execute: NULL on a non-opt-in connection directly locks in the no-regression behavior. 👍
• Uniform SQL_ERROR handling: Now clearly documented in code comments and the CHANGELOG as an opt-in trade-off where next-result may return success while reporting diagnostics.
• Environment-dependent tests: Switching to RAISERROR(...,16,1) and Error captured: yes checks (instead of divide-by-zero / exact SQLSTATE) makes these much more robust across driver/server versions.
• zend_clear_exception(): Now guarded with if( EG(exception) != NULL ).
• Connection-scope attribute: Documented in CHANGELOG + comments, and Test 5 verifies constructor opt-in.

I also checked the new SQL_ERROR/report_errors cleanup path: the manual current_results teardown mirrors the existing pattern in ~sqlsrv_stmt() / new_result_set(), there's no double-free (current_results is nulled and clean_up_results_metadata() only clears the metadata vector), and the PDO nextRowset current_results != NULL guard correctly skips SQLNumResultCols/SQLRowCount on the error marker.

Remaining nits (non-blocking)

1. In the error path, stmt->column_count = 0; stmt->row_count = 0; are immediately overwritten by clean_up_results_metadata() (which sets them to ACTIVE_NUM_COLS_INVALID/ACTIVE_NUM_ROWS_INVALID). They're dead assignments — drop them or rely on the helper.
2. The ~12-line current_results teardown block now appears in three places (destructor, new_result_set, error path). Consider extracting a small free_current_results() helper for maintainability.
3. Minor awareness note (pre-existing, not introduced here): sqlsrv re-execute always silent-drains, while PDO re-execute throws when not opted in. Just flagging the asymmetry.

Nothing blocking from my side — looks good to merge once the nits are optionally cleaned up and CI confirms the new tests pass across the supported driver/server matrix.