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

[Dan Ackroyd]

On 15 February 2018 at 16:53, Aljosha Papsch  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  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

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

[Aljosha Papsch]

Am 06.08.2017 um 17:12 schrieb Andrew Nester:




6 авг. 2017 г., в 16:30, Dan Ackroyd  написал(а):

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

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

[Andrew Nester]

> 6 авг. 2017 г., в 16:30, Dan Ackroyd  написал(а):
> 
> 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

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

[Dan Ackroyd]

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

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

[Andrew Nester]

> On Jul 31, 2017, at 6:33 PM, Johannes Schlüter  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 >> .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?

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

[Andrew Nester]

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

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

[Johannes Schlüter]

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

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

[Andrew Nester]

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


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

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

[Dan Ackroyd]

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

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

[Andrew Nester]

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

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

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

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

[Dan Ackroyd]

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

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

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

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

[Dan Ackroyd]

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

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

[Andrew Nester]

Hello!

I’ve been working on request introduced here 
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 


I would like to hear any feedback on it!
Thanks!