Re: RFC for Sqlite3 Extended Error Codes

Hello everybody, I've had the PR open for this a while now (https://github.com/php/php-src/pull/4166) and was hoping to get more eyes on it; Kalle was looking at it but he appears to have not been as active lately and as I believe it is in a good state right now I was hoping to have some other feedback on it. I was also curious if this would go into a 7.x version of PHP or if something like this would only be for the next major version (8.0). Thank you, Robert On Sat, May 18, 2019 at 5:51 AM Christoph M. Becker <cmbecker69@gmx.de> wrote: > > On 16.05.2019 at 15:51, Robert Kopack wrote: > > > On Wed, May 15, 2019 at 7:00 PM Christoph M. Becker <cmbecker69@gmx.de> > > wrote: > > > >> I haven't reviewed closely for now, but basically I like these > >> additions. With regard to sqlite3_extended_result_codes() we have to > >> review existing error checks (they likely need small adjustments, if > >> extended error codes are enabled). And we likely want to have a few > >> tests for the new functionality. > > > > I didn't consider that! And just to clarify are you referring to the > > results from sqlite3_extended_errcode() or the return value from > > sqlite3_extended_result_codes()? > > I was referring to the slight behavioral change that is caused by > calling sqlite3_extended_result_codes(db, 1). As I understand it, this > will cause other functions to possibly return extended error codes > instead of primary result codes. It seems, though, that there would not > be any conflict right now, since only SQLITE_OK, SQLITE_ROW and > SQLITE_DONE are checked in ext/sqlite3, and these do not have extended > error codes. Still it might make sense to mask these checks to prevent > future issues. And of course, we would have to be careful, if we add > checks for e.g. SQLITE_BUSY in the future. > > > If it's the latter, and after glancing at the sqlite source, it looks like > > that function returns either SQLITE_MISUSE (21) or SQLITE_OK (0). I put in > > -1 instead of 21 as the return for that function in error because I felt > > that was following what PHP normally does on error (perhaps FALSE would be > > better?) In fact, on further thought, returning the 0 from success on that > > might be confusing as well. Perhaps it would better suited to check the > > return and return TRUE on SQLITE_OK and FALSE on SQLITE_MISUSE? > > I think the method should return TRUE on success, and FALSE on failure. > An alternative might be to return the old value, like done by > SQLite3::enableExceptions(). > > > Thank you for your time! > > Thanks for your's. :) > > -- > Christoph M. Becker >