[PHP-DEV] [Request][Discussion] Introduce interfaces PDOInterface and PDOStatementInterface

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!

On 28 July 2017 at 07:40, Andrew Nester <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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 <[email protected]> wrote:
>
> On 28 July 2017 at 07:40, Andrew Nester <[email protected]> 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

On 31 July 2017 at 08:21, Andrew Nester <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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 <[email protected]> wrote:
>
> On 31 July 2017 at 08:21, Andrew Nester <[email protected]> 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

> On Jul 31, 2017, at 2:17 PM, Dan Ackroyd <[email protected]> wrote:
>
> On 31 July 2017 at 08:21, Andrew Nester <[email protected]> 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

On 31 July 2017 at 12:49, Andrew Nester <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

> On Jul 31, 2017, at 2:59 PM, Dan Ackroyd <[email protected]> wrote:
>
> On 31 July 2017 at 12:49, Andrew Nester <[email protected]> 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)

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

> On Jul 31, 2017, at 5:54 PM, Johannes Schlüter <[email protected]> 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"

> On Jul 31, 2017, at 6:33 PM, Johannes Schlüter <[email protected]> wrote:
>
> (off-list as this is distracting the discussion)
>
> On Mo, 2017-07-31 at 18:00 +0300, Andrew Nester wrote:
>>
>>> On Jul 31, 2017, at 5:54 PM, Johannes Schlüter <[email protected]
>>> .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
> [...]
>> 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"
>
> Yes, and my question is "why can't it?" I see no obvious reason in the
> code. If we can lift weird restrictions from internals this is always
> good ...
>
> johannes

I am thinking about writing an RFC for this and continue discussion there.
Will it be a good idea?

Andrew Nester wrote:
> I am thinking about writing an RFC for this and continue discussion there..
> Will it be a good idea?

You're apparently not very good at listening to suggestions, so I'll
be more direct.

I think you're wasting people's time until you actually try seeing if
PDO can be made to work with userland classes or not.

The next step should be you (or anyone else) attempting to make the
required changes to PDO to make it work, to discover what changes
would be needed to allow using userland classes.

If it isn't known what is needed to do to support it working, there is
no point raising the RFC.


Johannes Schlüter wrote:
> The actually question is: Why not?

My understanding is that some of the internal code that calls userland
functions and methods is basically bogus.

I had a PR (which I haven't had the energy to get round to sorting
out) to make the SPL call userland constructors correctly:
https://github.com/php/php-src/pull/1196

My surprise level would be zero, if there were similar issues in PDO
instantiating user classes, or other issues like the internal code
accessing properties directly without going through method access.

On the other hand, it might 'just work'.

cheers
Dan
Ack

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

> 6 авг. 2017 г., в 16:30, Dan Ackroyd <[email protected]> написал(а):
>
> Andrew Nester wrote:
>> I am thinking about writing an RFC for this and continue discussion there..
>> Will it be a good idea?
>
> You're apparently not very good at listening to suggestions, so I'll
> be more direct.
>
> I think you're wasting people's time until you actually try seeing if
> PDO can be made to work with userland classes or not.
>
> The next step should be you (or anyone else) attempting to make the
> required changes to PDO to make it work, to discover what changes
> would be needed to allow using userland classes.
>
> If it isn't known what is needed to do to support it working, there is
> no point raising the RFC.
>
>
> Johannes Schlüter wrote:
>> The actually question is: Why not?
>
> My understanding is that some of the internal code that calls userland
> functions and methods is basically bogus.
>
> I had a PR (which I haven't had the energy to get round to sorting
> out) to make the SPL call userland constructors correctly:
> https://github.com/php/php-src/pull/1196
>
> My surprise level would be zero, if there were similar issues in PDO
> instantiating user classes, or other issues like the internal code
> accessing properties directly without going through method access.
>
> On the other hand, it might 'just work'.
>
> cheers
> Dan
> Ack

Thanks for your feedback, I appreciate this.

Yes, I understand what you mean.
Fixing using this PDO attribute will solve issue with persistent PDO and will allow to customize PDO in all cases.

