Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
yguedidi opened this issue Jul 1, 2024 · 5 comments

Comments

@yguedidi
Copy link

yguedidi commented Jul 1, 2024

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:

  • use a ResultSetMappingBuilder
  • get a DBAL QueryBuilder with parameters sets with types
  • create an ORM NativeQuery from both
  • set the parameters of the NativeQuery from the parameters of the DBAL QueryBuilder

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
Copy link
Member

greg0ire commented Jul 2, 2024

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:

public function setParameter(string|int $key, mixed $value, ParameterType|ArrayParameterType|string|int|null $type = null): static

That's since #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
Copy link
Author

yguedidi commented Jul 3, 2024

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
Copy link
Member

greg0ire commented Jul 4, 2024

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
Copy link
Member

greg0ire commented Jul 4, 2024

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
Copy link
Author

yguedidi commented Jul 6, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants