Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105723
MIME-Version: 1.0
References: <CAMERU+4=XQ_8oji+McTjQB2pPXFLR1D6_2iWyFxiTVLCZX3pJQ@mail.gmail.com>
 <b7b52123-82ce-34bb-9ebf-365c0ac82ff3@gmx.de>
In-Reply-To: <b7b52123-82ce-34bb-9ebf-365c0ac82ff3@gmx.de>
Date: Thu, 16 May 2019 09:51:05 -0400
Message-ID: <CAMERU+6vpSkDUBpgMtU0Cob=OGXcG98WvAX+MY9xzJHApovzMw@mail.gmail.com>
To: "Christoph M. Becker" <cmbecker69@gmx.de>, internals@lists.php.net
Content-Type: multipart/alternative; boundary="0000000000006012160589018f43"
Subject: Re: RFC for Sqlite3 Extended Error Codes
From: rkopack@tenable.com (Robert Kopack)

--0000000000006012160589018f43
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, May 15, 2019 at 7:00 PM Christoph M. Becker <cmbecker69@gmx.de>
wrote:

> On 15.05.2019 at 19:14, Robert Kopack wrote:
>
> > I would to extend the functionality of the current sqlite3 implementati=
on
> > (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/f6c4d1eeccd27a3c5bd836fd8205b12=
83bd5eabc
> .
> > From
> > my understanding Scott MacVicar and Christoph M. Becker are the primary
> > maintainers for the sqlite3 extension and Ilia Alshanetsky is the prima=
ry
> > 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 =E2=80=93 i.e. pdo_sqlite is effe=
ctively
> 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.


> > if any of you would like to
> > comment on this (it's around 50 lines of new code) so I can get feedbac=
k
> on
> > this I would greatly appreciate it. I made sure to follow the conventio=
n
> 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.
>

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!

--0000000000006012160589018f43--