From d671145572fd3983f2a6170b4d2b116b77d3b82a Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Sun, 8 Nov 2020 23:17:59 +0100
Subject: [PATCH 1/6] Failing test case for #4400

---
 .../Tests/DBAL/Functional/LockModeTest.php    | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php

diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
new file mode 100644
index 00000000000..e4bcbe449ef
--- /dev/null
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\DBAL\Functional;
+
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Exception;
+use Doctrine\DBAL\LockMode;
+use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\DBAL\Schema\Table;
+use Doctrine\DBAL\TransactionIsolationLevel;
+use Doctrine\Tests\DbalTestCase;
+use Doctrine\Tests\TestUtil;
+
+class LockModeTest extends DbalTestCase
+{
+    /** @var Connection */
+    private $c1;
+
+    /** @var Connection */
+    private $c2;
+
+    public function setUp(): void
+    {
+        $this->c1 = TestUtil::getConnection();
+        $this->c2 = TestUtil::getConnection();
+
+        $table = new Table('users');
+        $table->addColumn('id', 'integer');
+
+        $this->c1->getSchemaManager()->createTable($table);
+
+        if ($this->c2->getSchemaManager()->tablesExist('users')) {
+            return;
+        }
+
+        if ($this->c2->getDatabasePlatform() instanceof SqlitePlatform) {
+            self::markTestSkipped('This test cannot run on SQLite using an in-memory database');
+        }
+
+        self::fail('Separate connections do not seem to talk to the same database');
+    }
+
+    public function tearDown(): void
+    {
+        $this->c1->getSchemaManager()->dropTable('users');
+
+        $this->c1->close();
+        $this->c2->close();
+    }
+
+    public function testLockModeNoneDoesNotBreakTransactionIsolation(): void
+    {
+        try {
+            $this->c1->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
+            $this->c2->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
+        } catch (Exception $e) {
+            self::markTestSkipped('This test must be able to set a transaction isolation level');
+        }
+
+        $this->c1->beginTransaction();
+        $this->c2->beginTransaction();
+
+        $this->c1->insert('users', ['id' => 1]);
+
+        $query = 'SELECT id FROM users';
+        $query = $this->c2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE);
+
+        self::assertFalse($this->c2->fetchOne($query));
+
+        $this->c1->commit();
+        $this->c2->commit();
+    }
+}

From d1dd96c332d40b3630f79604ced872c57e23f27f Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Mon, 9 Nov 2020 10:06:34 +0100
Subject: [PATCH 2/6] Attempt to debug Oracle

---
 phpunit.xml.dist                              |  4 +-
 .../DBAL/Functional/LockModeSQLLogger.php     | 47 +++++++++++++++++++
 .../Tests/DBAL/Functional/LockModeTest.php    | 27 +++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php

diff --git a/phpunit.xml.dist b/phpunit.xml.dist
index f79efc83e40..85e41c7b4f7 100644
--- a/phpunit.xml.dist
+++ b/phpunit.xml.dist
@@ -23,14 +23,14 @@
         <ini name="error_reporting" value="-1" />
 
         <!-- Test connection parameters -->
-        <!-- Uncomment, otherwise SQLite runs
+
         <var name="db_driver" value="pdo_mysql"/>
         <var name="db_host" value="localhost" />
         <var name="db_port" value="3306"/>
         <var name="db_user" value="root" />
         <var name="db_password" value="" />
         <var name="db_dbname" value="doctrine_tests" />
-        -->
+
         <!--<var name="db_event_subscribers" value="Doctrine\DBAL\Event\Listeners\OracleSessionInit">-->
 
         <!-- Privileged user connection parameters. Used to create and drop the test database -->
diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php
new file mode 100644
index 00000000000..812b8791977
--- /dev/null
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php
@@ -0,0 +1,47 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\DBAL\Functional;
+
+use Doctrine\DBAL\Logging\SQLLogger;
+
+use function microtime;
+
+class LockModeSQLLogger implements SQLLogger
+{
+    /** @var int */
+    private $cid;
+
+    /** @var list<array{cid: int, time: float, sql: string}> */
+    private $queries = [];
+
+    public function __construct(int $cid)
+    {
+        $this->cid = $cid;
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function startQuery($sql, ?array $params = null, ?array $types = null): void
+    {
+        $this->queries[] = [
+            'cid' => $this->cid,
+            'time' => microtime(true),
+            'sql' => $sql,
+        ];
+    }
+
+    public function stopQuery(): void
+    {
+    }
+
+    /**
+     * @return list<array{cid: int, time: float, sql: string}>
+     */
+    public function getQueries(): array
+    {
+        return $this->queries;
+    }
+}
diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
index e4bcbe449ef..756085a6335 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -13,6 +13,9 @@
 use Doctrine\Tests\DbalTestCase;
 use Doctrine\Tests\TestUtil;
 
+use function array_merge;
+use function usort;
+
 class LockModeTest extends DbalTestCase
 {
     /** @var Connection */
@@ -21,11 +24,20 @@ class LockModeTest extends DbalTestCase
     /** @var Connection */
     private $c2;
 
+    /** @var LockModeSQLLogger */
+    private $c1Logger;
+
+    /** @var LockModeSQLLogger */
+    private $c2Logger;
+
     public function setUp(): void
     {
         $this->c1 = TestUtil::getConnection();
         $this->c2 = TestUtil::getConnection();
 
+        $this->c1->getConfiguration()->setSQLLogger($this->c1Logger = new LockModeSQLLogger(1));
+        $this->c2->getConfiguration()->setSQLLogger($this->c2Logger = new LockModeSQLLogger(2));
+
         $table = new Table('users');
         $table->addColumn('id', 'integer');
 
@@ -48,6 +60,21 @@ public function tearDown(): void
 
         $this->c1->close();
         $this->c2->close();
+
+        $queries = array_merge(
+            $this->c1Logger->getQueries(),
+            $this->c2Logger->getQueries()
+        );
+
+        usort($queries, static function (array $a, array $b) {
+            return (int) (1000000 * ($a['time'] - $b['time']));
+        });
+
+        echo "\n";
+
+        foreach ($queries as $query) {
+            echo 'C' . $query['cid'] . ': ' . $query['sql'] . "\n";
+        }
     }
 
     public function testLockModeNoneDoesNotBreakTransactionIsolation(): void

From 6f97bda18a4baf91f82769a92854983f8c56744c Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Tue, 10 Nov 2020 14:12:56 +0100
Subject: [PATCH 3/6] Revert "Attempt to debug Oracle"

This reverts commit d1dd96c3
---
 phpunit.xml.dist                              |  4 +-
 .../DBAL/Functional/LockModeSQLLogger.php     | 47 -------------------
 .../Tests/DBAL/Functional/LockModeTest.php    | 27 -----------
 3 files changed, 2 insertions(+), 76 deletions(-)
 delete mode 100644 tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php

diff --git a/phpunit.xml.dist b/phpunit.xml.dist
index 85e41c7b4f7..f79efc83e40 100644
--- a/phpunit.xml.dist
+++ b/phpunit.xml.dist
@@ -23,14 +23,14 @@
         <ini name="error_reporting" value="-1" />
 
         <!-- Test connection parameters -->
-
+        <!-- Uncomment, otherwise SQLite runs
         <var name="db_driver" value="pdo_mysql"/>
         <var name="db_host" value="localhost" />
         <var name="db_port" value="3306"/>
         <var name="db_user" value="root" />
         <var name="db_password" value="" />
         <var name="db_dbname" value="doctrine_tests" />
-
+        -->
         <!--<var name="db_event_subscribers" value="Doctrine\DBAL\Event\Listeners\OracleSessionInit">-->
 
         <!-- Privileged user connection parameters. Used to create and drop the test database -->
diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php
deleted file mode 100644
index 812b8791977..00000000000
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeSQLLogger.php
+++ /dev/null
@@ -1,47 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Doctrine\Tests\DBAL\Functional;
-
-use Doctrine\DBAL\Logging\SQLLogger;
-
-use function microtime;
-
-class LockModeSQLLogger implements SQLLogger
-{
-    /** @var int */
-    private $cid;
-
-    /** @var list<array{cid: int, time: float, sql: string}> */
-    private $queries = [];
-
-    public function __construct(int $cid)
-    {
-        $this->cid = $cid;
-    }
-
-    /**
-     * @inheritDoc
-     */
-    public function startQuery($sql, ?array $params = null, ?array $types = null): void
-    {
-        $this->queries[] = [
-            'cid' => $this->cid,
-            'time' => microtime(true),
-            'sql' => $sql,
-        ];
-    }
-
-    public function stopQuery(): void
-    {
-    }
-
-    /**
-     * @return list<array{cid: int, time: float, sql: string}>
-     */
-    public function getQueries(): array
-    {
-        return $this->queries;
-    }
-}
diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
index 756085a6335..e4bcbe449ef 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -13,9 +13,6 @@
 use Doctrine\Tests\DbalTestCase;
 use Doctrine\Tests\TestUtil;
 
-use function array_merge;
-use function usort;
-
 class LockModeTest extends DbalTestCase
 {
     /** @var Connection */
@@ -24,20 +21,11 @@ class LockModeTest extends DbalTestCase
     /** @var Connection */
     private $c2;
 
-    /** @var LockModeSQLLogger */
-    private $c1Logger;
-
-    /** @var LockModeSQLLogger */
-    private $c2Logger;
-
     public function setUp(): void
     {
         $this->c1 = TestUtil::getConnection();
         $this->c2 = TestUtil::getConnection();
 
-        $this->c1->getConfiguration()->setSQLLogger($this->c1Logger = new LockModeSQLLogger(1));
-        $this->c2->getConfiguration()->setSQLLogger($this->c2Logger = new LockModeSQLLogger(2));
-
         $table = new Table('users');
         $table->addColumn('id', 'integer');
 
@@ -60,21 +48,6 @@ public function tearDown(): void
 
         $this->c1->close();
         $this->c2->close();
-
-        $queries = array_merge(
-            $this->c1Logger->getQueries(),
-            $this->c2Logger->getQueries()
-        );
-
-        usort($queries, static function (array $a, array $b) {
-            return (int) (1000000 * ($a['time'] - $b['time']));
-        });
-
-        echo "\n";
-
-        foreach ($queries as $query) {
-            echo 'C' . $query['cid'] . ': ' . $query['sql'] . "\n";
-        }
     }
 
     public function testLockModeNoneDoesNotBreakTransactionIsolation(): void

From b8c502c677a33513ef5d65ed490f33e594683282 Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Tue, 10 Nov 2020 14:15:02 +0100
Subject: [PATCH 4/6] Skip test on OCI8

---
 tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
index e4bcbe449ef..fb8cd77d323 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -5,6 +5,7 @@
 namespace Doctrine\Tests\DBAL\Functional;
 
 use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Driver\OCI8;
 use Doctrine\DBAL\Exception;
 use Doctrine\DBAL\LockMode;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
@@ -39,6 +40,11 @@ public function setUp(): void
             self::markTestSkipped('This test cannot run on SQLite using an in-memory database');
         }
 
+        if ($this->c2->getDriver() instanceof OCI8\Driver) {
+            // https://github.com/doctrine/dbal/issues/4417
+            self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
+        }
+
         self::fail('Separate connections do not seem to talk to the same database');
     }
 

From 38ae380df38a7619261f6999068859c6ce5b147a Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Tue, 10 Nov 2020 14:19:10 +0100
Subject: [PATCH 5/6] Fail early on OCI8

---
 tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
index fb8cd77d323..2b61aaeea75 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -27,6 +27,11 @@ public function setUp(): void
         $this->c1 = TestUtil::getConnection();
         $this->c2 = TestUtil::getConnection();
 
+        if ($this->c1->getDriver() instanceof OCI8\Driver) {
+            // https://github.com/doctrine/dbal/issues/4417
+            self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
+        }
+
         $table = new Table('users');
         $table->addColumn('id', 'integer');
 
@@ -40,11 +45,6 @@ public function setUp(): void
             self::markTestSkipped('This test cannot run on SQLite using an in-memory database');
         }
 
-        if ($this->c2->getDriver() instanceof OCI8\Driver) {
-            // https://github.com/doctrine/dbal/issues/4417
-            self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
-        }
-
         self::fail('Separate connections do not seem to talk to the same database');
     }
 

From e80eee222981ca91c560044720174feb53bb4e0e Mon Sep 17 00:00:00 2001
From: Benjamin Morel <benjamin.morel@gmail.com>
Date: Thu, 12 Nov 2020 13:49:07 +0100
Subject: [PATCH 6/6] Update failing test case to match #4400

---
 .../Tests/DBAL/Functional/LockModeTest.php    | 67 ++++++++++++-------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
index 2b61aaeea75..e1434406d9c 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/LockModeTest.php
@@ -9,39 +9,50 @@
 use Doctrine\DBAL\Exception;
 use Doctrine\DBAL\LockMode;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
 use Doctrine\DBAL\Schema\Table;
 use Doctrine\DBAL\TransactionIsolationLevel;
-use Doctrine\Tests\DbalTestCase;
+use Doctrine\Tests\DbalFunctionalTestCase;
 use Doctrine\Tests\TestUtil;
 
-class LockModeTest extends DbalTestCase
+class LockModeTest extends DbalFunctionalTestCase
 {
     /** @var Connection */
-    private $c1;
-
-    /** @var Connection */
-    private $c2;
+    private $connection2;
 
     public function setUp(): void
     {
-        $this->c1 = TestUtil::getConnection();
-        $this->c2 = TestUtil::getConnection();
+        parent::setUp();
 
-        if ($this->c1->getDriver() instanceof OCI8\Driver) {
+        if ($this->connection->getDriver() instanceof OCI8\Driver) {
             // https://github.com/doctrine/dbal/issues/4417
             self::markTestSkipped('This test fails on OCI8 for a currently unknown reason');
         }
 
+        if ($this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
+            // Use row versioning instead of locking on SQL Server (if we don't, the second connection will block when
+            // attempting to read the row created by the first connection, instead of reading the previous version);
+            // for some reason we cannot set READ_COMMITTED_SNAPSHOT ON when not running this test in isolation,
+            // there may be another connection active at this point; temporarily forcing to SINGLE_USER does the trick.
+            $name = $this->connection->fetchOne('SELECT db_name()');
+            $this->connection->executeStatement('ALTER DATABASE ' . $name . ' SET SINGLE_USER WITH ROLLBACK IMMEDIATE');
+            $this->connection->executeStatement('ALTER DATABASE ' . $name . ' SET READ_COMMITTED_SNAPSHOT ON');
+            $this->connection->executeStatement('ALTER DATABASE ' . $name . ' SET MULTI_USER');
+        }
+
         $table = new Table('users');
         $table->addColumn('id', 'integer');
+        $table->setPrimaryKey(['id']);
+
+        $this->connection->getSchemaManager()->createTable($table);
 
-        $this->c1->getSchemaManager()->createTable($table);
+        $this->connection2 = TestUtil::getConnection();
 
-        if ($this->c2->getSchemaManager()->tablesExist('users')) {
+        if ($this->connection2->getSchemaManager()->tablesExist('users')) {
             return;
         }
 
-        if ($this->c2->getDatabasePlatform() instanceof SqlitePlatform) {
+        if ($this->connection2->getDatabasePlatform() instanceof SqlitePlatform) {
             self::markTestSkipped('This test cannot run on SQLite using an in-memory database');
         }
 
@@ -50,32 +61,40 @@ public function setUp(): void
 
     public function tearDown(): void
     {
-        $this->c1->getSchemaManager()->dropTable('users');
+        parent::tearDown();
+
+        $this->connection2->close();
+
+        $this->connection->getSchemaManager()->dropTable('users');
+
+        if (! $this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
+            return;
+        }
 
-        $this->c1->close();
-        $this->c2->close();
+        $name = $this->connection->fetchOne('SELECT db_name()');
+        $this->connection->executeStatement('ALTER DATABASE ' . $name . ' SET READ_COMMITTED_SNAPSHOT OFF');
     }
 
     public function testLockModeNoneDoesNotBreakTransactionIsolation(): void
     {
         try {
-            $this->c1->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
-            $this->c2->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
+            $this->connection->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
+            $this->connection2->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);
         } catch (Exception $e) {
             self::markTestSkipped('This test must be able to set a transaction isolation level');
         }
 
-        $this->c1->beginTransaction();
-        $this->c2->beginTransaction();
+        $this->connection->beginTransaction();
+        $this->connection2->beginTransaction();
 
-        $this->c1->insert('users', ['id' => 1]);
+        $this->connection->insert('users', ['id' => 1]);
 
         $query = 'SELECT id FROM users';
-        $query = $this->c2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE);
+        $query = $this->connection2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE);
 
-        self::assertFalse($this->c2->fetchOne($query));
+        self::assertSame([], $this->connection2->fetchAllNumeric($query));
 
-        $this->c1->commit();
-        $this->c2->commit();
+        $this->connection->commit();
+        $this->connection2->commit();
     }
 }