[Request][Discussion] Introduce interfaces PDOInterface andPDOStatementInterface

  100067
July 28, 2017 06:40 newaltgroup@bk.ru (Andrew Nester)
Hello!

I’ve been working on request introduced here https://bugs.php.net/bug.php?id=74957 <https://bugs.php.net/bug.php?id=74957> related to implementing new PDOInterface and PDOStatementInterfaces.
At this point of time classes PDO and PDOStatement do not implement any interfaces that’s why code that uses PDO and PDOStatement are coupled and referred to concrete implementation , not interface.

It will help to add some custom classes and behavior to these classes and to decouple caller from details of PDO layer implementations.

At this point of time, the only way to add some custom classes and behavior is to approach this is by subclassing PDO and PDOStatement. 
You can use the PDO:: ATTR_STATEMENT_CLASS driver option to tell a PDO object which PDOStatement subclass to return from PDO::prepare().
But this is is not very straightforward and elegant (in terms of coding) way to do this.

But if PDO and PDOStatement implemented interfaces, it would be possible for the custom behaviour classes to implement those interfaces as well, making them interchangeable. 
No changes needed in callers of PDO/PDOStatement.

Here is my PR introducing this interface
https://github.com/php/php-src/pull/2657 <https://github.com/php/php-src/pull/2657>

I would like to hear any feedback on it!
Thanks!
  100096
July 29, 2017 10:13 danack@basereality.com (Dan Ackroyd)
On 28 July 2017 at 07:40, Andrew Nester <newaltgroup@bk.ru> wrote:
> Hello! > > the only way to add some custom classes and behavior is to approach this is by subclassing PDO and PDOStatement.
Please could you explicitly say when this is a problem, or what using an interface allows? I can imagine possible scenarios, but the discussion would be clearer if you gave concrete examples. cheers Dan
  100132
July 31, 2017 07:21 newaltgroup@bk.ru (Andrew Nester)
Yes, sure.

Good example has been provided in related issue in bug tracker.

Assume we are using persistent PDO and want to handle long running processes and add some logic when executing queries / connections (for instance logging).

It would require your custom classes deriving from PDO and PDOStatement where we add this additional logic.
According to documentation (http://php.net/manual/en/pdo.setattribute.php <http://php.net/manual/en/pdo.setattribute.php>) when we are using persistent PDO we can’t use PDO::ATTR_STATEMENT_CLASS and return our custom PDOStatement class instance from prepare method.

But just implementing PDOInterface and PDOStatementInterface will allow us to implement this and have proper type hints in userland code.

In addition, using interfaces is better from code architecture perspective (which is just my opinion and a bit arguable of course).

Thanks!




> On Jul 29, 2017, at 1:13 PM, Dan Ackroyd <danack@basereality.com> wrote: > > On 28 July 2017 at 07:40, Andrew Nester <newaltgroup@bk.ru> wrote: >> Hello! >> >> the only way to add some custom classes and behavior is to approach this is by subclassing PDO and PDOStatement. > > > Please could you explicitly say when this is a problem, or what using > an interface allows? > > I can imagine possible scenarios, but the discussion would be clearer > if you gave concrete examples. > > cheers > Dan
  100133
July 31, 2017 11:17 danack@basereality.com (Dan Ackroyd)
On 31 July 2017 at 08:21, Andrew Nester <newaltgroup@bk.ru> wrote:
> > when we are using persistent PDO we can’t use PDO::ATTR_STATEMENT_CLASS and > return our custom PDOStatement class > > But just implementing PDOInterface and PDOStatementInterface will allow us to implement > this and have proper type hints in userland code.
Are you sure having interfaces would change this? I would assume you can't use PDO::ATTR_STATEMENT_CLASS with persistent PDO due to a limitation of the implementation internal to PDO, rather than anything to do with what sub-classes what. Could you post a working example of being able to set PDO::ATTR_STATEMENT_CLASS with persistent PDO? cheers Dan
  100134
July 31, 2017 11:49 newaltgroup@bk.ru (Andrew Nester)
That’s actually the thing that you can’t use PDO::ATTR_STATEMENT_CLASS with persistent PDO.

To make it possible to have persistent PDO with custom PDOStatement you should have:
1) custom `CustomPDO implements PDOInterface` which will be somewhat proxy to PDO instance
2) custom `CustomPDOStatement implements PDOStatementInterface` which will be returned from CustomPDO::prepare and will have our additional logic + some stuff for persistence.

And in our userland code we can have type hints like `someMethod(PDOInterface $pdo)` or `someMethod(PDOStatementInterface $stmt)` 

I hope it explains a bit how interfaces could help here.



> On Jul 31, 2017, at 2:17 PM, Dan Ackroyd <danack@basereality.com> wrote: > > On 31 July 2017 at 08:21, Andrew Nester <newaltgroup@bk.ru> wrote: >> >> when we are using persistent PDO we can’t use PDO::ATTR_STATEMENT_CLASS and >> return our custom PDOStatement class >> >> But just implementing PDOInterface and PDOStatementInterface will allow us to implement >> this and have proper type hints in userland code. > > Are you sure having interfaces would change this? > > I would assume you can't use PDO::ATTR_STATEMENT_CLASS with persistent > PDO due to a limitation of the implementation internal to PDO, rather > than anything to do with what sub-classes what. > > Could you post a working example of being able to set > PDO::ATTR_STATEMENT_CLASS with persistent PDO? > > cheers > Dan
  100136
