odbc: make odbc_sql_error() formattable, drop odbc_colattribute_failed - #23612
Open
iliaal wants to merge 1 commit into
Open
odbc: make odbc_sql_error() formattable, drop odbc_colattribute_failed#23612iliaal wants to merge 1 commit into
iliaal wants to merge 1 commit into
Conversation
iliaal
added a commit
to iliaal/php-src
that referenced
this pull request
Sep 7, 2026
odbc_colattribute_failed() existed only because odbc_sql_error() could not take a column number. Make the reporter variadic so callers can format the context into it, and route the two SQLColAttribute sites through it. Those failures now also populate the connection's last-error state, so odbc_error() and odbc_errormsg() report them. Closes phpGH-23612
iliaal
force-pushed
the
fix/odbc-sql-error-varargs
branch
from
September 7, 2026 22:53
5092a7f to
08c3eaf
Compare
NattyNarwhal
reviewed
Sep 7, 2026
NattyNarwhal
left a comment
NattyNarwhal
left a comment
Member
There was a problem hiding this comment.
Looks good, just one minor nit. I can take a look at converting SQLError usage to SQLGetDiagRec next, or if you also want to tackle that, feel free.
| void odbc_bindcols(odbc_result *result); | ||
|
|
||
| #define ODBC_SQL_ERROR_PARAMS odbc_connection *conn_resource, ODBC_SQL_STMT_T stmt, char *func | ||
| #define ODBC_SQL_ERROR_PARAMS odbc_connection *conn_resource, ODBC_SQL_STMT_T stmt, const char *func |
Member
There was a problem hiding this comment.
I think we could probably drop the macro here
odbc_colattribute_failed() existed only because odbc_sql_error() could not take a column number. Make the reporter variadic so callers can format the context into it, and route the two SQLColAttribute sites through it. Those failures now also populate the connection's last-error state, so odbc_error() and odbc_errormsg() report them. Closes phpGH-23612
iliaal
force-pushed
the
fix/odbc-sql-error-varargs
branch
from
September 8, 2026 00:25
08c3eaf to
a9356d8
Compare
NattyNarwhal
approved these changes
Sep 8, 2026
NattyNarwhal
left a comment
NattyNarwhal
left a comment
Member
There was a problem hiding this comment.
LGTM; the additional allocation from this isn't ideal, but I'm not sure how we could avoid it without using the stack for part of it (seems a bit clumsy?) or having both the va_list and our additional params (not possible afaik).
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
2 participants
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.
Follow-up to #23601, per your suggestion there.
odbc_sql_error()takes a format now, so the two SQLColAttribute sites can name the column and the ad-hoc helper is gone. The other 50 callers pass a plain literal and needed no change; the printf attribute covers them.I left the SQLError in odbc_cursor() alone. It reads SQLSTATE S1015 and synthesizes a cursor name from that, so it wants its own change rather than a swap. Routing the SQLColAttribute failures through odbc_sql_error() also populates the connection's last-error state, so odbc_error() and odbc_errormsg() now report them, hence the NEWS line.