# RFC for Sqlite3 Extended Error Codes - Thread: https://externals.io/message/105717 - Original archive: https://news-web.php.net/php.internals/105717 - Started by Robert Kopack on 2019-05-15T17:14:17+00:00 - Messages: 5 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #105717 — Robert Kopack — 2019-05-15T17:14:17+00:00 URL: https://externals.io/message/105717#105717 Hello, My apologies if I violate any netiquette during this e-mail. This is my first time doing something like this and I am following the guide from the CONTRIBUTING.md to the best of my ability. I would to extend the functionality of the current sqlite3 implementation (in *ext/sqlite3* and in *ext/pdo_sqlite*) by exposing the functions related to extended result codes. I have already forked and made my changes at https://github.com/rkopack/php-src/commit/f6c4d1eeccd27a3c5bd836fd8205b1283bd5eabc. From my understanding Scott MacVicar and Christoph M. Becker are the primary maintainers for the sqlite3 extension and Ilia Alshanetsky is the primary maintainer for the pdo_sqlite extension; if any of you would like to comment on this (it's around 50 lines of new code) so I can get feedback on this I would greatly appreciate it. I made sure to follow the convention I saw in the file(s) to the best of my ability but I am sure there are little things I might have missed. Thank you for your time, Robert Kopack --- ## #105721 — Christoph M. Becker — 2019-05-15T22:54:58+00:00 URL: https://externals.io/message/105717#105721 > I would to extend the functionality of the current sqlite3 implementation > (in *ext/sqlite3* and in *ext/pdo_sqlite*) by exposing the functions > related to extended result codes. I have already forked and made my changes > at > https://github.com/rkopack/php-src/commit/f6c4d1eeccd27a3c5bd836fd8205b1283bd5eabc. > From > my understanding Scott MacVicar and Christoph M. Becker are the primary > maintainers for the sqlite3 extension and Ilia Alshanetsky is the primary > maintainer for the pdo_sqlite extension; It's best to look at EXTENSIONS in master[1], which also reveals the most up-to-date dates of maintainership – i.e. pdo_sqlite is effectively maintained by the core maintainers. > if any of you would like to > comment on this (it's around 50 lines of new code) so I can get feedback on > this I would greatly appreciate it. I made sure to follow the convention I > saw in the file(s) to the best of my ability but I am sure there are little > things I might have missed. 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. Feel free to submit a pull request[2]; it seems to me any discussion could happen on Github as well. :) [1] [2] Thanks, Christoph --- ## #105723 — Robert Kopack — 2019-05-16T13:51:05+00:00 URL: https://externals.io/message/105717#105723 On Wed, May 15, 2019 at 7:00 PM Christoph M. Becker wrote: > > > > […] > changes > > […] > https://github.com/rkopack/php-src/commit/f6c4d1eeccd27a3c5bd836fd8205b1283bd5eabc > . > > […] > > It's best to look at EXTENSIONS in master[1], which also reveals the > most up-to-date dates of maintainership – i.e. pdo_sqlite is effectively > maintained by the core maintainers. > That's strange as I did look in that file first which is how I found the three names. Perhaps I read it wrong. Mea Cupla. > > […] > on > > […] > I > > […] > little > > […] > > 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()? If the former, I'll have to look around to see where those kind of checks are done - do you know of any locations off the top of your head? From my understanding we can just mask the incoming error code with 0xFF (That's what sqlite does anyway) and always get "what we expected" it to be in the past instead of trying to account for the additional extended error codes in some other manner. 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? With regards to Unit Tests that is a fantastic idea (that I completely forgot about and i'm sure my team lead would lecture me on much to my chagrin) and I will be sure to add some to cover the new functions at a minimum. > Feel free to submit a pull request[2]; it seems to me any discussion > could happen on Github as well. :) > I have already done so ( https://github.com/php/php-src/pull/4166 ) and i'll be sure to keep any more discussion beyond these last questions there. Thank you for your time! --- ## #105741 — Christoph M. Becker — 2019-05-18T09:51:31+00:00 URL: https://externals.io/message/105717#105741 > On Wed, May 15, 2019 at 7:00 PM Christoph M. Becker > wrote: > > > […] > > 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 --- ## #105977 — Robert Kopack — 2019-06-18T19:34:41+00:00 URL: https://externals.io/message/105717#105977 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 > > > > > […] > > 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. > > > […] > > 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(). > > > […] > > Thanks for your's. :) > > -- > Christoph M. Becker