Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:105737
MIME-Version: 1.0
References: <CAJW__o0yRBpNTDXYV8Ns3SoyhYHOiBZXr8jPzZ+dBUXAt0Vk9Q@mail.gmail.com>
In-Reply-To: <CAJW__o0yRBpNTDXYV8Ns3SoyhYHOiBZXr8jPzZ+dBUXAt0Vk9Q@mail.gmail.com>
Date: Fri, 17 May 2019 16:19:44 -0400
Message-ID: <CAMERU+7=UkEZ3Ej-MZG4ZM+ZN2x7dh_Hx6kVmOLSCJLMFaOM2g@mail.gmail.com>
To: internals@lists.php.net
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Changing the default value of SQLite3::enableExceptions()
From: rkopack@tenable.com (Robert Kopack)

I feel like changing the default value of $enableExceptions to TRUE
makes more sense since that is what you would expect from the original
function call as it stands and is the least breakage of anything
already existing (since making it a required variable would throw
ArgumentCountError for any existing code).

WRT the PR you mentioned (since it's my own), do you think it would be
better to change the current toggleExtendedResultCodes to
enableExtendedResultCodes and have a similar function prototype to
enableExceptions? I didn't consider that when I wrote my changes (to
be honest I don't use the sqlite3 class directly so I didn't know
about it, I only used the pdo_sqlite layer above it).


On Fri, May 17, 2019 at 3:49 PM Kalle Sommer Nielsen <kalle@php.net> wrote:
>
> Evening Internals
>
> While reviewing a PR[1], which proposes to add two new methods to
> ext/sqlite3, one being an option to toggle on/off, I've noticed that
> there already is another method which also enables the extension to
> toggle on/off behavior, in this case; whether or not to throw
> exceptions instead of warnings.
>
> This method have a rather poor naming considering its default value.
> Observe the current prototype:
> SQLite3::enableExceptions ([ bool $enableExceptions =3D FALSE ] ) : bool
>
> What this means is that code like this may not do what you actually
> think it does:
> $link->enableExceptions();
>
> Due to the $enableExceptions parameter being default to false. I'm
> writing this email to get some feedback on thoughts regarding changing
> this default value to true (current thinking is PHP8) or simply
> dropping the default value, forcing users to specify whether or not to
> enable exception mode.
>
> I personally think its better to simply deal with the default value
> rather than making a bigger potential BC break by renaming the method
> to something like: SQLite3::toggleExceptions()
>
> Thoughts?
>
>
> [1] https://github.com/php/php-src/pull/4166
>
> --
> regards,
>
> Kalle Sommer Nielsen
> kalle@php.net
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>