doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.86k stars 2.5k forks source link

Sync type of parameters accepted by ORM NativeQuery setParameter and the one returned by DBAL QueryBuilder getParameterTypes #11538

Open yguedidi opened 1 week ago

yguedidi commented 1 week ago

Bug Report

Q A
BC Break no
Version 2.19.5

Summary

For some complex queries, I use the following pattern in a repository:

        $rsm = new ResultSetMappingBuilder($this->getEntityManager(), ResultSetMappingBuilder::COLUMN_RENAMING_INCREMENT);
        $rsm->addRootEntityFromClassMetadata(MyEntity::class, 'fi');

        $qb = $this->getMyDBALQueryBuilder(...);

        $qb->select($rsm->generateSelectClause());

        $query = $this->getEntityManager()->createNativeQuery($qb->getSQL(), $rsm);

        foreach ($qb->getParameters() as $key => $value) {
            $query->setParameter($key, $value, $qb->getParameterTypes()[$key] ?? null);
        }

        return $query->getResult();

it has the benefit to manager all parameters in the DBAL query builder, having it functional if used on it's own, and avoid forgetting to pass a parameter to the ORM native query.

But PHPStan is not OK with that.

Current behavior

I get the following PHPStan error:

Parameter #3 $type of method Doctrine\ORM\AbstractQuery<mixed,mixed>::setParameter() expects int|string|null, Doctrine\DBAL\Types\Type|int|string|null given.

How to reproduce

as shown in the Summary section:

Expected behavior

no PHPStan error, so maybe make the ORM NativeQuery setParameter methos accepts Doctrine\DBAL\Types\Type?

What do you think? Maybe there is a better way?

Note: for now the code works, it's PHPStan that complains

greg0ire commented 6 days ago

Hi mate! Hope you're doing well!

maybe make the ORM NativeQuery setParameter methos accepts Doctrine\DBAL\Types\Type?

That does not seems to be the case on 3.2.x: https://github.com/doctrine/orm/blob/1fe1a6a048dd420d06704f72b296a463237d7603/src/AbstractQuery.php#L333

That's since https://github.com/doctrine/orm/pull/9950 and #10995, and as you can see, the type declaration is widened to include ParameterType and ArrayParameterType, but not Type (which is something else entirely). Are you sure that at runtime, you ever end up with a Type instance being passed to setParameter?

yguedidi commented 5 days ago

Hey hey Grégoire! I'm fine, hope the same for you!

Are you sure that at runtime, you ever end up with a Type instance being passed to setParameter?

I'm almost sure that I never get a Type instance! either string or int through constants. Runtime works fine, but PHPStan complains because types are not compatible, as the returned type by getParameterTypes is wider than the accepted type by setParameter. (we're at PHPStan level 7)

greg0ire commented 4 days ago

I'm doing great, thanks! I think you might have found a DBAL bug, because when I run the entire test suite after dropping Type from the return type declaration of QueryBuilder::getParameterType(), it passes. A next step would be to try using a Type instance with setParameter on the DBAL qb and see what happens.

EDIT: it's not a DBAL bug. There is support for Type, it's just that getParameterType() is not used in this case in the test suite: https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Connection.php#L1276-L1279

greg0ire commented 4 days ago

so maybe make the ORM NativeQuery setParameter methos accepts Doctrine\DBAL\Types\Type

That sounds indeed like the way to go. Is there a reason not to do it at the AbstractQuery level?

yguedidi commented 1 day ago

Is there a reason not to do it at the AbstractQuery level?

Any reason I can think of :) I can try a PR if you want? which branch should it target?