Repository navigation
Conversation
SQLite3Stmt::reset() reports a failed sqlite3_reset() through php_sqlite3_error(), but SQLite3Result::reset() returned false silently and SQLite3Result::finalize() ignored the result. Both now surface the underlying SQLite error the same way; finalize() keeps its : true return type and reports without altering the return value. Fixes phpGH-22051
LamentXU123
left a comment
LamentXU123
left a comment
There was a problem hiding this comment.
No strong opinions. Might need to hear from @ndossche to deal with this as I have zero experience in sqlite.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Ok to me. Could ask Saki for a double check as dbs are her cup of tea.
|
@SakiTakamachi Would appreciate your feedback on this one 🙏 |
SakiTakamachi
left a comment
SakiTakamachi
left a comment
There was a problem hiding this comment.
The patch logic looks good to me!
I left a comment about the wording of the error message.
| sqlite3result_clear_column_names_cache(result_obj); | ||
|
|
||
| if (sqlite3_reset(result_obj->stmt_obj->stmt) != SQLITE_OK) { | ||
| php_sqlite3_error(result_obj->db_obj, sqlite3_errcode(sqlite3_db_handle(result_obj->stmt_obj->stmt)), "Unable to reset statement: %s", sqlite3_errmsg(sqlite3_db_handle(result_obj->stmt_obj->stmt))); |
There was a problem hiding this comment.
Unable to reset statement may be slightly misleading here, since a non- SQLITE_OK return from sqlite3_reset() may reflect an error from the previous evaluation rather than a failure of the reset operation itself.
How about this?
| php_sqlite3_error(result_obj->db_obj, sqlite3_errcode(sqlite3_db_handle(result_obj->stmt_obj->stmt)), "Unable to reset statement: %s", sqlite3_errmsg(sqlite3_db_handle(result_obj->stmt_obj->stmt))); | |
| php_sqlite3_error(result_obj->db_obj, sqlite3_errcode(sqlite3_db_handle(result_obj->stmt_obj->stmt)), "Statement reset reported an error: %s", sqlite3_errmsg(sqlite3_db_handle(result_obj->stmt_obj->stmt))); |
There was a problem hiding this comment.
Applied your wording to both sites in the merged commit.
| } else { | ||
| sqlite3_reset(result_obj->stmt_obj->stmt); | ||
| if (sqlite3_reset(result_obj->stmt_obj->stmt) != SQLITE_OK) { | ||
| php_sqlite3_error(result_obj->db_obj, sqlite3_errcode(sqlite3_db_handle(result_obj->stmt_obj->stmt)), "Unable to reset statement: %s", sqlite3_errmsg(sqlite3_db_handle(result_obj->stmt_obj->stmt))); |
4 participants
SQLite3Stmt::reset() reports a failed sqlite3_reset() through php_sqlite3_error(), but SQLite3Result::reset() returned false silently and SQLite3Result::finalize() ignored the result. Both now report the same way; finalize() keeps its : true return type and reports without changing it. The other sqlite3_reset/sqlite3_finalize calls are left unchecked deliberately: resets after a successful step cannot fail, the reset-before-execute for bug #77051 returns the prior run's stale code, the error-path finalize calls would re-report an error already surfaced in the same function, and the destructor calls have no userland frame to report from.
Fixes #22051