Everything

php

Matching PDO_SQLite features with SQLite3 extension

php.internals

BohwaZ/PHP

9 years ago

Hi people of the PHP world,

I just have proposed a patch to have SQLite3 open_blob feature
implemented in PDO_SQLite: https://github.com/php/php-src/pull/2698

This follows my patch to implement this feature in the SQLite3 extension
a few months ago.

Now my aim is to implement missing features in pdo_sqlite that are
already present in the SQLite3 extension (target is PHP 7.3 I guess).
I'm also keen to implement any SQLite3 feature offered by the C library
that are not already present in either extensions (if any).

This way we will be able to use either PDO or SQLite3 extension, and
have access to the same features.

Here are the features that are currently present in the SQLite3
extension but missing in PDO:

- bool Statement::readOnly() -> returns true if the statement doesn't
write to the database
- Opening a database as read-only (using SQLite flags OPEN_READWRITE,
OPEN_READONLY and OPEN_CREATE)

Please tell me if you see anything else that I missed and I'll add it to
my todo-list.

Thanks.

Adam Baratz

9 years ago

>
> I just have proposed a patch to have SQLite3 open_blob feature implemented
> in PDO_SQLite: https://github.com/php/php-src/pull/2698

[cross-posted from GitHub]

A new method is an API change to me, so an RFC would be warranted. I'm
reluctant to add driver-specific methods, since that seems opposed to PDO's
driver-agnostic API, but that's not to say we couldn't hash something out.

Thanks,
Adam

BohwaZ/PHP

9 years ago

Le 22/08/2017 07:55, Adam Baratz a écrit :

> A new method is an API change to me, so an RFC would be warranted. I'm
> reluctant to add driver-specific methods, since that seems opposed to
> PDO's
> driver-agnostic API, but that's not to say we couldn't hash something
> out.

Do we need a RFC every time we patch for a missing method ?

Btw there's already sqliteCreateFunction, sqliteCreateAggregate and so
on, so it's pretty much just the continuation of that logic, even though
I don't really like it either (especially as lazy-loading by extending
PDO makes it so that the methods are not accessible until the connection
has been established, which is a weird behaviour imho).

Adam Baratz

9 years ago

On Mon, Aug 21, 2017 at 5:54 PM, BohwaZ/PHP <php@bohwaz.net> wrote:

> Le 22/08/2017 07:55, Adam Baratz a écrit :
>
>> A new method is an API change to me, so an RFC would be warranted. I'm
>> reluctant to add driver-specific methods, since that seems opposed to
>> PDO's
>> driver-agnostic API, but that's not to say we couldn't hash something out.
>>
>
> Do we need a RFC every time we patch for a missing method ?
>
> Btw there's already sqliteCreateFunction, sqliteCreateAggregate and so on,
> so it's pretty much just the continuation of that logic, even though I
> don't really like it either (especially as lazy-loading by extending PDO
> makes it so that the methods are not accessible until the connection has
> been established, which is a weird behaviour imho).

I see. Didn't realize it was an existing pattern. Not going to say I love
it, but that's not a reason to break with it now. We could maybe talk
separately about better ways to do this sort of thing, to avoid the
situation you describe, but I don't need to derail this conversation any
further.

Thanks for taking the time to get parity between the extensions.

Adam

Marco Pivetta

9 years ago

Yes, because once it lands in core, it sticks around for almost eternity.

On 21 Aug 2017 23:54, "BohwaZ/PHP" <php@bohwaz.net> wrote:

BohwaZ/PHP

9 years ago

> Yes, because once it lands in core, it sticks around for almost
> eternity.

Yeah but is it necessary for something that is just missing, because the
pdo_sqlite implementation is incomplete, and is basically following what
already exists, without changing anything?

That change was implemented in the SQLite3 extension without a RFC, so
I'm quite confused here.

I kinda feel like it's a weird thing to submit an RFC that would
basically ask the question "should pdo_sqlite only implement a subset of
SQLite", because well it is most likely that if you are using a DB
driver with PDO you most likely want to be able to access that DB
features, no?

