Improve forward compatibility between 2.x and 3.0 by mdumoulin · Pull Request #4529 · doctrine/dbal

mdumoulin · 2021-03-04T16:48:51Z

Q	A
Type	feature
BC Break	no
Fixed issues	#4280

Summary

The current Doctrine\DBAL\Connection executeQuery(...) and executeCacheQuery() return a ResultStatement interface object. This PR try to improve forward compatibility between v2.x & v3.0 by adding some of missing features to Result returned by Connection :

fetchNumeric()
fetchAssociative()
fetchOne()
fetchAllNumeric()
fetchAllAssociative()
fetchAllKeyValue()
fetchAllAssociativeIndexed()
fetchFirstColumn()
iterateNumeric()
iterateAssociative()
iterateKeyValue()
iterateAssociativeIndexed()
iterateColumn()
rowCount()
columnCount()
free()

I used this version as reference : https://github.com/doctrine/dbal/blob/3.0.x/src/Result.php

Examples

Static analysis error

Here under, methods which throw a static analysis error in phpstan but actually work. This is all methods defined in Abstraction\Result interface :

/** @var Doctrine\DBAL\Connection $connection */
$connection->executeQuery('SELECT "something"')->fetchNumeric();
$connection->executeQuery('SELECT "something"')->fetchAssociative();
$connection->executeQuery('SELECT "something"')->fetchOne();
$connection->executeQuery('SELECT "something"')->fetchAllNumeric();
$connection->executeQuery('SELECT "something"')->fetchAllAssociative();
$connection->executeQuery('SELECT "something"')->fetchFirstColumn();
$connection->executeQuery('SELECT "something"')->iterateNumeric();
$connection->executeQuery('SELECT "something"')->iterateAssociative();
$connection->executeQuery('SELECT "something"')->iterateNumeric();
$connection->executeQuery('SELECT "something"')->iterateColumn();

The outpout pattern when running phpstan is the following :

Call to an undefined method Doctrine\DBAL\Driver\ResultStatement::fetchOne().

Not available methods

Here under, methods which throw undefined method fatal & static analyse errors. This is all methods added in Abstraction\V3Result interface :

$connection->executeQuery('SELECT "something"')->fetchAllKeyValue();
$connection->executeQuery('SELECT "something"')->fetchAllAssociativeIndexed();
$connection->executeQuery('SELECT "something"')->iterateKeyValue();
$connection->executeQuery('SELECT "something"')->iterateAssociativeIndexed();

The output pattern when trying to use one of those methods :

Fatal error: Uncaught Error: Call to undefined method Doctrine\DBAL\Driver\PDO\Statement::fetchAllKeyValue() in /var/www/test.php on line 41

Error: Call to undefined method Doctrine\DBAL\Driver\PDO\Statement::fetchAllKeyValue() in /var/www/test.php on line 41

Call Stack:
    0.0006     415576   1. {main}() /var/www/test.php:0

Implementation key points

The strategy is to :

Add a new interfaces with missing V3 Result features
Make Connection return an object implementing ResultStatement & V3Result interfaces

lib/Doctrine/DBAL/Cache/ArrayStatement.php

beberlei

This is very important, however I think we should be bolder and move the V3Result to be Doctrine\DBAL\Result otherwise the API will change again in 3.0 and you cannot rely on typing the result tpye, example:

use Doctrine\DBAL\Result;

public function performQuery()
{
     $result = $this->connection->executeQuery('SELECT * FROM orders');
     return $this->convertQueryResult($result);
}

private function convertQueryResult(Result $result)
{
}

lib/Doctrine/DBAL/Abstraction/V3Result.php

lib/Doctrine/DBAL/Connection.php

lib/Doctrine/DBAL/Abstraction/V3Result.php

lib/Doctrine/DBAL/Connection.php

lib/Doctrine/DBAL/Driver/V3CompatStatement.php

lib/Doctrine/DBAL/Connection.php

lib/Doctrine/DBAL/Driver/FetchUtils.php

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

lib/Doctrine/DBAL/Cache/ArrayStatement.php

lib/Doctrine/DBAL/Cache/ResultCacheStatement.php

mdumoulin · 2021-03-10T16:11:39Z

@morozov Drivers statements no longer implements wrapper level Result. It break some unit tests because of ResultStatement wrapper. The following code will no longer work for example.

$statement = $connection->executeQuery('SELECT "something"');

if ($statement instanceof ArrayStatement) {
    // True in 2.12.x version, false in this branch
}

I can't see any generic solution for this kind of edge case. Maybe we should let the user enable the Statement wrapper on purpose. We will avoid the risk of regression and allow to resolve deprecation on 2.x. It can be implemented by adding a ForwardCompatibility\Connection wrapper for example.

morozov · 2021-03-11T01:43:01Z

Maybe we should let the user enable the Statement wrapper on purpose. We will avoid the risk of regression and allow to resolve deprecation on 2.x. It can be implemented by adding a ForwardCompatibility\Connection wrapper for example.

If it's an opt-in layer, then there will be no improvement for static analysis, because the analyzers won't be able to detect the opt-in. As for the examples that currently don't work, e.g.:

$connection->executeQuery('SELECT "something"')->fetchNumeric();

There's already an alternative:

$connection->fetchNumeric('SELECT "something"');

beberlei · 2021-03-11T06:10:07Z

Yes this edge case break is ok imho, the api is returning a statement, expecting a specific statement is not part of the interface

morozov

Looks good.