But what I want to suggest is more straightforward way to customize PDO behaviour and have proper type hints at the same time, because at my opinion using PDO attributes for customization is not very straightforward.

I shown you some examples of use cases of these interfaces. As I think example from Doctrine DBAL shows that this might be useful. Let me know if you would like to see some others.

I would like to hear some arguments why my proposed solution will not work or what exactly wrong with interfaces proposed and solution in general.

Thanks in advance.



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Am 06.08.2017 um 17:12 schrieb Andrew Nester:
>
>
>> 6 авг. 2017 г., в 16:30, Dan Ackroyd <[email protected]> написал(а):
>>
>> Andrew Nester wrote:
>>> I am thinking about writing an RFC for this and continue discussion there.
>>> Will it be a good idea?
>>
>> You're apparently not very good at listening to suggestions, so I'll
>> be more direct.
>>
>> I think you're wasting people's time until you actually try seeing if
>> PDO can be made to work with userland classes or not.
>>
>> The next step should be you (or anyone else) attempting to make the
>> required changes to PDO to make it work, to discover what changes
>> would be needed to allow using userland classes.
>>
>> If it isn't known what is needed to do to support it working, there is
>> no point raising the RFC.
>>
>>
>> Johannes Schlüter wrote:
>>> The actually question is: Why not?
>>
>> My understanding is that some of the internal code that calls userland
>> functions and methods is basically bogus.
>>
>> I had a PR (which I haven't had the energy to get round to sorting
>> out) to make the SPL call userland constructors correctly:
>> https://github.com/php/php-src/pull/1196
>>
>> My surprise level would be zero, if there were similar issues in PDO
>> instantiating user classes, or other issues like the internal code
>> accessing properties directly without going through method access.
>>
>> On the other hand, it might 'just work'.
>>
>> cheers
>> Dan
>> Ack
>
> Thanks for your feedback, I appreciate this.
>
> Yes, I understand what you mean.
> Fixing using this PDO attribute will solve issue with persistent PDO and will allow to customize PDO in all cases.
>
> But what I want to suggest is more straightforward way to customize PDO behaviour and have proper type hints at the same time, because at my opinion using PDO attributes for customization is not very straightforward.
>
> I shown you some examples of use cases of these interfaces. As I think example from Doctrine DBAL shows that this might be useful. Let me know if you would like to see some others.
>
> I would like to hear some arguments why my proposed solution will not work or what exactly wrong with interfaces proposed and solution in general.
>
> Thanks in advance.
>
>

Let me revive this thread. I submitted bug #74957 and therefore I am in
favor of the interfaces. My original motivation for the bug report was
having a custom implementation which would be able to catch from the
original PDO "MySQL server has gone away" and broken pipes, arising from
idle connections being reused after a while. This not only happens in
mostly idle daemons, but could also happen in this circumstance:

1. two connections are opened: log and work.
2. log connection is used.
3. work connection is used, blocking for several minutes.
4. log connection is used again.

Result is broken pipe for the log connection.

Now, I'm not too sure if the custom PDO implementation will help for
this use case (catching broken pipes), because in one app instance PDO
triggers an error and in another instance it causes a shutdown. Same app
version, both PHP 7.1.

The interfaces would still be a nice addition. They provide a good basis
for experimentation. They allow general straight forward customization
of PDO, i.e. not having to find out about the arcane and funny looking
customization method PDO:: ATTR_STATEMENT_CLASS, which is also
restricted to PDOStatement customization.

Best regards
Aljosha

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 15 February 2018 at 16:53, Aljosha Papsch <[email protected]> wrote:
>
> Let me revive this thread.


So.......have you looked at doing the suggested next steps? Or
possibly offered to pay someone to do it?


On 6 August 2017 at 14:30, Dan Ackroyd <[email protected]> wrote:
>
> The next step should be you (or anyone else) attempting to make the
> required changes to PDO to make it work, to discover what changes
> would be needed to allow using userland classes.
>
> If it isn't known what is needed to do to support it working, there is
> no point raising the RFC.


To be clear - this idea sounds like a good one.

But it needs someone to do the work, rather than more discussion about.

cheers
Dan
Ack

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php