Or are you saying that we should have a vote on whether the
implementation should follow what is already existing in PDO or should
propose something new instead? Because I frankly don't know what would
be a better idea than driver-specific methods and I don't know enough
C/have enough time to do anything else, so I won't submit any
proposition that I won't be able to do myself.

Cheers.

Marco Pivetta

9 years ago

PDO is already a mess, and adding method that appear/disappear dynamically
whether you enable an extension or not... is a horror show.

From my PoV, since we (doctrine) have to abstract away from it all the
time, we'd rather have it as tidy and well-thought-out as possible,
especially since there already is so much damage done.

Couldn't care less about exposed/unexposed features if the endpoints are on
the wrong object, or cause even more weirdness to work with. It is not
helpful: it's just more tech debt dumped on millions of consumers.


On 23 Aug 2017 4:39 AM, "BohwaZ/PHP" <php@bohwaz.net> wrote:

BohwaZ/PHP

9 years ago

> PDO is already a mess, and adding method that appear/disappear
> dynamically
> whether you enable an extension or not... is a horror show.
>
> From my PoV, since we (doctrine) have to abstract away from it all the
> time, we'd rather have it as tidy and well-thought-out as possible,
> especially since there already is so much damage done.

Yes I agree, but it's not the point, this behaviour already exists, I'm
not introducing anything new here.

> Couldn't care less about exposed/unexposed features if the endpoints
> are on
> the wrong object, or cause even more weirdness to work with. It is not
> helpful: it's just more tech debt dumped on millions of consumers.

I care because I need those features in my projects, and I'm not the
only one.

But if those late-loading methods don't suit you, what do you propose
instead?

Marco Pivetta

9 years ago

I suggest adding dedicated functions that given a PDO instance and the
parameters you needed do what you want to do.

And yes, you are adding new stuff: php-src has this widespread
misunderstanding that "reproducing new lower layer behaviour in upper
layers" should happen no matter what.


On 23 Aug 2017 6:39 AM, "BohwaZ/PHP" <php@bohwaz.net> wrote:

BohwaZ/PHP

9 years ago

Le 23/08/2017 16:57, Marco Pivetta a écrit :

> I suggest adding dedicated functions that given a PDO instance and the
> parameters you needed do what you want to do.

So if I understand correctly:

$pdo = new PDO('sqlite::memory:');
$extended = new PDO_Extended_SQLite($pdo);
$blob = $extended->openBlob(...);

That's kinda better, but that doesn't solve much, as that means that
only one method will be in this new class, and PDO will still have
sqliteCreate... methods
as we won't cause unnecessary BC breaks.

I'm not a C developer though and I don't see how that would be possible.

My feeling is that it would have been better to have a class extending
PDO for each driver, adding additional constants and methods, but that's
not how PDO has been designed, and I'm not gonna suggest that we should
rewrite PDO now.

> And yes, you are adding new stuff: php-src has this widespread
> misunderstanding that "reproducing new lower layer behaviour in upper
> layers" should happen no matter what.

I don't get your point, this is already in a upper layer no? PDO already
has that for SQLite and I just checked and pgSQL too:

PDO::pgsqlCopyFromArray
PDO::pgsqlCopyFromFile
PDO::pgsqlCopyToArray
PDO::pgsqlCopyToFile
PDO::pgsqlGetNotify
PDO::pgsqlGetPid
PDO::pgsqlLOBCreate
PDO::pgsqlLOBOpen
PDO::pgsqlLOBUnlink

I'm not gonna create another way of having driver-specific features that
wouldn't make sense in relation to existing methods, and I don't have
time to rewrite a significant part of PDO that would cause a major
BC-break.

We all agree that this is not the best way, but it's too late for that I
think.

So can we go back to topic?

I'm just trying to improve existing PHP in the (very little amount of)
free time that I've got, following what has already been done, not
trying to refactor/reinvent/do major changes stuff here.

Marco Pivetta

9 years ago

PDO\sqlite_open_blob($pdo, ...).

And no, just because a lot of folks jumped from the bridge doesn't make
jumping from the bridge acceptable. You can pursue the approach, but you
will get a negative vote from over here: that's also where my influence
stops though.

On 23 Aug 2017 7:15 AM, "BohwaZ/PHP" <php@bohwaz.net> wrote:

Lester Caine

9 years ago

On 23/08/17 06:15, BohwaZ/PHP wrote:

> I'm not gonna create another way of having driver-specific features that
> wouldn't make sense in relation to existing methods, and I don't have
> time to rewrite a significant part of PDO that would cause a major
> BC-break.

The original intention of PDO was that it did not matter just what
underlying database you were using, anything written for one would
simply work with any other. All of the driver specific extensions make a
mockery of that, and in many cases today PDO will only work with the
database it was programmed for ... so why use PDO at all in that
instance? For example a BLOB improvement to PDO should work
transparently across all databases that support blobs ... and error when
blobs are not available.

--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Adam Baratz

9 years ago

>
> That change was implemented in the SQLite3 extension without a RFC, so I'm
> quite confused here.
>

My literal reading of the contribution guide[1] is that new features should
be preceded by an RFC, though there are certainly cases where this doesn't
happen. I don't have a clear enough understanding of this community to say
when RFCs can be skipped. I'm not sure who the authority would be. The
release manager for the next minor rev? Anyone with commit privileges?

I kinda feel like it's a weird thing to submit an RFC that would basically

> ask the question "should pdo_sqlite only implement a subset of SQLite",
> because well it is most likely that if you are using a DB driver with PDO
> you most likely want to be able to access that DB features, no?
>
> Or are you saying that we should have a vote on whether the implementation
> should follow what is already existing in PDO or should propose something
> new instead? Because I frankly don't know what would be a better idea than
> driver-specific methods and I don't know enough C/have enough time to do
> anything else, so I won't submit any proposition that I won't be able to do
> myself.
>

Only speaking for myself: I'd just want the RFC to act as a sanity check
that the method you're adding has the right signature and it's being
implemented consistently with similar methods. I don't like the design
pattern either, but it's not like you're introducing it. It's better for
PDO to be feature complete than to have a perfect architecture. A tool
missing functionality will be less desirable to use. I'd be more concerned
about the consequences of that state. Whoever wants to figure out the right
way to support extension-specific methods would have to figure out how to
deprecate the existing methods. Adding one wouldn't really change the scope
of this problem, which isn't intractable in the greater scheme of things.

Thanks,
Adam

---
[1] https://github.com/php/php-src/blob/master/CONTRIBUTING.md

Christoph Becker

9 years ago

On 24.08.2017 at 00:06, Adam Baratz wrote:

>> That change was implemented in the SQLite3 extension without a RFC, so I'm
>> quite confused here.
>
> My literal reading of the contribution guide[1] is that new features should
> be preceded by an RFC, though there are certainly cases where this doesn't
> happen. I don't have a clear enough understanding of this community to say
> when RFCs can be skipped. I'm not sure who the authority would be. The
> release manager for the next minor rev? Anyone with commit privileges?

It seems to me this is rather simple: if a feature is uncontroversial,
it doesn't need an RFC.  Since nobody objected against PR #2528, it was
merged (after some weeks).  However, there have been objections against
PR #2698, so an RFC appears to be appropriate.

--
Christoph M. Becker

BohwaZ/PHP

9 years ago

On Thu, 24 Aug 2017 11:55:10 +0200 / "Christoph M. Becker"
<cmbecker69@gmx.de> said :

> It seems to me this is rather simple: if a feature is uncontroversial,
> it doesn't need an RFC.  Since nobody objected against PR #2528, it
> was merged (after some weeks).  However, there have been objections
> against PR #2698, so an RFC appears to be appropriate.

Cool, that's a rule that makes sense :) maybe it should be in the
contributing guide?

Can someone give me RFC power on the wiki (my account is "bohwaz")?
Thanks.

Should I include the proposal for adding the PDOStatement::readOnly()
method?

Should it be named PDOStatement::sqliteReadOnly() or readOnly()? I have
no idea. Or should it even be PDO::sqliteIsReadOnly($statement)?

I think that adding read-only and read-write opening options for SQLite
PDO shouldn't raise any eyebrows here as it doesn't involve any new
method, so I'll do a separate patch for that. Anyone objecting?

Cheers.