Allow ConnectionLocator to be used with PDO Persistent Connections

[dbraff]

This PR modifies the LoggedStatement class to be a proxy wrapping the PDOStatement class, which is constructed by the Connection class injecting the PDOStatement into it when logging is enabled.

This allows persistent connections to be used along with the logging functionality and the ConnectionLocator.

In order to maintain the existing type hints for methods returning a PDOStatement instance, this was done by overriding each of the method of the parent class as opposed to using the magic __call method.

A test case was added to cover the case of a ConnectionLocator with a persistent connection, and an existing test case, which verified that PDO natively returned LoggedStatement instances, was removed as it would no longer be applicable.

[pmjones]

Nice. Give me a bit to look this over. Thanks!

[dbraff]

Thank you!

[dbraff]

Hi @pmjones, was wondering if you had any time to look at this further. If you have concerns or think a different approach would be better, please let me know.

[dbraff]

Hi @pmjones, I came back to look over this PR and realized what a mistake I had made in the changes to the LoggedStatement class. I've corrected this so that the overriding methods are forwarding the returns of the PDOStatement instance. Apologies for the oversight and if you have another chance to look this over please let me know if you have any concerns or feedback.

[dbraff]

Hi @pmjones, it's been a while since I last commented here on this. Could you please give an update on your thoughts for this PR and the issue it attempts to resolve? Any feedback would be greatly appreciated.

As an aside, it does appear that doctrine/dbal previously had a similar lack of support. That was resolved recently with this PR.
doctrine/dbal#3515

[pmjones]

Hey @dbraff looking at it today -- thanks for your patience!

[pmjones]

@dbraff Would it be fair to say that the bulk of the change is to explicitly add PDOStatement methods to LoggedStatement, so that the internal $statement methods called directly, instead of making parent::*() calls?

[dbraff]

Hi @pmjones, thanks for getting back to me on this!

@dbraff Would it be fair to say that the bulk of the change is to explicitly add PDOStatement methods to LoggedStatement, so that the internal $statement methods called directly, instead of making parent::*() calls?

Yes, I would agree that's fair to say. I wish there was a less repetitive way to wrap the PDOStatement class but I couldn't think of a better way to do that without causing a backwards compatibility issue with users of this package already potentially relying on PDOStatement type hints. If you have any suggestions for how to approach this differently, I'd love to hear them.

[pmjones]

I wish there was a less repetitive way to wrap

I don't think there is; one's first thought is to use __call()but it won't get invoked, since the methods exist in the parent class.

[dbraff]

I wish there was a less repetitive way to wrap

I don't think there is; one's first thought is to use __call()but it won't get invoked, since the methods exist in the parent class.

Yeah, that was my first thought as well.

[pmjones]

All right, I think this looks good! Couple of typos but nothing major. The LoggedStatement test coverage has dropped to 50%, though, because of the newly-added methods. Will merge, then add tests, then release.

Thanks for your work, and (again) for your patience!

[dbraff]

All right, I think this looks good! Couple of typos but nothing major. The LoggedStatement test coverage has dropped to 50%, though, because of the newly-added methods. Will merge, then add tests, then release.

Thanks for your work, and (again) for your patience!

No problem at all! Thank you taking the time to look at it, looking forward to bringing this library up again at my organization!