Move fix for user provided pdo connection #4621

bizurkur · 2021-04-27T12:12:37Z

Q	A
Type	bug
BC Break	yes
Fixed issues	#4619

Summary

Commit 4297f50 broke Connection when passing in $params['pdo']

#4590 says it should have fixed the issue, but I'm using 2.13.1 and it still seems to be broken.

I took the fix that was already applied in #4590 and moved it to the Connection class. This way it gets applied to all connections, not just connections built using the DriverManager.

If it's a good idea to keep the fix applied inside DriverManager, too, please let me know and I can add that back.

How to reproduce

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Driver\PDO\MySQL\Driver;

$params = [
    'pdo' => new \PDO('mysql:<whatever>'),
];
$driver = new Driver();

$connection = new Connection($params, $driver);
// This is a query that Doctrine Migrations tool does:
$connection->executeQuery("SHOW FULL TABLES WHERE Table_type = 'BASE TABLE'");

Also, I love Doctrine. You all are doing awesome work!

bizurkur · 2021-04-27T12:14:47Z

Oh, and I opened this PR against 2.12.x because it looked like the code had the same issue in that branch. Please let me know if I need to open a separate PR against 2.13.x or if 2.12.x will get merged into 2.13.x.

mdumoulin · 2021-04-27T16:28:29Z

lib/Doctrine/DBAL/Connection.php

