Drop DBAL 2.x support (#963)

* Drop DBAL 2.x support * require DBAL v3.3+ for getNativeConnection() method * HOOK_AFTER_*_QUERY pass affected rows
atk4 · Jan 22, 2022 · 5a0eb69 · 5a0eb69
1 parent e82eebf
commit 5a0eb69
Show file tree

Hide file tree

Showing 17 changed files with 55 additions and 182 deletions.
diff --git a/.github/workflows/test-unit.yml b/.github/workflows/test-unit.yml
@@ -73,14 +73,6 @@ jobs:
           echo "memory_limit = 2G" > /usr/local/etc/php/conf.d/custom-memory-limit.ini
           vendor/bin/phpstan analyse
 
-      - name: Run Static Analysis with DBAL 2.x (only for StaticAnalysis)
-        if: matrix.type == 'StaticAnalysis'
-        run: |
-          composer require "doctrine/dbal:^2.10" --ansi --prefer-dist --no-interaction --no-progress --optimize-autoloader
-          echo '<?php require_once __DIR__ . "/MySqlPlatform.php";' > vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/MySQLPlatform.php
-          echo '<?php require_once __DIR__ . "/PostgreSqlPlatform.php";' > vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/PostgreSQLPlatform.php
-          vendor/bin/phpstan analyse
-
   unit-test:
     name: Unit
     runs-on: ubuntu-latest

diff --git a/composer.json b/composer.json
@@ -36,13 +36,13 @@
     "require": {
         "php": ">=7.4 <8.2",
         "atk4/core": "dev-develop",
-        "doctrine/dbal": "^2.13.5 || ^3.2",
+        "doctrine/dbal": "^3.3",
         "mvorisek/atk4-hintable": "~1.7.1"
     },
     "require-release": {
         "php": ">=7.4 <8.2",
         "atk4/core": "~3.2.0",
-        "doctrine/dbal": "^2.13.5 || ^3.2",
+        "doctrine/dbal": "^3.š",
         "mvorisek/atk4-hintable": "~1.7.1"
     },
     "require-dev": {

diff --git a/docs/hooks.rst b/docs/hooks.rst
@@ -193,13 +193,12 @@ and your update() may not actually update anything. This does not normally
 generate an error, however if you want to actually make sure that update() was
 effective, you can implement this through a hook::
 
-    $m->onHook(Persistence\Sql::HOOK_AFTER_UPDATE_QUERY, function($m, $update, $st) {
-        if (!$st->rowCount()) {
+    $m->onHook(Persistence\Sql::HOOK_AFTER_UPDATE_QUERY, function($m, $update, $c) {
+        if ($c === 0) {
             throw (new \Atk4\Core\Exception('Update didn\'t affect any records'))
                 ->addMoreInfo('query', $update->getDebugQuery())
-                ->addMoreInfo('statement', $st)
-                ->addMoreInfo('model', $m)
-                ->addMoreInfo('conditions', $m->conditions);
+                ->addMoreInfo('statement', $c)
+                ->addMoreInfo('model', $m);
         }
     });
 

diff --git a/docs/sql.rst b/docs/sql.rst
@@ -466,7 +466,7 @@ procedure inside Model::init() then set $table property to a temporary table::
         function init(): void {
             parent::init();
 
-            $q = $this->expr("call get_nominal_sheet([],[],'2014-10-01','2015-09-30',0)", [
+            $res = $this->expr("call get_nominal_sheet([],[],'2014-10-01','2015-09-30',0)", [
                 $this->getApp()->system->getId(),
                 $this->getApp()->system['contractor_id']
             ])->execute();

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -30,13 +30,11 @@ parameters:
             path: '*'
             count: 2
 
-        # for Doctrine DBAL 2.x, remove the support once Doctrine ORM 2.10 is released
-        # see https://github.com/doctrine/orm/issues/8526
+        # fix https://github.com/phpstan/phpstan-deprecation-rules/issues/52 and https://github.com/phpstan/phpstan/issues/6444
         -
-            message: '~^(Call to an undefined method Doctrine\\DBAL\\Driver\\Connection::getWrappedConnection\(\)\.|Call to an undefined method Doctrine\\DBAL\\Connection::createSchemaManager\(\)\.|Call to an undefined static method Doctrine\\DBAL\\Exception::invalidPdoInstance\(\)\.|Call to method (getCreateTableSQL|getClobTypeDeclarationSQL|initializeCommentedDoctrineTypes)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\\w+Platform:\n.+|Anonymous class extends deprecated class Doctrine\\DBAL\\Platforms\\(PostgreSQL94Platform|SQLServer2012Platform):\n.+|Call to deprecated method fetch(|All)\(\) of class Doctrine\\DBAL\\Result:\n.+|Call to deprecated method getSchemaManager\(\) of class Doctrine\\DBAL\\Connection:\n.+|Call to deprecated method getWrappedConnection\(\) of class Doctrine\\DBAL\\Connection:\n.+getNativeConnection\(\).+|Call to deprecated method getWrappedResourceHandle\(\) of class Doctrine\\DBAL\\Driver\\Mysqli\\Connection:\n.+getNativeConnection\(\).+|Access to an undefined property Doctrine\\DBAL\\Driver\\PDO\\Connection::\$connection\.|Parameter #1 \$dsn of class Doctrine\\DBAL\\Driver\\PDO\\SQLSrv\\Connection constructor expects string, Doctrine\\DBAL\\Driver\\PDO\\Connection given\.|Method Atk4\\Data\\Persistence\\Sql\\Expression::execute\(\) should return Doctrine\\DBAL\\Result\|PDOStatement but returns bool\.|PHPDoc tag @return contains generic type Doctrine\\DBAL\\Schema\\AbstractSchemaManager<Doctrine\\DBAL\\Platforms\\AbstractPlatform> but class Doctrine\\DBAL\\Schema\\AbstractSchemaManager is not generic\.|Class Doctrine\\DBAL\\Platforms\\(MySqlPlatform|PostgreSqlPlatform) referenced with incorrect case: Doctrine\\DBAL\\Platforms\\(MySQLPlatform|PostgreSQLPlatform)\.)$~'
+            message: '~^Call to method (getClobTypeDeclarationSQL|getCreateTableSQL)\(\) of deprecated class Doctrine\\DBAL\\Platforms\\(SQLServerPlatform|AbstractPlatform):\nUse.+instead\.$~'
             path: '*'
-            # count for DBAL 3.x matched in "src/Persistence/GenericPlatform.php" file
-            count: 40
+            count: 2
 
         # TODO these rules are generated, this ignores should be fixed in the code
         # for src/Schema/TestCase.php

diff --git a/src/Persistence/GenericPlatform.php b/src/Persistence/GenericPlatform.php
@@ -6,46 +6,11 @@
 
 use Doctrine\DBAL\Exception as DbalException;
 use Doctrine\DBAL\Platforms;
-use Mvorisek\Atk4\Hintable\Phpstan\PhpstanUtil;
 
 class GenericPlatform extends Platforms\AbstractPlatform
 {
     private function createNotSupportedException(): \Exception
     {
-        if (\Atk4\Data\Persistence\Sql\Connection::isComposerDbal2x()) {
-            // hack for PHPStan, keep ignored error count for DBAL 2.x and DBAL 3.x the same
-            if (PhpstanUtil::alwaysFalseAnalyseOnly()) {
-                $connection = (new class() extends Sql\Connection {})->connection();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-                $connection->getSchemaManager();
-            }
-        }
-
         return DbalException::notSupported('SQL');
     }
 

diff --git a/src/Persistence/Sql.php b/src/Persistence/Sql.php
@@ -549,10 +549,9 @@ protected function insertRaw(Model $model, array $dataRaw)
 
         $insert->setMulti($dataRaw);
 
-        $st = null;
         try {
             $model->hook(self::HOOK_BEFORE_INSERT_QUERY, [$insert]);
-            $st = $insert->execute();
+            $c = $insert->execute()->rowCount();
         } catch (SqlException $e) {
             throw (new Exception('Unable to execute insert query', 0, $e))
                 ->addMoreInfo('model', $model)
@@ -568,7 +567,7 @@ protected function insertRaw(Model $model, array $dataRaw)
             $idRaw = '';
         }
 
-        $model->hook(self::HOOK_AFTER_INSERT_QUERY, [$insert, $st]);
+        $model->hook(self::HOOK_AFTER_INSERT_QUERY, [$insert, $c]);
 
         return $idRaw;
     }
@@ -584,9 +583,8 @@ protected function updateRaw(Model $model, $idRaw, array $dataRaw): void
 
         $model->hook(self::HOOK_BEFORE_UPDATE_QUERY, [$update]);
 
-        $st = null;
         try {
-            $st = $update->execute();
+            $c = $update->execute()->rowCount();
         } catch (SqlException $e) {
             throw (new Exception('Unable to update due to query error', 0, $e))
                 ->addMoreInfo('model', $model)
@@ -602,10 +600,10 @@ protected function updateRaw(Model $model, $idRaw, array $dataRaw): void
             }
         }
 
-        $model->hook(self::HOOK_AFTER_UPDATE_QUERY, [$update, $st]);
+        $model->hook(self::HOOK_AFTER_UPDATE_QUERY, [$update, $c]);
 
         // if any rows were updated in database, and we had expressions, reload
-        if ($model->reload_after_save && $st->rowCount()) {
+        if ($model->reload_after_save && $c > 0) {
             $d = $model->getDirtyRef();
             $model->reload();
             \Closure::bind(function () use ($model) {
@@ -624,14 +622,14 @@ protected function deleteRaw(Model $model, $idRaw): void
         $model->hook(self::HOOK_BEFORE_DELETE_QUERY, [$delete]);
 
         try {
-            $st = $delete->execute();
+            $c = $delete->execute()->rowCount();
         } catch (SqlException $e) {
             throw (new Exception('Unable to delete due to query error', 0, $e))
                 ->addMoreInfo('model', $model)
                 ->addMoreInfo('scope', $model->getModel(true)->scope()->toWords());
         }
 
-        $model->hook(self::HOOK_AFTER_DELETE_QUERY, [$delete, $st]);
+        $model->hook(self::HOOK_AFTER_DELETE_QUERY, [$delete, $c]);
     }
 
     public function getFieldSqlExpression(Field $field, Expression $expression): Expression

diff --git a/src/Persistence/Sql/Connection.php b/src/Persistence/Sql/Connection.php
@@ -8,8 +8,6 @@
 use Doctrine\Common\EventManager;
 use Doctrine\DBAL\Connection as DbalConnection;
 use Doctrine\DBAL\Driver\Connection as DbalDriverConnection;
-use Doctrine\DBAL\Driver\Mysqli\Connection as DbalMysqliConnection;
-use Doctrine\DBAL\Driver\OCI8\Connection as DbalOci8Connection;
 use Doctrine\DBAL\DriverManager;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Platforms\OraclePlatform;
@@ -182,7 +180,7 @@ public static function resolveConnectionClass(string $driverName): string
     public static function connect($dsn, $user = null, $password = null, $args = [])
     {
         if ($dsn instanceof DbalConnection) {
-            $driverName = self::getDriverNameFromDbalDriverConnection($dsn->getWrappedConnection());
+            $driverName = self::getDriverNameFromDbalDriverConnection($dsn->getWrappedConnection()); // @phpstan-ignore-line https://github.com/doctrine/dbal/issues/5199
             $connectionClass = self::resolveConnectionClass($driverName);
             $dbalConnection = $dsn;
         } elseif ($dsn instanceof DbalDriverConnection) {
@@ -201,45 +199,9 @@ public static function connect($dsn, $user = null, $password = null, $args = [])
         ], $args));
     }
 
-    final public static function isComposerDbal2x(): bool
-    {
-        return !class_exists(DbalResult::class);
-    }
-
-    /**
-     * @param DbalDriverConnection|DbalConnection $connection
-     *
-     * @return object|resource
-     */
-    private static function getDriverFromDbalDriverConnection(object $connection)
-    {
-        // TODO replace this method with Connection::getNativeConnection() once only DBAL 3.3+ is supported
-        // https://github.com/doctrine/dbal/pull/5037
-
-        if (self::isComposerDbal2x()) {
-            if ($connection instanceof \PDO || $connection instanceof \mysqli) {
-                return $connection;
-            }
-        }
-
-        $isDbal2x = self::isComposerDbal2x(); // remove once https://github.com/phpstan/phpstan/issues/6319 is fixed
-        $wrappedConnection = $connection instanceof DbalMysqliConnection
-            ? $connection->getWrappedResourceHandle()
-            : ($connection instanceof DbalOci8Connection
-                ? \Closure::bind(fn () => $isDbal2x ? $connection->dbh : $connection->connection, null, DbalOci8Connection::class)() // @phpstan-ignore-line
-                : $connection->getWrappedConnection());
-
-        if ($wrappedConnection instanceof \PDO || $wrappedConnection instanceof \mysqli
-            || (is_resource($wrappedConnection) && get_resource_type($wrappedConnection) === 'oci8 connection')) {
-            return $wrappedConnection;
-        }
-
-        return self::getDriverFromDbalDriverConnection($wrappedConnection);
-    }
-
     private static function getDriverNameFromDbalDriverConnection(DbalDriverConnection $connection): string
     {
-        $driver = self::getDriverFromDbalDriverConnection($connection);
+        $driver = $connection->getNativeConnection();
 
         if ($driver instanceof \PDO) {
             return 'pdo_' . $driver->getAttribute(\PDO::ATTR_DRIVER_NAME);
@@ -272,7 +234,7 @@ protected static function connectFromDsn(array $dsn): DbalDriverConnection
             (static::class)::createDbalEventManager()
         );
 
-        return $dbalConnection->getWrappedConnection();
+        return $dbalConnection->getWrappedConnection(); // @phpstan-ignore-line https://github.com/doctrine/dbal/issues/5199
     }
 
     protected static function connectFromDbalDriverConnection(DbalDriverConnection $dbalDriverConnection): DbalConnection
@@ -286,15 +248,15 @@ protected static function connectFromDbalDriverConnection(DbalDriverConnection $
 
         if ($dbalConnection->getDatabasePlatform() instanceof PostgreSQLPlatform) {
             \Closure::bind(function () use ($dbalConnection) {
-                $dbalConnection->platform = new class() extends \Doctrine\DBAL\Platforms\PostgreSQL94Platform {
+                $dbalConnection->platform = new class() extends \Doctrine\DBAL\Platforms\PostgreSQL94Platform { // @phpstan-ignore-line
                     use Postgresql\PlatformTrait;
                 };
             }, null, DbalConnection::class)();
         }
 
         if ($dbalConnection->getDatabasePlatform() instanceof SQLServerPlatform) {
             \Closure::bind(function () use ($dbalConnection) {
-                $dbalConnection->platform = new class() extends \Doctrine\DBAL\Platforms\SQLServer2012Platform {
+                $dbalConnection->platform = new class() extends \Doctrine\DBAL\Platforms\SQLServer2012Platform { // @phpstan-ignore-line
                     use Mssql\PlatformTrait;
                 };
             }, null, DbalConnection::class)();
@@ -350,10 +312,8 @@ public function connection()
 
     /**
      * Execute Expression by using this connection.
-     *
-     * @return DbalResult|\PDOStatement PDOStatement iff for DBAL 2.x
      */
-    public function execute(Expression $expr): object
+    public function execute(Expression $expr): DbalResult
     {
         if ($this->connection === null) {
             throw new Exception('Queries cannot be executed through this connection');

diff --git a/src/Persistence/Sql/Expression.php b/src/Persistence/Sql/Expression.php
@@ -530,10 +530,8 @@ function ($matches) use ($params, &$numParams, &$i, &$j) {
 
     /**
      * @param DbalConnection|Connection $connection
-     *
-     * @return DbalResult|\PDOStatement PDOStatement iff for DBAL 2.x
      */
-    public function execute(object $connection = null): object
+    public function execute(object $connection = null): DbalResult
     {
         if ($connection === null) {
             $connection = $this->connection;
@@ -600,9 +598,6 @@ public function execute(object $connection = null): object
             }
 
             $result = $statement->execute(); // @phpstan-ignore-line
-            if (Connection::isComposerDbal2x()) {
-                return $statement; // @phpstan-ignore-line
-            }
 
             return $result;
         } catch (DbalException $e) {
@@ -653,11 +648,7 @@ public function getRowsIterator(): \Traversable
     {
         // DbalResult::iterateAssociative() is broken with streams with Oracle database
         // https://github.com/doctrine/dbal/issues/5002
-        if (Connection::isComposerDbal2x()) {
-            $iterator = $this->execute();
-        } else {
-            $iterator = $this->execute()->iterateAssociative();
-        }
+        $iterator = $this->execute()->iterateAssociative();
 
         foreach ($iterator as $row) {
             yield array_map(function ($v) {
@@ -675,14 +666,10 @@ public function getRows(): array
     {
         // DbalResult::fetchAllAssociative() is broken with streams with Oracle database
         // https://github.com/doctrine/dbal/issues/5002
-        if (Connection::isComposerDbal2x()) {
-            $result = $this->execute();
-        } else {
-            $result = $this->execute();
-        }
+        $result = $this->execute();
 
         $rows = [];
-        while (($row = Connection::isComposerDbal2x() ? $result->fetchAssociative() : $result->fetch()) !== false) {
+        while (($row = $result->fetchAssociative()) !== false) {
             $rows[] = array_map(function ($v) {
                 return $this->castGetValue($v);
             }, $row);
@@ -698,11 +685,7 @@ public function getRows(): array
      */
     public function getRow(): ?array
     {
-        if (Connection::isComposerDbal2x()) {
-            $row = $this->execute()->fetch();
-        } else {
-            $row = $this->execute()->fetchAssociative();
-        }
+        $row = $this->execute()->fetchAssociative();
 
         if ($row === false) {
             return null;

diff --git a/src/Persistence/Sql/Mssql/PlatformTrait.php b/src/Persistence/Sql/Mssql/PlatformTrait.php
@@ -15,12 +15,14 @@ public function getClobTypeDeclarationSQL(array $column)
         return (str_starts_with($res, 'VARCHAR') ? 'N' : '') . $res;
     }
 
-    protected function initializeCommentedDoctrineTypes()
-    {
-        parent::initializeCommentedDoctrineTypes();
-
-        $this->markDoctrineTypeCommented('text');
-    }
+    // TODO test DBAL DB diff for each supported Field type
+    // then fix using https://github.com/doctrine/dbal/issues/5194#issuecomment-1018790220
+//    protected function initializeCommentedDoctrineTypes()
+//    {
+//        parent::initializeCommentedDoctrineTypes();
+//
+//        $this->markDoctrineTypeCommented('text');
+//    }
 
     // SQL Server DBAL platform has buggy identifier escaping, fix until fixed officially, see:
     // https://github.com/doctrine/dbal/pull/4360

diff --git a/src/Persistence/Sql/Mysql/ExpressionTrait.php b/src/Persistence/Sql/Mysql/ExpressionTrait.php
@@ -4,19 +4,12 @@
 
 namespace Atk4\Data\Persistence\Sql\Mysql;
 
-use Atk4\Data\Persistence\Sql\Connection as SqlConnection;
-
 trait ExpressionTrait
 {
     protected function hasNativeNamedParamSupport(): bool
     {
-        // TODO use Connection::getNativeConnection() once only DBAL 3.3+ is supported
-        // https://github.com/doctrine/dbal/pull/5037
         $dbalConnection = $this->connection->connection();
-        $nativeConnection = \Closure::bind(function () use ($dbalConnection) {
-            return SqlConnection::getDriverFromDbalDriverConnection($dbalConnection->getWrappedConnection());
-        }, null, SqlConnection::class)();
 
-        return !$nativeConnection instanceof \mysqli;
+        return !$dbalConnection->getNativeConnection() instanceof \mysqli;
     }
 }