FIx: Do not call SQLCancel in core_sqlsrv_next_result after SQLMoreResults error

[jahnvi480]

Github issue #1599

This pull request improves how the SQLSRV and PDO_SQLSRV drivers handle errors during multi-statement batches, specifically when a non-fatal error (such as divide-by-zero) occurs mid-batch with XACT_ABORT OFF. Previously, the driver would call SQLCancel() on any error, which destroyed the remaining result sets. Now, errors are reported but the batch remains navigable, allowing users to continue to subsequent result sets. The changes also ensure that error information is available without interrupting batch navigation, and add comprehensive tests for these scenarios.

Error handling and batch navigation improvements:

• Updated core_sqlsrv_next_result to distinguish between internal and user-facing error handling: for user-facing calls (like sqlsrv_next_result and PDO::nextRowset), SQL errors no longer trigger SQLCancel(), allowing continued navigation through remaining result sets after a mid-batch error. Errors are reported via standard error handlers, and any pending Zend exceptions are cleared to avoid aborting navigation.
• Modified function signatures and call sites (pdo_stmt.cpp, stmt.cpp) to support the new error handling logic by passing additional parameters to core_sqlsrv_next_result.

Test coverage:

• Added pdo_batch_error_continue.phpt to test that PDO::nextRowset() reports errors but allows continued navigation after a mid-batch error, for both ERRMODE_WARNING and ERRMODE_EXCEPTION modes.
• Added sqlsrv_batch_error_continue.phpt to verify that sqlsrv_next_result() behaves correctly after a mid-batch error, that error info is available, and that re-execution works after such errors.

Dependency and include updates:

• Included zend_exceptions.h in core_stmt.cpp to enable Zend exception management for the improved error handling logic.

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.76%. Comparing base (68e407b) to head (b5b5fe2).

Files with missing lines	Patch %	Lines
source/shared/core_stmt.cpp	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1600      +/-   ##
==========================================
- Coverage   85.76%   85.76%   -0.01%     
==========================================
  Files          23       23              
  Lines        7207     7214       +7     
==========================================
+ Hits         6181     6187       +6     
- Misses       1026     1027       +1     

Files with missing lines	Coverage Δ
source/pdo_sqlsrv/pdo_stmt.cpp	81.61% <100.00%> (ø)
source/sqlsrv/stmt.cpp	88.30% <100.00%> (ø)
source/shared/core_stmt.cpp	93.48% <88.88%> (-0.04%)	⬇️

🚀 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.