@@ -195,6 +197,7 @@ public function __construct(

        if (isset($params['pdo'])) {
            $this->_conn = $params['pdo'];
+            $this->_conn->setAttribute(PDO::ATTR_STATEMENT_CLASS, [PDODriverStatement::class, []]);


Maybe we should add some logic of the DriverManager here :

Check that pdo is a PDO instance

Trigger a deprecation

If we do so, I'm not sure that deprecation in DriverManager will still be useful. Someone using factory would get 2 deprecation.

Moved the deprecation warning to trigger from Connection and removed it from DriverManager

Added the PDO check

mdumoulin · 2021-04-27T16:34:49Z

tests/Doctrine/Tests/DBAL/ConnectionTest.php

@@ -701,7 +704,7 @@ public static function dataCallConnectOnce(): iterable
    public function testCallConnectOnce(string $method, array $params): void
    {
        $driverMock   = $this->createMock(Driver::class);
-        $pdoMock      = $this->createMock(Connection::class);
+        $pdoMock      = $this->createMock(PDO::class);
        $platformMock = $this->createMock(AbstractPlatform::class);
        $stmtMock     = $this->createMock(Statement::class);



Not sure that your tests modification actually test your modification.

I would rather add a functional test where the connection is not built by DriverManager :

dbal/tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

Lines 354 to 363 in 1372a42

public function testUserProvidedPDOConnection(): void

{

$connection = DriverManager::getConnection([

'pdo' => new PDO('sqlite::memory:'),

]);

$result = $connection->executeQuery('SELECT 1');

self::assertInstanceOf(ForwardCompatibility\Result::class, $result);

}

greg0ire · 2021-04-27T16:59:35Z

2.12.x is no longer maintained, please target 2.13 instead.

lib/Doctrine/DBAL/Connection.php

morozov · 2021-05-02T17:54:09Z

lib/Doctrine/DBAL/Connection.php

@@ -193,8 +195,20 @@ public function __construct(
        $this->_driver = $driver;
        $this->params  = $params;

+        // check for existing pdo object


This comment is redundant. The code is self-explanatory.

lib/Doctrine/DBAL/DriverManager.php

tests/Doctrine/Tests/DBAL/ConnectionTest.php

morozov · 2021-05-02T18:09:51Z

Property Doctrine\DBAL\Connection::$_conn (Doctrine\DBAL\Driver\Connection|null) does not accept PDO.

This error probably will have to be suppressed since the library violates its own API by allowing a PDO instance to be used instead of a driver connection.

morozov · 2021-05-02T18:11:41Z

Please rebase on top of 2.13.x to fix the MySQL build failures (see #4625).

bizurkur · 2021-05-13T20:07:08Z

Please rebase on top of 2.13.x to fix the MySQL build failures (see #4625).

Rebase pushed

andrewkhan1 · 2021-06-14T08:52:18Z

Any luck getting this one pushed? It is preventing us from updating some of our dependencies. Thanks for all of your hard work everyone! 💯

morozov · 2021-06-14T16:44:35Z

Somehow I missed all the updates after my last review. Will take a look. Thanks for the reminder, @andrewkhan1.

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

morozov

Looks good overall. Please see the comments and squash commits.

bizurkur · 2021-06-15T03:42:52Z

Sorry, but I'm just gonna close this. The PR was meant to be an extremely simple fix to move the code from #4590 into another class, nothing more. Yet it seems like each time I make the changes you guys ask for, you review it again and ask for different changes. I was only trying to move one line of code to a better location.

Feel free to re-open, copy/paste to a new PR, do whatever you want. I personally don't need this fix anymore so I've lost interest.

greg0ire · 2021-06-15T07:20:45Z

Come on! I hate it when a contributor gets discouraged at 99% of the task! Let me do the remaining 1%, it would be a shame to waste your efforts so close to the finish line.

The goal is to apply the fix to all connections, not just connections built using the DriverManager. Fixes #4619

morozov · 2021-06-27T19:22:53Z

Thanks, @bizurkur.

mdumoulin reviewed Apr 27, 2021

View reviewed changes

greg0ire changed the base branch from 2.12.x to 2.13.x April 27, 2021 16:59

morozov reviewed Apr 27, 2021

View reviewed changes

lib/Doctrine/DBAL/Connection.php Outdated Show resolved Hide resolved

morozov reviewed May 2, 2021

View reviewed changes

morozov reviewed Jun 15, 2021

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php Outdated Show resolved Hide resolved

morozov reviewed Jun 15, 2021

View reviewed changes

bizurkur closed this Jun 15, 2021

greg0ire reopened this Jun 15, 2021

greg0ire requested a review from morozov June 15, 2021 07:20

Move fix for user provided pdo connection to Connection class

9fe5679

The goal is to apply the fix to all connections, not just connections built using the DriverManager. Fixes #4619

morozov added BC Fix Configuration Connections PDO labels Jun 27, 2021

morozov linked an issue Jun 27, 2021 that may be closed by this pull request

ensureForwardCompatibilityStatement broke existing code #4619

Closed

morozov added this to the 2.13.3 milestone Jun 27, 2021

morozov approved these changes Jun 27, 2021

View reviewed changes

morozov merged commit 6fe08ab into doctrine:2.13.x Jun 27, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move fix for user provided pdo connection #4621

Move fix for user provided pdo connection #4621

bizurkur commented Apr 27, 2021

bizurkur commented Apr 27, 2021 •

edited

Loading

mdumoulin Apr 27, 2021

bizurkur Apr 27, 2021

mdumoulin Apr 27, 2021 •

edited

Loading

bizurkur Apr 27, 2021

greg0ire commented Apr 27, 2021

morozov May 2, 2021

bizurkur May 13, 2021

morozov commented May 2, 2021

morozov commented May 2, 2021

bizurkur commented May 13, 2021

andrewkhan1 commented Jun 14, 2021

morozov commented Jun 14, 2021

morozov left a comment

bizurkur commented Jun 15, 2021

greg0ire commented Jun 15, 2021

morozov commented Jun 27, 2021

	public function testUserProvidedPDOConnection(): void
	{
	$connection = DriverManager::getConnection([
	'pdo' => new PDO('sqlite::memory:'),
	]);

	$result = $connection->executeQuery('SELECT 1');

	self::assertInstanceOf(ForwardCompatibility\Result::class, $result);
	}

Move fix for user provided pdo connection #4621

Move fix for user provided pdo connection #4621

Conversation

bizurkur commented Apr 27, 2021

Summary

How to reproduce

bizurkur commented Apr 27, 2021 • edited Loading

mdumoulin Apr 27, 2021

Choose a reason for hiding this comment

bizurkur Apr 27, 2021

Choose a reason for hiding this comment

mdumoulin Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

bizurkur Apr 27, 2021

Choose a reason for hiding this comment

greg0ire commented Apr 27, 2021

morozov May 2, 2021

Choose a reason for hiding this comment

bizurkur May 13, 2021

Choose a reason for hiding this comment

morozov commented May 2, 2021

morozov commented May 2, 2021

bizurkur commented May 13, 2021

andrewkhan1 commented Jun 14, 2021

morozov commented Jun 14, 2021

morozov left a comment

Choose a reason for hiding this comment

bizurkur commented Jun 15, 2021

greg0ire commented Jun 15, 2021

morozov commented Jun 27, 2021

bizurkur commented Apr 27, 2021 •

edited

Loading

mdumoulin Apr 27, 2021 •

edited

Loading