From bdb7983c8f81103434547da238b53f4302b74505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Thu, 22 Oct 2020 20:31:14 +0200 Subject: [PATCH 01/15] Bump version to 2.12.1-DEV --- .doctrine-project.json | 12 +++++++++--- README.md | 26 +++++++++++++------------- lib/Doctrine/DBAL/Version.php | 2 +- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/.doctrine-project.json b/.doctrine-project.json index ff81ef1e9f0..d1ffa1bd08c 100644 --- a/.doctrine-project.json +++ b/.doctrine-project.json @@ -12,15 +12,21 @@ "upcoming": true }, { - "name": "2.11", - "branchName": "2.11.x", - "slug": "2.11", + "name": "2.12", + "branchName": "2.12.x", + "slug": "2.12", "current": true, "aliases": [ "current", "stable" ] }, + { + "name": "2.11", + "branchName": "2.11.x", + "slug": "2.11", + "maintained": false + }, { "name": "2.10", "branchName": "2.10.x", diff --git a/README.md b/README.md index 8f21ee0b5b5..259f7c626a0 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,11 @@ # Doctrine DBAL -| [Master][Master] | [2.11][2.11] | +| [Master][Master] | [2.12][2.12] | |:----------------:|:----------:| -| [![Build status][Master image]][Master] | [![Build status][2.11 image]][2.11] | -| [![GitHub Actions][GA master image]][GA master] | [![GitHub Actions][GA 2.11 image]][GA 2.11] | -| [![AppVeyor][AppVeyor master image]][AppVeyor master] | [![AppVeyor][AppVeyor 2.11 image]][AppVeyor 2.11] | -| [![Code Coverage][Coverage image]][CodeCov Master] | [![Code Coverage][Coverage 2.11 image]][CodeCov 2.11] | +| [![Build status][Master image]][Master] | [![Build status][2.12 image]][2.12] | +| [![GitHub Actions][GA master image]][GA master] | [![GitHub Actions][GA 2.12 image]][GA 2.12] | +| [![AppVeyor][AppVeyor master image]][AppVeyor master] | [![AppVeyor][AppVeyor 2.12 image]][AppVeyor 2.12] | +| [![Code Coverage][Coverage image]][CodeCov Master] | [![Code Coverage][Coverage 2.12 image]][CodeCov 2.12] | Powerful database abstraction layer with many features for database schema introspection, schema management and PDO abstraction. @@ -24,11 +24,11 @@ Powerful database abstraction layer with many features for database schema intro [GA master]: https://github.com/doctrine/dbal/actions?query=workflow%3A%22Continuous+Integration%22+branch%3Amaster [GA master image]: https://github.com/doctrine/dbal/workflows/Continuous%20Integration/badge.svg - [2.11 image]: https://img.shields.io/travis/doctrine/dbal/2.11.x.svg?style=flat-square - [Coverage 2.11 image]: https://codecov.io/gh/doctrine/dbal/branch/2.11.x/graph/badge.svg - [2.11]: https://github.com/doctrine/dbal/tree/2.11.x - [CodeCov 2.11]: https://codecov.io/gh/doctrine/dbal/branch/2.11.x - [AppVeyor 2.11]: https://ci.appveyor.com/project/doctrine/dbal/branch/2.11.x - [AppVeyor 2.11 image]: https://ci.appveyor.com/api/projects/status/i88kitq8qpbm0vie/branch/2.11.x?svg=true - [GA 2.11]: https://github.com/doctrine/dbal/actions?query=workflow%3A%22Continuous+Integration%22+branch%3A2.11.x - [GA 2.11 image]: https://github.com/doctrine/dbal/workflows/Continuous%20Integration/badge.svg?branch=2.11.x + [2.12 image]: https://img.shields.io/travis/doctrine/dbal/2.12.x.svg?style=flat-square + [Coverage 2.12 image]: https://codecov.io/gh/doctrine/dbal/branch/2.12.x/graph/badge.svg + [2.12]: https://github.com/doctrine/dbal/tree/2.12.x + [CodeCov 2.12]: https://codecov.io/gh/doctrine/dbal/branch/2.12.x + [AppVeyor 2.12]: https://ci.appveyor.com/project/doctrine/dbal/branch/2.12.x + [AppVeyor 2.12 image]: https://ci.appveyor.com/api/projects/status/i88kitq8qpbm0vie/branch/2.12.x?svg=true + [GA 2.12]: https://github.com/doctrine/dbal/actions?query=workflow%3A%22Continuous+Integration%22+branch%3A2.12.x + [GA 2.12 image]: https://github.com/doctrine/dbal/workflows/Continuous%20Integration/badge.svg?branch=2.12.x diff --git a/lib/Doctrine/DBAL/Version.php b/lib/Doctrine/DBAL/Version.php index bb8236fcbf9..85c3b8961cc 100644 --- a/lib/Doctrine/DBAL/Version.php +++ b/lib/Doctrine/DBAL/Version.php @@ -17,7 +17,7 @@ class Version /** * Current Doctrine Version. */ - public const VERSION = '2.12.0'; + public const VERSION = '2.12.1-DEV'; /** * Compares a Doctrine version with the current one. From a500d5e81ff5a6bdfdfc51444f32165cad253c8a Mon Sep 17 00:00:00 2001 From: Jacob Dreesen Date: Wed, 28 Oct 2020 13:05:57 +0100 Subject: [PATCH 02/15] Fix headline in the upgrade docs --- UPGRADE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UPGRADE.md b/UPGRADE.md index 132e058d08d..2500e7f12aa 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -76,7 +76,7 @@ The following PDO-related classes outside of the PDO namespace have been depreca 3. `prefersSequences()`. 4. `supportsForeignKeyOnUpdate()`. -##`ServerInfoAwareConnection::requiresQueryForServerVersion()` is deprecated. +## `ServerInfoAwareConnection::requiresQueryForServerVersion()` is deprecated. The `ServerInfoAwareConnection::requiresQueryForServerVersion()` method has been deprecated as an implementation detail which is the same for almost all supported drivers. From f80b055324b2577748570683fcb67e0c1ab7e3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sat, 31 Oct 2020 12:52:22 +0100 Subject: [PATCH 03/15] Remove segfault comment from PDOOracle driver --- lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php b/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php index b6792996c71..eff2f5ef49b 100644 --- a/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php +++ b/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php @@ -10,8 +10,7 @@ /** * PDO Oracle driver. * - * WARNING: This driver gives us segfaults in our testsuites on CLOB and other - * stuff. PDO Oracle is not maintained by Oracle or anyone in the PHP community, + * WARNING: PDO Oracle is not maintained by Oracle or anyone in the PHP community, * which leads us to the recommendation to use the "oci8" driver to connect * to Oracle instead. * From 09eafbc7ecc384210e54146b07421b37f7b3869a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Sat, 31 Oct 2020 23:33:43 +0100 Subject: [PATCH 04/15] Catch PDOException When using PDO, an exception is supposed to be thrown since we are using the error mode that behaves that way. It only seems to be the case since PHP 8 though. --- tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php b/tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php index 434bf37abe7..024feb8444f 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/TransactionTest.php @@ -4,6 +4,7 @@ use Doctrine\DBAL\Platforms\MySqlPlatform; use Doctrine\Tests\DbalFunctionalTestCase; +use PDOException; use function sleep; @@ -35,6 +36,12 @@ public function testCommitFalse(): void sleep(2); // during the sleep mysql will close the connection - $this->assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings + try { + $this->assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings + } catch (PDOException $e) { + /* For PDO, we are using ERRMODE EXCEPTION, so this catch should be + * necessary as the equivalent of the error control operator above. + * This seems to be the case only since PHP 8 */ + } } } From 435c7fd112c979f454f50c05132c9c1b2fb8ab8d Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 31 Oct 2020 17:27:47 -0700 Subject: [PATCH 05/15] Update PHP_CodeSniffer to 3.5.8 --- composer.lock | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/composer.lock b/composer.lock index 22f01fdd8f7..66ab60cc8b7 100644 --- a/composer.lock +++ b/composer.lock @@ -3197,16 +3197,16 @@ }, { "name": "squizlabs/php_codesniffer", - "version": "3.5.5", + "version": "3.5.8", "source": { "type": "git", "url": "https://github.com/squizlabs/PHP_CodeSniffer.git", - "reference": "73e2e7f57d958e7228fce50dc0c61f58f017f9f6" + "reference": "9d583721a7157ee997f235f327de038e7ea6dac4" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/73e2e7f57d958e7228fce50dc0c61f58f017f9f6", - "reference": "73e2e7f57d958e7228fce50dc0c61f58f017f9f6", + "url": "https://api.github.com/repos/squizlabs/PHP_CodeSniffer/zipball/9d583721a7157ee997f235f327de038e7ea6dac4", + "reference": "9d583721a7157ee997f235f327de038e7ea6dac4", "shasum": "" }, "require": { @@ -3249,7 +3249,7 @@ "source": "https://github.com/squizlabs/PHP_CodeSniffer", "wiki": "https://github.com/squizlabs/PHP_CodeSniffer/wiki" }, - "time": "2020-04-17T01:09:41+00:00" + "time": "2020-10-23T02:01:07+00:00" }, { "name": "symfony/console", From 29fc5c6590a1f02ce61583b8bd42a8c54bab9975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Sun, 1 Nov 2020 16:38:34 +0100 Subject: [PATCH 06/15] Remove no longer valid comment - pdo_oci is maintaned by php people --- lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php b/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php index eff2f5ef49b..b8e0a96f44e 100644 --- a/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php +++ b/lib/Doctrine/DBAL/Driver/PDOOracle/Driver.php @@ -10,10 +10,6 @@ /** * PDO Oracle driver. * - * WARNING: PDO Oracle is not maintained by Oracle or anyone in the PHP community, - * which leads us to the recommendation to use the "oci8" driver to connect - * to Oracle instead. - * * @deprecated Use {@link PDO\OCI\Driver} instead. */ class Driver extends AbstractOracleDriver From 7b4f16ada5ee4a72f4a099731683cda66161aca0 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 3 Nov 2020 10:49:47 -0800 Subject: [PATCH 07/15] Deprecate colon prefix for prepared statement parameters --- UPGRADE.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index 2500e7f12aa..161c9c75d73 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,22 @@ # Upgrade to 2.12 +## Deprecated colon prefix for prepared statement parameters + +The usage of the colon prefix when binding named parameters is deprecated. + +```php +$sql = 'SELECT * FROM users WHERE name = :name OR username = :username'; +$stmt = $conn->prepare($sql); + +// The usage of the leading colon is deprecated +$stmt->bindValue(':name', $name); + +// Only the parameter name should be passed +$stmt->bindValue('username', $username); + +$stmt->execute(); +``` + ## PDO signature changes with php 8 In php 8.0, the method signatures of two PDO classes which are extended by DBAL have changed. This affects the following classes: From 9babd9e6d1b5bddeb04cc548171147524112b414 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Wed, 4 Nov 2020 19:40:41 -0800 Subject: [PATCH 08/15] Deprecate inappropriate usage of prepared statement parameters --- UPGRADE.md | 31 +++++++++++++++++++ .../data-retrieval-and-manipulation.rst | 8 +++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index 161c9c75d73..2ea6c38351e 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,36 @@ # Upgrade to 2.12 +## Deprecated non-zero based positional parameter keys + +The usage of one-based and other non-zero-based keys when binding positional parameters is deprecated. + +It is recommended to not use any array keys so that the value of the parameter array complies with the [`list<>`](https://psalm.dev/docs/annotating_code/type_syntax/array_types/) type constraint. + +```php +// This is valid (implicit zero-based parameter indexes) +$conn->fetchNumeric('SELECT ?, ?', [1, 2]); + +// This is invalid (one-based parameter indexes) +$conn->fetchNumeric('SELECT ?, ?', [1 => 1, 2 => 2]); + +// This is invalid (arbitrary parameter indexes) +$conn->fetchNumeric('SELECT ?, ?', [-31 => 1, 5 => 2]); + +// This is invalid (non-sequential parameter indexes) +$conn->fetchNumeric('SELECT ?, ?', [0 => 1, 3 => 2]); +``` + +## Deprecated skipping prepared statement parameters + +Some underlying drivers currently allow skipping prepared statement parameters. For instance: + +```php +$conn->fetchOne('SELECT ?'); +// NULL +``` + +This behavior should not be relied upon and may change in future versions. + ## Deprecated colon prefix for prepared statement parameters The usage of the colon prefix when binding named parameters is deprecated. diff --git a/docs/en/reference/data-retrieval-and-manipulation.rst b/docs/en/reference/data-retrieval-and-manipulation.rst index 7e85e3c3a82..7902584c3de 100644 --- a/docs/en/reference/data-retrieval-and-manipulation.rst +++ b/docs/en/reference/data-retrieval-and-manipulation.rst @@ -91,9 +91,11 @@ are then replaced by their actual values in a second step (execute). $stmt->bindValue(1, $id); $stmt->execute(); -Placeholders in prepared statements are either simple positional question marks (?) or named labels starting with -a double-colon (:name1). You cannot mix the positional and the named approach. The approach -using question marks is called positional, because the values are bound in order from left to right +Placeholders in prepared statements are either simple positional question marks (``?``) or named labels starting with +a colon (e.g. ``:name1``). You cannot mix the positional and the named approach. You have to bind a parameter +to each placeholder. + +The approach using question marks is called positional, because the values are bound in order from left to right to any question mark found in the previously prepared SQL query. That is why you specify the position of the variable to bind into the ``bindValue()`` method: From 73229446693c2bdd1a52d5e0bce43b834ace05bf Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Mon, 2 Nov 2020 13:00:56 +0100 Subject: [PATCH 09/15] Remove redundant phpstan param from DriverManager::getConnection() This effectively prevented phpstan from inferring type of `T` template. > Unable to resolve the template type T in call to method static method Doctrine\DBAL\DriverManager::getConnection() --- lib/Doctrine/DBAL/DriverManager.php | 1 - tests/Doctrine/Tests/DBAL/ConnectionTest.php | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Doctrine/DBAL/DriverManager.php b/lib/Doctrine/DBAL/DriverManager.php index 17ba8584da2..6a11b489f4a 100644 --- a/lib/Doctrine/DBAL/DriverManager.php +++ b/lib/Doctrine/DBAL/DriverManager.php @@ -119,7 +119,6 @@ private function __construct() * * @throws Exception * - * @phpstan-param mixed[] $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ diff --git a/tests/Doctrine/Tests/DBAL/ConnectionTest.php b/tests/Doctrine/Tests/DBAL/ConnectionTest.php index 675881ac207..32129bf6c91 100644 --- a/tests/Doctrine/Tests/DBAL/ConnectionTest.php +++ b/tests/Doctrine/Tests/DBAL/ConnectionTest.php @@ -36,7 +36,7 @@ class ConnectionTest extends DbalTestCase /** @var Connection */ private $connection; - /** @var string[] */ + /** @var array{wrapperClass?: class-string} */ protected $params = [ 'driver' => 'pdo_mysql', 'host' => 'localhost', From 12508e6ac5b4c83548fbc26ba5e679e2d8bc9d72 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 7 Nov 2020 19:23:14 -0800 Subject: [PATCH 10/15] ResultCacheStatement::fetchAllAssociative does not store results in cache --- .../DBAL/Cache/ResultCacheStatement.php | 16 +++----- .../Tests/DBAL/Functional/ResultCacheTest.php | 39 ++++++++++++++++++- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php b/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php index 07a6c220611..61bb5022076 100644 --- a/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php +++ b/lib/Doctrine/DBAL/Cache/ResultCacheStatement.php @@ -242,7 +242,9 @@ public function fetchAllNumeric(): array $data = $this->statement->fetchAll(FetchMode::ASSOCIATIVE); } - $this->store($data); + $this->data = $data; + + $this->saveToCache(); return array_map('array_values', $data); } @@ -258,7 +260,9 @@ public function fetchAllAssociative(): array $data = $this->statement->fetchAll(FetchMode::ASSOCIATIVE); } - $this->store($data); + $this->data = $data; + + $this->saveToCache(); return $data; } @@ -322,14 +326,6 @@ private function doFetch() return false; } - /** - * @param array> $data - */ - private function store(array $data): void - { - $this->data = $data; - } - private function saveToCache(): void { if ($this->data === null) { diff --git a/tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php b/tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php index 05d3ff70cd2..5336a1053ea 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php +++ b/tests/Doctrine/Tests/DBAL/Functional/ResultCacheTest.php @@ -4,6 +4,7 @@ use Doctrine\Common\Cache\ArrayCache; use Doctrine\DBAL\Cache\QueryCacheProfile; +use Doctrine\DBAL\Cache\ResultCacheStatement; use Doctrine\DBAL\Driver\ResultStatement; use Doctrine\DBAL\FetchMode; use Doctrine\DBAL\Logging\DebugStack; @@ -212,7 +213,10 @@ public function testDontFinishNoCache(): void self::assertCount(2, $this->sqlLogger->queries); } - public function testFetchAllSavesCache(): void + /** + * @dataProvider fetchAllProvider + */ + public function testFetchingAllRowsSavesCache(callable $fetchAll): void { $layerCache = new ArrayCache(); @@ -222,11 +226,42 @@ public function testFetchAllSavesCache(): void [], new QueryCacheProfile(0, 'testcachekey', $layerCache) ); - $stmt->fetchAll(); + + $fetchAll($stmt); self::assertCount(1, $layerCache->fetch('testcachekey')); } + /** + * @return iterable> + */ + public static function fetchAllProvider(): iterable + { + yield 'fetchAll' => [ + static function (ResultCacheStatement $statement): void { + $statement->fetchAll(); + }, + ]; + + yield 'fetchAllAssociative' => [ + static function (ResultCacheStatement $statement): void { + $statement->fetchAllAssociative(); + }, + ]; + + yield 'fetchAllNumeric' => [ + static function (ResultCacheStatement $statement): void { + $statement->fetchAllNumeric(); + }, + ]; + + yield 'fetchFirstColumn' => [ + static function (ResultCacheStatement $result): void { + $result->fetchFirstColumn(); + }, + ]; + } + public function testFetchAllColumn(): void { $query = $this->connection->getDatabasePlatform() From b085da7d8571e6b9d5f3fff48c14192795d431f0 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sun, 18 Oct 2020 11:55:11 -0700 Subject: [PATCH 11/15] Testing Guidelines --- docs/en/reference/testing.rst | 123 ++++++++++++++++++++++++++++++++++ docs/en/sidebar.rst | 1 + 2 files changed, 124 insertions(+) create mode 100644 docs/en/reference/testing.rst diff --git a/docs/en/reference/testing.rst b/docs/en/reference/testing.rst new file mode 100644 index 00000000000..6e6f402e648 --- /dev/null +++ b/docs/en/reference/testing.rst @@ -0,0 +1,123 @@ +Testing Guidelines +=================== + +To ensure high quality, all components of the Doctrine DBAL library are extensively covered with tests. + +Having the code covered with tests and running all tests against each individual code change helps prevent +breakages of the library logic when its code changes. + +Additionally, when code changes are accompanied by new tests, the tests: + +1. Help understand what problem the given code change is trying to solve. +2. Make sure that the problem being solved needs to be solved in the DBAL. +3. Document the proper usage of the DBAL APIs. + +Requirements +------------ + +1. Each pull request that adds new or changes the existing logic must have tests. + + .. note:: + + Modifications to the keyword lists under the ``Doctrine\DBAL\Platforms\Keywords`` namespace + don't have to be covered with tests. + +2. The test that covers certain logic must fail without this logic implemented. + +Types of Tests +-------------- + +Doctrine DBAL primarily uses unit and integration tests. + +Unit Tests +~~~~~~~~~~ + +Unit tests are meant to cover the logic of a given unit (e.g. a class or a method) including the logic +of its interaction with other units. In this case, the other units could be mocked. + +Unit tests are most welcomed for testing the logic that the DBAL itself defines (e.g. logging, caching, data types). + +In this case, the DBAL is the source of truth about what this logic is and the test plays the role of its description. + +Integration Tests +~~~~~~~~~~~~~~~~~ + +Integration (a.k.a. functional) tests are required when the behavior under test is dictated by the logic +defined outside of the DBAL. It could be: + +- The underlying database platform. +- The underlying database driver. +- SQL syntax and the standard as such. + +It is important to have integration tests for the cases above. Unlike unit tests, they make the external components +the source of truth and help make sure that the logic implemented in the DBAL is correct even if the external components +change (e.g. a new version of a database platform is supported). + +When are Integration Tests not Required? +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Some cases cannot be reproduced with the existing integration testing suite. It could be the scenarios that involve +multiple concurrent database connections, transactions, locking, performance-related issues, etc. + +In such cases, it is still important that a pull request fixing the issues is accompanied by a free-form reproducer +that demonstrates the issue being fixed. + +Recommendations on Writing Tests +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Tests in Doctrine DBAL are located under the ``tests`` directory and implemented on top of PHPUnit. Use its +`documentation `_ to get started. + +Writing Integration Tests +^^^^^^^^^^^^^^^^^^^^^^^^^ + +Integration tests are located under the ``tests/Doctrine/Tests/DBAL/Functional`` directory. Unlike unit tests, +they require a real database connection to test their logic against. + +It is recommended to use ``Doctrine\DBAL\Tests\FunctionalTestCase`` as the base class for integration tests. +Based on the configuration, it will automatically create and connect to the test database. + +Data Fixtures in Integration Tests +++++++++++++++++++++++++++++++++++ + +To test selecting and fetching data from the database, the test may create the necessary schema and populate it +with the test data. To create database tables, instead of checking if the table exists, it is recommended +to use ``AbstractSchemaManager::dropAndCreateTable()``. This way, the table will be dropped and created every time +providing better isolation between the test runs. + +Testing Different Database Platforms +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Although most of the issues are originally discovered on a specific database platform, +the integration tests for all issues should be implemented by default at the database abstraction level +and run against all the platforms that support the API being tested. + +This allows us to ensure that the same scenario that was found failing on one platform also works on others. Or otherwise, +the same issue could be reproduced on the platforms where it wasn't originally tested. + +If the newly added test fails on other platforms, and fixing it is out of the scope, the test can be explicitly marked +as incomplete which will identify the issue. + +Examples of such tests could be found under the ``Doctrine\Tests\DBAL\Functional\Platform`` namespace. + +Using Unit and Integration Tests Together +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +For example, the ``AbstractPlatform::modifyLimitQuery()`` method has both unit and integration tests. + +1. Unit test cases for each platform (``Doctrine\Tests\DBAL\Platforms\*PlatformTest``) have a test that calls + ``$platform->modifyLimitQuery()`` and asserts that the resulting SQL looks as expected. + These tests cannot guarantee that the generated SQL is valid syntactically and semantically but they guarantee + that the code works as designed. They provide fast feedback because they don't require a database connection + and can test all platforms in a single test suite run. +2. There is an integration test ``Doctrine\Tests\DBAL\Functional\ModifyLimitQueryTest`` which calls + ``$platform->modifyLimitQuery()`` and executes the generated queries on a real database to which the test suite + is connected. This test guarantees that the generated queries are valid but it's much slower and works + only with one database at a time. + +As you can see, both approaches have their strengths and weaknesses and can complement each other. + +.. warning:: + + Do not mix the unit and the integration approaches in one test. Each of the approaches has its area of application + and purpose. Mixing them makes it harder to identify the reason and the impact of a failing mixed-type test. diff --git a/docs/en/sidebar.rst b/docs/en/sidebar.rst index 4ada9f1caf1..f0c7ace4c61 100644 --- a/docs/en/sidebar.rst +++ b/docs/en/sidebar.rst @@ -20,5 +20,6 @@ reference/caching reference/known-vendor-issues reference/upgrading + reference/testing explanation/implicit-indexes From b63a652f0d0d34f65b9e5fcd9088ac119ef3ebff Mon Sep 17 00:00:00 2001 From: Kieran Date: Mon, 9 Nov 2020 14:23:45 +0000 Subject: [PATCH 12/15] Added /ci to .gitattributes --- .gitattributes | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitattributes b/.gitattributes index 97a6be4d775..245c7f56ca3 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,4 +1,5 @@ /.appveyor.yml export-ignore +/ci export-ignore /composer.lock export-ignore /docs export-ignore /.doctrine-project.json export-ignore From 1c908c30090794e8b44ab95f4a060e6a69025efe Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Wed, 11 Nov 2020 14:05:20 -0800 Subject: [PATCH 13/15] Mark SQLParserUtils internal --- lib/Doctrine/DBAL/SQLParserUtils.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Doctrine/DBAL/SQLParserUtils.php b/lib/Doctrine/DBAL/SQLParserUtils.php index 5b558f01343..604904dfce2 100644 --- a/lib/Doctrine/DBAL/SQLParserUtils.php +++ b/lib/Doctrine/DBAL/SQLParserUtils.php @@ -26,6 +26,8 @@ /** * Utility class that parses sql statements with regard to types and parameters. + * + * @internal */ class SQLParserUtils { From 3ed11aa8a9e988b398a368ff4a45964693eb59be Mon Sep 17 00:00:00 2001 From: Benjamin Morel Date: Sun, 1 Nov 2020 23:22:31 +0100 Subject: [PATCH 14/15] LockMode::NONE should not set WITH (NOLOCK) This fixes the issue detailed in #4391, with SQL Server and SQL Anywhere setting WITH (NOLOCK) for LockMode::NONE, which effectively means using a READ UNCOMMITTED isolation level at table level, which is not the contract of LockMode::NONE. --- .../DBAL/Platforms/SQLAnywherePlatform.php | 2 +- .../DBAL/Platforms/SQLServerPlatform.php | 2 +- .../DBAL/Functional/LockMode/NoneTest.php | 101 ++++++++++++++++++ .../AbstractSQLServerPlatformTestCase.php | 4 +- .../Platforms/SQLAnywherePlatformTest.php | 2 +- .../Platforms/SQLServer2012PlatformTest.php | 4 +- .../DBAL/Platforms/SQLServerPlatformTest.php | 2 +- 7 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php index f7921cbde48..fd7eafdc71b 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php @@ -52,7 +52,7 @@ public function appendLockHint($fromClause, $lockMode) { switch (true) { case $lockMode === LockMode::NONE: - return $fromClause . ' WITH (NOLOCK)'; + return $fromClause; case $lockMode === LockMode::PESSIMISTIC_READ: return $fromClause . ' WITH (UPDLOCK)'; diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php index b41f33bef46..693517609c4 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php @@ -1574,7 +1574,7 @@ public function appendLockHint($fromClause, $lockMode) { switch (true) { case $lockMode === LockMode::NONE: - return $fromClause . ' WITH (NOLOCK)'; + return $fromClause; case $lockMode === LockMode::PESSIMISTIC_READ: return $fromClause . ' WITH (HOLDLOCK, ROWLOCK)'; diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php new file mode 100644 index 00000000000..e91dd2a824b --- /dev/null +++ b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php @@ -0,0 +1,101 @@ +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. + $db = $this->connection->getDatabase(); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET SINGLE_USER WITH ROLLBACK IMMEDIATE'); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT ON'); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET MULTI_USER'); + } + + $table = new Table('users'); + $table->addColumn('id', 'integer'); + $table->setPrimaryKey(['id']); + + $this->connection->getSchemaManager()->dropAndCreateTable($table); + + $this->connection2 = TestUtil::getConnection(); + + if ($this->connection2->getSchemaManager()->tablesExist('users')) { + return; + } + + if ($this->connection2->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 + { + parent::tearDown(); + + if ($this->connection2->isTransactionActive()) { + $this->connection2->rollBack(); + } + + $this->connection2->close(); + + $this->connection->getSchemaManager()->dropTable('users'); + + if (! $this->connection->getDatabasePlatform() instanceof SQLServerPlatform) { + return; + } + + $db = $this->connection->getDatabase(); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT OFF'); + } + + public function testLockModeNoneDoesNotBreakTransactionIsolation(): void + { + try { + $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->connection->beginTransaction(); + $this->connection2->beginTransaction(); + + $this->connection->insert('users', ['id' => 1]); + + $query = 'SELECT id FROM users'; + $query = $this->connection2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE); + + self::assertFalse($this->connection2->fetchOne($query)); + } +} diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php index 7a63f3ffccf..273e109beb9 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php @@ -374,14 +374,14 @@ public function testModifyLimitQueryWithOrderByClause(): void } $sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' . ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC'; $alteredSql = 'SELECT TOP 15 m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php index 5f3bd50e2ac..119b87bc3f9 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php @@ -266,7 +266,7 @@ public static function getLockHints(): iterable [null, ''], [false, ''], [true, ''], - [LockMode::NONE, ' WITH (NOLOCK)'], + [LockMode::NONE, ''], [LockMode::OPTIMISTIC, ''], [LockMode::PESSIMISTIC_READ, ' WITH (UPDLOCK)'], [LockMode::PESSIMISTIC_WRITE, ' WITH (XLOCK)'], diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php index 1c21098b678..d4f8220e9df 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php @@ -224,14 +224,14 @@ public function testModifyLimitQueryWithExtraLongQuery(): void public function testModifyLimitQueryWithOrderByClause(): void { $sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' . ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC'; $expected = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php index 2357c1a796b..08f2e4a5530 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php @@ -39,7 +39,7 @@ public static function getLockHints(): iterable { return [ [null, ''], - [LockMode::NONE, ' WITH (NOLOCK)'], + [LockMode::NONE, ''], [LockMode::OPTIMISTIC, ''], [LockMode::PESSIMISTIC_READ, ' WITH (HOLDLOCK, ROWLOCK)'], [LockMode::PESSIMISTIC_WRITE, ' WITH (UPDLOCK, ROWLOCK)'], From adce7a954a1c2f14f85e94aed90c8489af204086 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 14 Nov 2020 12:26:58 -0800 Subject: [PATCH 15/15] Release 2.12.1 --- lib/Doctrine/DBAL/Version.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/DBAL/Version.php b/lib/Doctrine/DBAL/Version.php index 85c3b8961cc..9e9f4391270 100644 --- a/lib/Doctrine/DBAL/Version.php +++ b/lib/Doctrine/DBAL/Version.php @@ -17,7 +17,7 @@ class Version /** * Current Doctrine Version. */ - public const VERSION = '2.12.1-DEV'; + public const VERSION = '2.12.1'; /** * Compares a Doctrine version with the current one.