Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Connection::getNativeConnection() #5037

Merged
merged 1 commit into from Nov 27, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Nov 27, 2021

Q A
Type feature
BC Break no
Fixed issues N/A

Summary

As discussed on #4966, accessing the native connection is a rare but valid use-case.

  • Symfony Messenger ueses it to access the notify API from Postgres
  • Sometimes, an application might want to temporarily alter attributes of the PDO connection.
  • On batch operations, applications running on MySQL might want to switch to unbuffered queries.
  • Our own portability middleware does this for PDO connections

Unpacking the native connection is tedious and fragile. And with the middleware system, it gets close to impossible to access the native connection. Even our own portability middleware will fail to access it if we move it higher in the middleware stack.

Making the native connection accessible through a userland middleware as suggested in #4966 won't work reliably either because that middleware would have to be registered pretty low in the middleware stack. As soon as there's a lower middleware that wraps the connection, our userland middleware would fail. Our middleware would basically have the same problem as the portability middleware.

This PR proposes to standardize the access to the native connection by adding dedicated methods to the Connection class and the drivers' Connection interface. It also deprecates various old accessors from driver connection implementations.

$this->connect();

assert($this->_conn !== null);
if (! method_exists($this->_conn, 'getNativeConnection')) {
Copy link
Member

@morozov morozov Nov 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to introduce a deprecated interface extending Connection and implement it where possible? In the next major, it will be merged with the driver-level Connection.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make it hard to implement a middleware in a third-party library that is compatible with DBAL 3.2, 3.3 and 4.0 at the same time. That middleware would have to implement an interface that does not exist in all versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to backport the new interface to 3.2.x and make the code better typed but I don't have a strong preference. The reasoning above makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, the introduction of the abstract middleware classes has opened up the path for the deprecated interface idea, I guess. A middleware would conditionally implement that interface via the abstract class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't the abstract class only available in 3.3.x as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but we would be able to build middlewares on 3.3 that are forward compatible with 4.0. That would be sufficient for me at least.

@morozov
Copy link
Member

morozov commented Nov 27, 2021

Our own portability middleware does this for PDO connections

This is a different problem. If we want to use native driver capabilities for case conversion, case conversion should be part of the driver API, not an external wrapper. Once instructed to convert column case, the driver should either do it natively or delegate it to a userland implementation. With this approach, there will be no need to use the native connection outside of the driver.

@derrabus derrabus force-pushed the improvement/native-connection branch from 5f822e5 to b06a406 Compare November 27, 2021 16:44
morozov
morozov previously approved these changes Nov 27, 2021
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Whichever of the two latest PRs gets merged last will need to implement the new method in the abstract middleware connection. Or it won't?

@derrabus
Copy link
Member Author

Whichever of the two latest PRs gets merged last will need to implement the new method in the abstract middleware connection. Or it won't?

I've merged the other PR and rebased this one. The new method is now implemented in AbstractConnectionMiddleware.

@derrabus derrabus force-pushed the improvement/native-connection branch from 2d338a7 to 6682bbd Compare November 27, 2021 17:01
src/Connection.php Outdated Show resolved Hide resolved
src/Driver/Mysqli/Connection.php Outdated Show resolved Hide resolved
@derrabus derrabus force-pushed the improvement/native-connection branch from 6682bbd to c8b05b0 Compare November 27, 2021 17:20
UPGRADE.md Outdated Show resolved Hide resolved
@derrabus derrabus force-pushed the improvement/native-connection branch from c8b05b0 to d9f969b Compare November 27, 2021 18:01
@derrabus derrabus merged commit bf37340 into doctrine:3.3.x Nov 27, 2021
@derrabus derrabus deleted the improvement/native-connection branch November 27, 2021 18:09
fabpot added a commit to symfony/symfony that referenced this pull request Nov 29, 2021
…derrabus)

This PR was merged into the 5.3 branch.

Discussion
----------

[Messenger] Leverage DBAL's getNativeConnection() method

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | N/A

Doctrine DBAL 3 introduced a new driver middleware system that makes it difficult to unwrap the native PDO connection which we need for Messenger's Postgres transport. If an application would actually make use of the middleware system, accessing the PDO connection would become impossible for us.

Because of that, I have added a method `getNativeConnection()` to DBAL with doctrine/dbal#5037. This PR leverages this new method.

Commits
-------

cde5ec7 Leverage DBAL's getNativeConnection() method
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants