Everything

php

Changing the default value of SQLite3::enableExceptions()

php.internals

Kalle Sommer Nielsen

7 years ago

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 = 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

Robert Kopack

7 years ago

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:

Kalle Sommer Nielsen

7 years ago

Hi

Den fre. 17. maj 2019 kl. 23.20 skrev Robert Kopack <rkopack@tenable.com>:

>
> 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).

I thought about proposing that, but I wanted to run this issue by
internals first to hear others thoughts on the matter. I definitely
think consistency is something we should embrace more and thinking
about it for a day then I do think we should keep the naming similar,
but avoid a design like SQLite3::enableExceptions().

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Christoph Becker

7 years ago

On 17.05.2019 at 21:42, Kalle Sommer Nielsen wrote:

> 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.

ACK.

> Observe the current prototype:
> SQLite3::enableExceptions ([ bool $enableExceptions = 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()

ACK.

However, I don't think that we can change the default value in the near
future for BC reasons.  Deprecating (7.4) and removing support (8.0) for
calling the method without argument would be the way to go, I think.

[In a somewhat distant future, we might consider reintroducing support
for argument-less calls, but in this case it might make sense to let the
method work as getter (i.e. without argument, it would return the
current value without changing it).]

> [1] https://github.com/php/php-src/pull/4166

Thanks,
Christoph

Kalle Sommer Nielsen

7 years ago

Hi Christoph

Den lør. 18. maj 2019 kl. 13.00 skrev Christoph M. Becker <cmbecker69@gmx.de>:

> However, I don't think that we can change the default value in the near
> future for BC reasons.  Deprecating (7.4) and removing support (8.0) for
> calling the method without argument would be the way to go, I think.

Yeah I agree here. We could go a similar around to what you proposed
for the curl version function, something like (for PHP-7.4):

if (ZEND_NUM_ARGS() == 0) {
/* E_DEPRECATED: Relying on the default value of $enableExceptions
is deprecated as it will change from FALSE to TRUE ...*/
}

And for master simply making the parameter required. I suspect the
impact of this is very minimal as most users of this would not be
affected by this proposal unless its a bug in their code (as I don't
expect anyone to call ->enableExceptions() to *disable* them being
anything but a bug).

> [In a somewhat distant future, we might consider reintroducing support
> for argument-less calls, but in this case it might make sense to let the
> method work as getter (i.e. without argument, it would return the
> current value without changing it).]

Currently SQLite3::enableExceptions() will return the old value, I
think we should retain this and only change the default value.

--
regards,

Kalle Sommer Nielsen
kalle@php.net