morozov · 2021-03-13T18:36:07Z

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

    {
        yield 'fetchAll' => [
-            static function (ResultCacheStatement $statement): void {
+            static function (ForwardCompatibility\Result $statement): void {


The expected ResultCacheStatement type was unnecessarily specific. Let's loosen it to Driver\ResultStatement. If it had been implemented this way, it wouldn't require a change now.

morozov · 2021-03-15T16:44:40Z

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


        yield 'fetchFirstColumn' => [
-            static function (ResultCacheStatement $result): void {
+            static function (ForwardCompatibility\Result $result): void {


#4529 (comment) applies to all the above cases.

Not sure to understand here. ResultStament interface does not contain the fetchAllAssociative(), fetchAllNumeric() & fetchFirstColumn() methods. I can use the Doctrine\DBAL\Result interface, if we prefer not relying on concrete object.

Sorry, it should have been Driver\Result.

Alright, so the expected type was not unnecessarily specific: it was the type that implemented both of the old and new APIs. It still implements them but now the fetch*() methods return another class that implements both interfaces as well. Looks good 👍

morozov · 2021-03-16T19:58:59Z

@mdumoulin It looks good. I'd like all the changes to be squashed in one commit. Please do or otherwise, I'll do it later before the merge.

mdumoulin · 2021-03-17T10:52:05Z

@morozov Great ! I just squashed all my commits in a single one.

Make Doctrine\DBAL\Connection executeQuery() & executeCachedQuery() return a result implementing the following v3.0 features : - fetchNumeric() - fetchAssociative() - fetchOne() - fetchAllNumeric() - fetchAllAssociative() - fetchAllKeyValue() - fetchAllAssociativeIndexed() - fetchFirstColumn() - iterateNumeric() - iterateAssociative() - iterateKeyValue() - iterateAssociativeIndexed() - iterateColumn() - rowCount() - columnCount() - free()

morozov · 2021-03-17T17:00:11Z

Thanks, @mdumoulin!

mdumoulin · 2021-03-18T09:06:52Z

@morozov @beberlei Thanks again for your quick feedback & review.

, doctrine#4571)

…ompatibility-result Add missing runtime deprecations related to FC Result API (#4529, #4571)

morozov reviewed Mar 4, 2021

View reviewed changes

lib/Doctrine/DBAL/Cache/ArrayStatement.php Outdated Show resolved Hide resolved

mdumoulin force-pushed the improve-v2-forward-compatibility branch 2 times, most recently from 3173be4 to d1b7d7d Compare March 5, 2021 15:40

morozov reviewed Mar 5, 2021

View reviewed changes

lib/Doctrine/DBAL/Cache/ArrayStatement.php Outdated Show resolved Hide resolved

beberlei reviewed Mar 7, 2021

View reviewed changes

lib/Doctrine/DBAL/Abstraction/V3Result.php Outdated Show resolved Hide resolved

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

beberlei mentioned this pull request Mar 7, 2021

Deprecations #4535

Merged

morozov reviewed Mar 7, 2021

View reviewed changes

beberlei added this to the 2.13.0 milestone Mar 8, 2021

beberlei changed the base branch from 2.12.x to 2.13.x March 8, 2021 08:51

mdumoulin force-pushed the improve-v2-forward-compatibility branch 5 times, most recently from 3a45490 to 42249a0 Compare March 8, 2021 15:24

morozov reviewed Mar 9, 2021

View reviewed changes

lib/Doctrine/DBAL/Cache/ArrayStatement.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Cache/ArrayStatement.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Cache/ResultCacheStatement.php Outdated Show resolved Hide resolved

mdumoulin force-pushed the improve-v2-forward-compatibility branch from ea422e9 to 9868993 Compare March 13, 2021 13:42

morozov reviewed Mar 13, 2021

View reviewed changes

morozov reviewed Mar 15, 2021

View reviewed changes

mdumoulin force-pushed the improve-v2-forward-compatibility branch from ef2eb93 to 0beb8c2 Compare March 17, 2021 10:48

morozov force-pushed the improve-v2-forward-compatibility branch from 0beb8c2 to 4297f50 Compare March 17, 2021 15:11

morozov approved these changes Mar 17, 2021

View reviewed changes

morozov added Connections Improvement Prepared Statements labels Mar 17, 2021

morozov merged commit b49127c into doctrine:2.13.x Mar 17, 2021

beberlei mentioned this pull request Mar 27, 2021

Deprecate Statement::fetch* methods in favor of Result::fetch* counterparts #4554

Closed

github-actions bot mentioned this pull request Mar 30, 2021

Merge release 2.13.0 into 3.0.x #4563

Merged

This was referenced Mar 30, 2021

Authentication request failed contao/contao#2928

Closed

Add conflict for doctrine/dbal 2.13.0 contao/conflicts#24

Merged

beberlei added a commit to beberlei/dbal that referenced this pull request Apr 2, 2021

Add missing runtime deprecations related to FC Result API (doctrine#4529

b63901b

, doctrine#4571)

beberlei mentioned this pull request Apr 2, 2021

Add missing runtime deprecations related to FC Result API (#4529, #4571) #4579

Merged

beberlei added a commit that referenced this pull request Apr 4, 2021

Merge pull request #4579 from beberlei/runtime-deprecations-forward-c…

79a6a84

…ompatibility-result Add missing runtime deprecations related to FC Result API (#4529, #4571)

Jean85 mentioned this pull request Jun 1, 2021

Rework DBAL compat layer getsentry/sentry-symfony#512

Closed

driehle mentioned this pull request Nov 4, 2021

Added support for DBAL v3 doctrine/DoctrineORMModule#675

Merged

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

Uh oh!

Conversation

mdumoulin commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Examples

Static analysis error

Not available methods

Implementation key points

Uh oh!

Uh oh!

Uh oh!

beberlei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdumoulin commented Mar 10, 2021

Uh oh!

morozov commented Mar 11, 2021

Uh oh!

beberlei commented Mar 11, 2021

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

morozov Mar 13, 2021

Choose a reason for hiding this comment

Uh oh!

morozov Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

mdumoulin Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

morozov Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

morozov Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

morozov commented Mar 16, 2021

Uh oh!

mdumoulin commented Mar 17, 2021

Uh oh!

morozov commented Mar 17, 2021

Uh oh!

mdumoulin commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdumoulin commented Mar 4, 2021 •

edited

Loading