Introduce fetch* methods in QueryBuilder for SELECT queries

[andrew-demb]

Feature Request

Q	A
New Feature	yes
RFC	yes
BC Break	no

Summary

Since DBAL deprecates concept of fetch mode in favor usage specific to every fetch mode Connection methods, there are no easy way to avoid deprecated staff with QueryBuilder usage.

As an alternative to deprecated API DBAL may provide new methods in QueryBuilder - fetchAllAssociative() and other, related to new fetch modes.

Implementation for fetch associative method in QueryBuilder may looks like this:

public function fetchAllAssociative(): array
{
    // assert current state contains SELECT query
 
    return $this->connection->fetchAllAssociative($this->getSQL(), $this->params, $this->paramTypes);
}

[morozov]

there are no easy way to avoid deprecated staff with QueryBuilder usage.

Okay, now I see why it was originally brought up in #4280. While the idea of adding methods looks okay, I'm not really a fan of runtime assertions (the need for which in this case is a downside of the QueryBuilder design, not the proposed feature itself).

Apart from the transition from DBAL 2 to DBAL 3, does the addition of these (eight, I believe) new methods solve any other problem than being able to skip the call to execute() and get right away to fetch() or iterate()?

[andrew-demb]

Also it will help to know return types for executed queries.
For now phpdoc @return ResultStatement|int is not clear for code - will it be an int, or ResultStatement (and what items will be received on iterating over ResultStatement).

[PowerKiKi]

Adding those methods would avoid the following PHPStan error:

Cannot call method fetchFirstColumn() on Doctrine\DBAL\Driver\ResultStatement|int.

It was already reported (a while ago) in phpstan/phpstan#687.

For now I workaround by ignoring those error in PHPStan, but I think the proper fix would be a more predictable API. The new methods would allow that.

[morozov]

Adding those methods would avoid the following PHPStan error:

Cannot call method fetchFirstColumn() on Doctrine\DBAL\Driver\ResultStatement|int.

It was already reported (a while ago) in phpstan/phpstan#687.

The PHPStan issue above is logged for the fetchAll() method, not fetchFirstColumn(), so the issue looks irrelevant to the upgrade path.

[PowerKiKi]

With doctrine/dbal 3.0.0 and phpstan/phpstan 0.12.76, latest versions at the time of writing, and the following test code:

<?php

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Query\QueryBuilder;

require 'vendor/autoload.php';

function test(Connection $connection, QueryBuilder $queryBuilder): void
{
    // All of those calls are fine, with a clear and predictable API
    $connection->fetchAllAssociative('SELECT 1');
    $connection->fetchAllAssociativeIndexed('SELECT 1');
    $connection->fetchAllKeyValue('SELECT 1');
    $connection->fetchAllNumeric('SELECT 1');
    $connection->fetchAssociative('SELECT 1');
    $connection->fetchFirstColumn('SELECT 1');
    $connection->fetchNumeric('SELECT 1');
    $connection->fetchOne('SELECT 1');
    $connection->executeStatement('UPDATE table SET value = 1');

    // All of those calls are ambiguous and raise errors in PHPStan,
    // because the API is unpredictable, because `execute()` support
    // both queries and statements and so may return two different and incompatible types
    $queryBuilder->execute()->fetchAllAssociative();
    $queryBuilder->execute()->fetchAllAssociativeIndexed();
    $queryBuilder->execute()->fetchAllKeyValue();
    $queryBuilder->execute()->fetchAllNumeric();
    $queryBuilder->execute()->fetchAssociative();
    $queryBuilder->execute()->fetchFirstColumn();
    $queryBuilder->execute()->fetchNumeric();
    $queryBuilder->execute()->fetchOne();

    // A queryBuilder containing an UPDATE would be fine
    $queryBuilder->execute();
}

We get the following PHPStan errors when running ./vendor/bin/phpstan analyse test.php --level max:

 ------ ------------------------------------------------------------------------------ 
  Line   test.php                                                                      
 ------ ------------------------------------------------------------------------------ 
  24     Cannot call method fetchAllAssociative() on Doctrine\DBAL\Result|int.         
  25     Cannot call method fetchAllAssociativeIndexed() on Doctrine\DBAL\Result|int.  
  26     Cannot call method fetchAllKeyValue() on Doctrine\DBAL\Result|int.            
  27     Cannot call method fetchAllNumeric() on Doctrine\DBAL\Result|int.             
  28     Cannot call method fetchAssociative() on Doctrine\DBAL\Result|int.            
  29     Cannot call method fetchFirstColumn() on Doctrine\DBAL\Result|int.            
  30     Cannot call method fetchNumeric() on Doctrine\DBAL\Result|int.                
  31     Cannot call method fetchOne() on Doctrine\DBAL\Result|int.                    
 ------ ------------------------------------------------------------------------------ 
                                                                                                                        
 [ERROR] Found 8 errors          

So I think @andrew-demb suggestion to introduce new, predictable methods make sense. I would go as far as deprecating then removing execute() altogether, in order to have the final API mirror exactly what exist in Connection, so it would be used like so:

// Exactly same method names as those in `Connection`, but without argument
$queryBuilder->fetchAllAssociative();
$queryBuilder->fetchAllAssociativeIndexed();
$queryBuilder->fetchAllKeyValue();
$queryBuilder->fetchAllNumeric();
$queryBuilder->fetchAssociative();
$queryBuilder->fetchFirstColumn();
$queryBuilder->fetchNumeric();
$queryBuilder->fetchOne();
$queryBuilder->executeStatement();

// Deprecated and then removed entirely
$queryBuilder->execute();

Also I don't think asserting that the queryBuilder contains a query (vs a statement) is strictly necessary. This assertion is not done in Connection, so there might not be a need to check that at a "higher" level either.

[morozov]

So I think @andrew-demb suggestion to introduce new, predictable methods make sense.

Agree.

I would go as far as deprecating then removing execute() altogether, in order to have the final API mirror exactly what exist in Connection […]

Agree.

Also I don't think asserting that the queryBuilder contains a query (vs a statement) is strictly necessary.

It's not necessary if proven by a test: we can do something like fetchOne('UPDATE table SET foo = bar') and make sure it yields an empty result set w/o an error with all drivers (I just checked it with SQLite).

[morozov]

@PowerKiKi, #4489 doesn't contain the deprecation you proposed and is ready to be merged. If you want to get the old method deprecated, please file another PR.

[PowerKiKi]

I'll create a new PR to deprecate execute() as soon as #4489 is merged.

[PowerKiKi]

I just created #4578 as a follow-up to this.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.