July 31, 2017 11:59 danack@basereality.com (Dan Ackroyd)
On 31 July 2017 at 12:49, Andrew Nester <newaltgroup@bk.ru> wrote:
> > To make it possible to have persistent PDO with custom PDOStatement you > should have: > > 1) custom `CustomPDO implements PDOInterface` which will be somewhat proxy to PDO instance > 2) custom `CustomPDOStatement implements PDOStatementInterface`...
"I think you should be more explicit here between steps one and two. http://www.sciencecartoonsplus.com/pages/gallery.php I'm not saying that's not going to work......I'm saying you should try to make a working example, to show that it works. That PDO with persistent connections doesn't support ATTR_STATEMENT_CLASS hints that it is doing some magic internally. Working around that magic in userland, while preserving the persistent connection functionality might be 'non-trivial'. Actually, there should be tests for that functionality as part of the PR, probably. cheers Dan
  100137
July 31, 2017 12:20 newaltgroup@bk.ru (Andrew Nester)
> On Jul 31, 2017, at 2:59 PM, Dan Ackroyd <danack@basereality.com> wrote: > > On 31 July 2017 at 12:49, Andrew Nester <newaltgroup@bk.ru> wrote: >> >> To make it possible to have persistent PDO with custom PDOStatement you >> should have: >> >> 1) custom `CustomPDO implements PDOInterface` which will be somewhat proxy to PDO instance >> 2) custom `CustomPDOStatement implements PDOStatementInterface`... > > "I think you should be more explicit here between steps one and two. > http://www.sciencecartoonsplus.com/pages/gallery.php > > I'm not saying that's not going to work......I'm saying you should try > to make a working example, to show that it works. > > That PDO with persistent connections doesn't support > ATTR_STATEMENT_CLASS hints that it is doing some magic internally. > > Working around that magic in userland, while preserving the persistent > connection functionality might be 'non-trivial'. > > Actually, there should be tests for that functionality as part of the > PR, probably. > > cheers > Dan
Yes, sure. I’m just thinking that instead of creating own example, it’s better to take real one. Here is the issue in Doctrine DBAL code similar to the one we’re discussing. https://github.com/doctrine/dbal/issues/2315 <https://github.com/doctrine/dbal/issues/2315> And here you can see couple of interfaces have been created. These interfaces are mirror of PDO and PDOStatement interface. https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/ResultStatement.php <https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/ResultStatement.php> https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/Statement.php <https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/Statement.php> https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/Connection.php <https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/Connection.php> Here is example implementation of this interfaces https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/PDOStatement.php <https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/PDOStatement.php> https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/PDOConnection.php <https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Driver/PDOConnection.php> And they came into issue with using ATTR_STATEMENT_CLASS (mentioned above) I absolutely agree with you that implementing persistent stuff in user land code is non-trivial thing but it’s just one of use case where interfaces could be useful. But I think that code above proves need of these interfaces because they created them on their own. And in general I think it makes user code a bit more readable. I don’t think that concrete implementation of interfaces should be included in tests because it’s just one of use case of these new interfaces (not the easiest one and there could be more of them)
  100138
July 31, 2017 14:54 johannes@schlueters.de (Johannes =?ISO-8859-1?Q?Schl=FCter?=)
On Mo, 2017-07-31 at 14:49 +0300, Andrew Nester wrote:
> That’s actually the thing that you can’t use > PDO::ATTR_STATEMENT_CLASS with persistent PDO.
The actually question is: Why not? - From a quick glance on the code I see no obvious reason. In speculation I assume the implementor thought "Well, we can't guarantee that a class that is there in one request will be there on the next release and it will quite certainly be at a different memory address thus the cached class_entry pointer will be wrong" but the user has to reset the attribute anyways ... we just have to make sure the different dbh->def_stmt_flags are clean when a new PDO connection object is created recovering an old connection ... johannes
  100139
July 31, 2017 15:00 newaltgroup@bk.ru (Andrew Nester)
> On Jul 31, 2017, at 5:54 PM, Johannes Schlüter <johannes@schlueters.de> wrote: > > On Mo, 2017-07-31 at 14:49 +0300, Andrew Nester wrote: >> That’s actually the thing that you can’t use >> PDO::ATTR_STATEMENT_CLASS with persistent PDO. > > The actually question is: Why not? - From a quick glance on the code I > see no obvious reason. In speculation I assume the implementor thought > "Well, we can't guarantee that a class that is there in one request > will be there on the next release and it will quite certainly be at a > different memory address thus the cached class_entry pointer will be > wrong" but the user has to reset the attribute anyways ... we just have > to make sure the different dbh->def_stmt_flags are clean when a new PDO > connection object is created recovering an old connection ... > > johannes
Besides code style/architecture things (which is of course questionable) the issue with ATTR_STATEMENT_CLASS is that it simply doesn’t work with persistent PDO connect and raises "General error: PDO::ATTR_STATEMENT_CLASS cannot be used with persistent PDO instances"
  100135
July 31, 2017 11:51 newaltgroup@bk.ru (Andrew Nester)
> On Jul 31, 2017, at 2:17 PM, Dan Ackroyd <danack@basereality.com> wrote: > > On 31 July 2017 at 08:21, Andrew Nester <newaltgroup@bk.ru> wrote: >> >> when we are using persistent PDO we can’t use PDO::ATTR_STATEMENT_CLASS and >> return our custom PDOStatement class >> >> But just implementing PDOInterface and PDOStatementInterface will allow us to implement >> this and have proper type hints in userland code. > > Are you sure having interfaces would change this? > > I would assume you can't use PDO::ATTR_STATEMENT_CLASS with persistent > PDO due to a limitation of the implementation internal to PDO, rather > than anything to do with what sub-classes what. > > Could you post a working example of being able to set > PDO::ATTR_STATEMENT_CLASS with persistent PDO? > > cheers > Dan
Ouch, I’m sorry, Dan, for `top` posting instead of `bottom` one