Merge branch '2.13.x'

* 2.13.x: Bump doctrine/coding-standard [GH-4643] Fix SQLServerPlatform::quoteIdentifier() [GH-4645] Use error suppression instead of an error handler in MySQLi Connection Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality Add type-inference tests to github actions, so that we test also that types match up Add warnings in docblocks Rework the caching documentation Fix #4637 by duplicating the type definition for `DriverManager::getConnection($args)` params Signed-off-by: Alexander M. Turek <me@derrabus.de>
doctrine · Jun 7, 2021 · e6add29 · e6add29
2 parents 7ef2a33 + 2f286f4
commit e6add29
Show file tree

Hide file tree

Showing 18 changed files with 189 additions and 65 deletions.
diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml
@@ -53,3 +53,9 @@ jobs:
         with:
           composer_require_dev: true
           args: --shepherd
+
+      - name: Psalm type inference tests
+        uses: docker://vimeo/psalm-github-actions:4.6.4
+        with:
+          composer_require_dev: true
+          args: --config=psalm-strict.xml
diff --git a/composer.json b/composer.json
@@ -38,7 +38,7 @@
         "doctrine/event-manager": "^1.0"
     },
     "require-dev": {
-        "doctrine/coding-standard": "8.2.0",
+        "doctrine/coding-standard": "9.0.0",
         "jetbrains/phpstorm-stubs": "2020.2",
         "phpstan/phpstan": "0.12.81",
         "phpstan/phpstan-strict-rules": "^0.12.2",

diff --git a/docs/en/reference/caching.rst b/docs/en/reference/caching.rst
@@ -1,10 +1,22 @@
 Caching
 =======
 
-A ``Doctrine\DBAL\Statement`` can automatically cache result sets.
+A ``Doctrine\DBAL\Statement`` can automatically cache result sets. The
+feature is optional though, and by default, no result set is cached.
 
-For this to work an instance of ``Doctrine\Common\Cache\Cache`` must be provided.
-This can be set on the configuration object (optionally it can also be passed at query time):
+To use the result cache, there are three mandatory steps:
+
+1. Configure a global result cache, or provide one at query time.
+2. Provide a cache profile for the result set you want to cache when
+   making a query.
+3. Read the entire result set from the database.
+
+Configuring the result cache
+----------------------------
+
+Any instance of ``Doctrine\Common\Cache\Cache`` can be used as a result
+cache and can be set on the configuration object (optionally it can also
+be passed at query time):
 
 ::
 
@@ -13,29 +25,40 @@ This can be set on the configuration object (optionally it can also be passed at
     $config = $conn->getConfiguration();
     $config->setResultCacheImpl($cache);
 
-To get the result set of a query cached it is necessary to pass a
-``Doctrine\DBAL\Cache\QueryCacheProfile`` instance to the ``executeQuery`` or ``executeCacheQuery``
-instance. The difference between these two methods is that the former does not
-require this instance, while the later has this instance as a required parameter:
+Providing a cache profile
+-------------------------
+
+To get the result set of a query cached, it is necessary to pass a
+``Doctrine\DBAL\Cache\QueryCacheProfile`` instance to the
+``executeQuery()`` or ``executeCacheQuery()`` methods. The difference
+between these two methods is that the former has the cache profile as an
+optional argument, whereas it is required when calling the latter:
 
 ::
 
     <?php
     $stmt = $conn->executeQuery($query, $params, $types, new QueryCacheProfile(0, "some key"));
     $stmt = $conn->executeCacheQuery($query, $params, $types, new QueryCacheProfile(0, "some key"));
 
-It is also possible to pass in a ``Doctrine\Common\Cache\Cache`` instance into the
-constructor of ``Doctrine\DBAL\Cache\QueryCacheProfile`` in which case it overrides
-the default cache instance:
+As stated before, it is also possible to pass in a
+``Doctrine\Common\Cache\Cache`` instance into the constructor of
+``Doctrine\DBAL\Cache\QueryCacheProfile`` in which case it overrides the
+default cache instance:
 
 ::
 
     <?php
     $cache = new \Doctrine\Common\Cache\FilesystemCache(__DIR__);
     new QueryCacheProfile(0, "some key", $cache);
 
-In order for the data to actually be cached its necessary to ensure that the entire
-result set is read (the easiest way to ensure this is to use one of the ``fetchAll*()`` methods):
+Reading the entire result set
+-----------------------------
+
+Caching half a result set would cause bugs if a subsequent caller needed
+more rows from that same result sets. To be able to cache the entire
+result set, it must be fetched entirely from the database, and not all
+APIs do that. The easiest way to ensure that is to use one of the
+``fetchAll*()`` methods:
 
 ::
 
@@ -45,4 +68,6 @@ result set is read (the easiest way to ensure this is to use one of the ``fetchA
 
 .. warning::
 
-    When using the cache layer not all fetch modes are supported. See the code of the ``Doctrine\DBAL\Cache\CachingResult`` for details.
+    When using the cache layer not all fetch modes are supported. See
+    the code of the ``Doctrine\DBAL\Cache\CachingResult`` for
+    details.
diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -76,10 +76,6 @@
         <exclude-pattern>src/Events.php</exclude-pattern>
     </rule>
 
-    <rule ref="SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedProperty">
-        <exclude-pattern>tests/Tools/TestAsset/*</exclude-pattern>
-    </rule>
-
     <!-- see https://github.com/squizlabs/PHP_CodeSniffer/issues/2099 -->
     <rule ref="Squiz.Commenting.FunctionComment.InvalidNoReturn">
         <exclude-pattern>src/Platforms/AbstractPlatform.php</exclude-pattern>
@@ -122,11 +118,6 @@
         <exclude-pattern>tests/Functional/ResultCacheTest.php</exclude-pattern>
     </rule>
 
-    <rule ref="SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedMethod">
-        <exclude-pattern>*/lib/Doctrine/DBAL/Driver/PDOConnection.php</exclude-pattern>
-        <exclude-pattern>*/lib/Doctrine/DBAL/Driver/PDOStatement.php</exclude-pattern>
-    </rule>
-
     <!-- See https://github.com/slevomat/coding-standard/issues/770 -->
     <rule ref="SlevomatCodingStandard.Namespaces.UnusedUses">
         <exclude-pattern>src/Driver/ExceptionConverterDriver.php</exclude-pattern>

diff --git a/psalm-strict.xml b/psalm-strict.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<psalm
+    totallyTyped="true"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns="https://getpsalm.org/schema/config"
+    xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+>
+    <projectFiles>
+        <directory name="static-analysis" />
+        <ignoreFiles>
+            <directory name="src" />
+            <directory name="vendor" />
+        </ignoreFiles>
+    </projectFiles>
+</psalm>
diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -7,6 +7,7 @@
 >
     <projectFiles>
         <directory name="src" />
+        <directory name="static-analysis" />
         <directory name="tests" />
         <ignoreFiles>
             <directory name="vendor" />

diff --git a/src/Connection.php b/src/Connection.php
@@ -158,11 +158,10 @@ class Connection
      * @param Driver              $driver       The driver to use.
      * @param Configuration|null  $config       The configuration, optional.
      * @param EventManager|null   $eventManager The event manager, optional.
+     * @psalm-param Params $params
+     * @phpstan-param array<string,mixed> $params
      *
      * @throws Exception
-     *
-     * @phpstan-param array<string,mixed> $params
-     * @psalm-param Params $params
      */
     public function __construct(
         array $params,
@@ -204,9 +203,8 @@ public function __construct(
      * @internal
      *
      * @return array<string,mixed>
-     *
-     * @phpstan-return array<string,mixed>
      * @psalm-return Params
+     * @phpstan-return array<string,mixed>
      */
     public function getParams()
     {

diff --git a/src/Connections/PrimaryReadReplicaConnection.php b/src/Connections/PrimaryReadReplicaConnection.php
@@ -8,6 +8,7 @@
 use Doctrine\DBAL\Driver;
 use Doctrine\DBAL\Driver\Connection as DriverConnection;
 use Doctrine\DBAL\Driver\Exception as DriverException;
+use Doctrine\DBAL\DriverManager;
 use Doctrine\DBAL\Event\ConnectionEventArgs;
 use Doctrine\DBAL\Events;
 use Doctrine\DBAL\Exception;
@@ -56,7 +57,7 @@
  *
  * Instantiation through the DriverManager looks like:
  *
- * @psalm-import-type Params from \Doctrine\DBAL\DriverManager
+ * @psalm-import-type Params from DriverManager
  * @example
  *
  * $conn = DriverManager::getConnection(array(
@@ -95,12 +96,11 @@ class PrimaryReadReplicaConnection extends Connection
      * @internal The connection can be only instantiated by the driver manager.
      *
      * @param array<string,mixed> $params
+     * @psalm-param Params $params
+     * @phpstan-param array<string,mixed> $params
      *
      * @throws Exception
      * @throws InvalidArgumentException
-     *
-     * @phpstan-param array<string,mixed> $params
-     * @psalm-param Params $params
      */
     public function __construct(
         array $params,

diff --git a/src/DriverManager.php b/src/DriverManager.php
@@ -145,12 +145,36 @@ private function __construct()
      * @param array<string,mixed> $params
      * @param Configuration|null  $config       The configuration to use.
      * @param EventManager|null   $eventManager The event manager to use.
+     * @psalm-param array{
+     *     charset?: string,
+     *     dbname?: string,
+     *     default_dbname?: string,
+     *     driver?: key-of<self::DRIVER_MAP>,
+     *     driverClass?: class-string<Driver>,
+     *     driverOptions?: array<mixed>,
+     *     host?: string,
+     *     keepSlave?: bool,
+     *     keepReplica?: bool,
+     *     master?: OverrideParams,
+     *     memory?: bool,
+     *     password?: string,
+     *     path?: string,
+     *     pdo?: \PDO,
+     *     platform?: Platforms\AbstractPlatform,
+     *     port?: int,
+     *     primary?: OverrideParams,
+     *     replica?: array<OverrideParams>,
+     *     sharding?: array<string,mixed>,
+     *     slaves?: array<OverrideParams>,
+     *     user?: string,
+     *     wrapperClass?: class-string<T>,
+     * } $params
+     * @phpstan-param array<string,mixed> $params
+     *
+     * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection)
      *
      * @throws Exception
      *
-     * @phpstan-param array<string,mixed> $params
-     * @psalm-param Params $params
-     * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection)
      * @template T of Connection
      */
     public static function getConnection(
@@ -211,11 +235,10 @@ public static function getAvailableDrivers(): array
 
     /**
      * @param array<string,mixed> $params
+     * @psalm-param Params $params
+     * @phpstan-param array<string,mixed> $params
      *
      * @throws Exception
-     *
-     * @phpstan-param array<string,mixed> $params
-     * @psalm-param Params $params
      */
     private static function createDriver(array $params): Driver
     {
@@ -258,16 +281,15 @@ private static function normalizeDatabaseUrlPath(string $urlPath): string
      * updated list of parameters.
      *
      * @param mixed[] $params The list of parameters.
+     * @psalm-param Params $params
+     * @phpstan-param array<string,mixed> $params
      *
      * @return mixed[] A modified list of parameters with info from a database
      *                 URL extracted into indidivual parameter parts.
+     * @psalm-return Params
+     * @phpstan-return array<string,mixed>
      *
      * @throws Exception
-     *
-     * @phpstan-param array<string,mixed> $params
-     * @phpstan-return array<string,mixed>
-     * @psalm-param Params $params
-     * @psalm-return Params
      */
     private static function parseDatabaseUrl(array $params): array
     {

diff --git a/src/Platforms/AbstractPlatform.php b/src/Platforms/AbstractPlatform.php
@@ -3529,10 +3529,9 @@ protected function createReservedKeywordsList(): KeywordList
      * @deprecated Implement {@link createReservedKeywordsList()} instead.
      *
      * @return string
+     * @psalm-return class-string<KeywordList>
      *
      * @throws Exception If not supported on this platform.
-     *
-     * @psalm-return class-string<KeywordList>
      */
     protected function getReservedKeywordsClass()
     {

diff --git a/src/Platforms/SQLServer2012Platform.php b/src/Platforms/SQLServer2012Platform.php
@@ -1584,7 +1584,7 @@ protected function getReservedKeywordsClass()
      */
     public function quoteSingleIdentifier($str)
     {
-        return '[' . str_replace(']', '][', $str) . ']';
+        return '[' . str_replace(']', ']]', $str) . ']';
     }
 
     /**

diff --git a/static-analysis/driver-manager-retrieves-correct-connection-type.php b/static-analysis/driver-manager-retrieves-correct-connection-type.php
@@ -0,0 +1,19 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\StaticAnalysis\DBAL;
+
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\DriverManager;
+
+final class MyConnection extends Connection
+{
+}
+
+function makeMeACustomConnection(): MyConnection
+{
+    return DriverManager::getConnection([
+        'wrapperClass' => MyConnection::class,
+    ]);
+}
diff --git a/tests/DriverManagerTest.php b/tests/DriverManagerTest.php
@@ -13,6 +13,8 @@
 use PHPUnit\Framework\TestCase;
 use stdClass;
 
+use function array_intersect_key;
+use function array_merge;
 use function get_class;
 use function in_array;
 use function is_array;
@@ -125,18 +127,26 @@ public function testDatabaseUrlPrimaryReplica(): void
             'password' => 'bar',
             'host'     => 'localhost',
             'port'     => 11211,
+            'dbname'   => 'baz',
+            'driver'   => 'pdo_mysql',
+            'url'      => 'mysql://foo:bar@localhost:11211/baz',
         ];
 
-        foreach ($expected as $key => $value) {
-            self::assertArrayHasKey($key, $params['primary']);
-            self::assertEquals($value, $params['primary'][$key]);
-
-            self::assertArrayHasKey($key, $params['replica']['replica1']);
-            self::assertEquals($value, $params['replica']['replica1'][$key]);
-        }
-
-        self::assertEquals('baz', $params['primary']['dbname']);
-        self::assertEquals('baz_replica', $params['replica']['replica1']['dbname']);
+        self::assertEquals(
+            [
+                'primary' => $expected,
+                'replica' => [
+                    'replica1' => array_merge(
+                        $expected,
+                        [
+                            'dbname' => 'baz_replica',
+                            'url'    => 'mysql://foo:bar@localhost:11211/baz_replica',
+                        ]
+                    ),
+                ],
+            ],
+            array_intersect_key($params, ['primary' => null, 'replica' => null])
+        );
     }
 
     /**

diff --git a/tests/Functional/ExceptionTest.php b/tests/Functional/ExceptionTest.php
@@ -353,10 +353,9 @@ public function testConnectionExceptionSqLite(): void
 
     /**
      * @param array<string, mixed> $params
+     * @psalm-param Params $params
      *
      * @dataProvider getConnectionParams
-     *
-     * @psalm-param Params $params
      */
     public function testConnectionException(array $params): void
     {