Improve forward compatibility between 2.x and 3.0

[mdumoulin]

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

[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)
{
}

[mdumoulin]

@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]

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]

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]

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]

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

[mdumoulin]

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.

[morozov]

Sorry, it should have been Driver\Result.

[morozov]

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]

@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]

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

[morozov]

Thanks, @mdumoulin!

[mdumoulin]

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