Dbal: Static analysis issue in 2.11

[enumag]

Bug Report

| Q | A
|------------ | ------
| BC Break | yes
| Version | 2.11.0

Summary

ResultStatement::fetchAll() is deprecated with message "Use fetchAllNumeric(), fetchAllAssociative() or fetchFirstColumn() instead."

The problem is that these new methods are not on ResultStatement interface and the @return annotation on Connection::executeQuery() was not changed which causes PHPStan to report an issue.

How to reproduce

When I change this code:

        $result = $connection->executeQuery($sql)->fetchAll();

according to the deprecation notice:

        $result = $connection->executeQuery($sql)->fetchAllAssociative();

PHPStan then reports:

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

Expected behaviour

No PHPStan issues - executeQuery should have @return Result.

[greg0ire]

Useful links:

https://github.com/doctrine/dbal/blob/5d2282389438138b91b31742f7b7f4779e1a2122/lib/Doctrine/DBAL/Connection.php#L1130-L1134

https://github.com/doctrine/dbal/blob/5d2282389438138b91b31742f7b7f4779e1a2122/lib/Doctrine/DBAL/Driver/ResultStatement.php#L76-L97

https://github.com/doctrine/dbal/blob/2.11.x/lib/Doctrine/DBAL/Driver/Result.php

executeQuery should have @return Result.

Maybe it should have @return Result&ResultStatement?

[enumag]

Or possibly @return Statement? (Doctrine\DBAL\Statement)

[greg0ire]

Why? Doctrine\DBAL\Statement does not implement Doctrine\DBAL\Driver\ResultStatement, does it?

[enumag]

Ah sorry, I meant Doctrine\DBAL\Driver\Statement. But then again that doesn't implement Result either. These statements and result classes/interfaces are quite confusing to be honest.

[morozov]

It's not reported by PHPStan on the library itself because during the rework I replaced all code blocks like $connection->executeQuery($sql)->fetchAllAssociative() with $connection->fetchAllAssociative($sql).

While this is being fixed, you can do the same.

[morozov]

These statements and result classes/interfaces are quite confusing to be honest.

This is because it's a forward-compatibility layer which has to be available to the users on the one hand but shouldn't change the API on the other. It will be all straightforward in 3.0.

[morozov]

Maybe it should have @return Result&ResultStatement?

That would work in this case but it invalidates other existing logic, e.g.:

https://github.com/doctrine/dbal/blob/ad1e48238b38a8ec10a7f461e7426c7f3985b603/lib/Doctrine/DBAL/Connection.php#L611-L615

It's not guaranteed that an overridden implementation of the method in extending classes will also return a Result (which basically is what PHPStan reports in the client code).

$connection->fetchAllAssociative($sql) looks the most straightforward way to avoid static analysis issues. Alternatively, we may implement @return Result&ResultStatement but it will require suppressing a lot of new static analysis errors on the library side (both in PHPStan and Psalm).

@enumag please let us know if $connection->fetchAllAssociative($sql) will work for you.

[enumag]

@morozov Thanks! I didn't know about $connection->fetchAllAssociative($sql). I believe it will work just fine for me. I'll leave it up to you whether you want to close this issue or not.

[morozov]

I will leave it open in case if somebody has an idea how to fix the problem which does exist but I'm not planning to work on a solution since it's not obvious.

[rainbow-alex]

Phpstan might be right to report this, if you create a doctrine connection from an existing pdo instance, you will get a \PDOStatement (not \Doctrine\DBAL\Driver\PDOStatement) which does not implement the new fetch methods.

[andrew-demb]

Hi.

Suggestion about $connection->fetchAllAssociative() will work fine for cases, when connection used to run queries.
But if it is usage from query builder, there is no simple alternative (except manual expanding sql, parameters, types to call connection method with them), only execute(): int|ResultStatement.
https://github.com/doctrine/dbal/blob/5d2282389438138b91b31742f7b7f4779e1a2122/lib/Doctrine/DBAL/Query/QueryBuilder.php#L204

I don't have any suggestion for fixing problem yet :(

[Firehed]

@andrew-demb - When I was trying to work around this, adding assert($stmt instanceof \Doctrine\DBAL\Driver\Result); made PHPStan happy, presumably by creating the union type suggested here.

I ended up also converting to a different method as it worked for my use-case, but I believe assert should work pretty much everywhere.