SQLFilter::addFilterConstraint and psalm template constraints

[soullivaneuh]

Bug Report

Q	A
BC Break	yes (phpstan)
Version	2.10.1

Summary

Since the upgrade of doctrine/orm from 2.8.4 to 2.10.1, we have the following PHPStan error:

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Doctrine/ORM/Query/Filter/OwnerFilter.php                                                                                                                                                              
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  36     Method App\Doctrine\ORM\Query\Filter\OwnerFilter::addFilterConstraint() has parameter $targetEntity with generic class Doctrine\ORM\Mapping\ClassMetadata but does not specify its types: T            
         💡 You can turn this off by setting checkGenericClassInNonGenericObjectType: false in your phpstan.neon.                                                                                                

Here is the related class:

<?php

declare(strict_types=1);

namespace App\Doctrine\ORM\Query\Filter;

use App\Annotation\OwnerAware;
use App\Entity\Organization;
use App\Entity\OrganizationMember;
use App\Entity\User;
use Doctrine\Common\Annotations\Reader;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\MappingException;
use Doctrine\ORM\Query\Filter\SQLFilter;

final class OwnerFilter extends SQLFilter
{
    private Reader $annotationReader;

    private EntityManagerInterface $entityManager;

    public function setAnnotationReader(Reader $annotationReader): void
    {
        $this->annotationReader = $annotationReader;
    }

    public function setEntityManager(EntityManagerInterface $entityManager): void
    {
        $this->entityManager = $entityManager;
    }

    /**
     * {@inheritdoc}
     */
    public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
    {
        try {
            $userId = $this->getParameter('user_id');

            // We must ignore if user id is not provided. Useful for queries from CLI commands.
            if ($userId === '') {
                return '';
            }
        } catch (\InvalidArgumentException $exception) {
            return '';
        }

        return $this->getOwnerAssociationConstraint($targetEntity, $targetTableAlias, $userId);
    }
}

We try to add the correct templated parameter:

diff --git a/src/Doctrine/ORM/Query/Filter/OwnerFilter.php b/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
index 49a0fb790..67311bdd9 100644
--- a/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
+++ b/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
@@ -32,6 +32,9 @@ final class OwnerFilter extends SQLFilter
 
     /**
      * {@inheritdoc}
+     *
+     * @psalm-template T of ClassMetadata
+     * @psalm-param ClassMetadata<T> $targetEntity
      */
     public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
     {

Now we have this:

 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Doctrine/ORM/Query/Filter/OwnerFilter.php                                                                                                                                                                                                                                                
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  39     Parameter #1 $targetEntity (Doctrine\ORM\Mapping\ClassMetadata<T of Doctrine\ORM\Mapping\ClassMetadata>) of method App\Doctrine\ORM\Query\Filter\OwnerFilter::addFilterConstraint() should be contravariant with parameter $targetEntity (Doctrine\ORM\Mapping\ClassMetadata) of method  
         Doctrine\ORM\Query\Filter\SQLFilter::addFilterConstraint()                                                                                                                                                                                                                               

It looks like this is because we don't have any template here: https://github.com/doctrine/orm/blob/2.10.x/lib/Doctrine/ORM/Query/Filter/SQLFilter.php#L194-L201

However, if we do this:

diff --git a/src/Doctrine/ORM/Query/Filter/OwnerFilter.php b/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
index 49a0fb790..6ac140e45 100644
--- a/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
+++ b/src/Doctrine/ORM/Query/Filter/OwnerFilter.php
@@ -32,6 +32,9 @@ final class OwnerFilter extends SQLFilter
 
     /**
      * {@inheritdoc}
+     *
+     * @psalm-template T of ClassMetadata
+     * @psalm-param T $targetEntity
      */
     public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias)
     {

We don't have any error. But this looks wrong as T should be the entity itself and not the ClassMetadata instance.

Maybe there is no bug, but I didn't find any clear documentation about this kind of implement either.

[greg0ire]

Hi buddy!

We try to add the correct templated parameter:

+     * @psalm-template T of ClassMetadata
+     * @psalm-param ClassMetadata<T> $targetEntity

wut?

T of ClassMetadata?

This means the line below expands to ClassMetadata<ClassMetadata>>… should it be T of Owner or something instead?

[KevinVanSonsbeek]

The problem is that the SQLFilter does not specify type T for the $targetEntity parameter on the abstract addFilterConstraint method.

But trying to define T in a superseding class will cause the should be contravariant with parameter $targetEntity to be given by phpstan.

<?php

declare(strict_types=1);

namespace App\Doctrine\Filters;

use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Query\Filter\SQLFilter;

final class PersonFilter extends SQLFilter
{
    /**
     * @inheritDoc
     *
     * @param ClassMetadata<object> $targetEntity
     */
    public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias): string
    {
        // Do something
    }
}

[greg0ire]

@KevinVanSonsbeek consider sending a PR to address this 🙏

[KevinVanSonsbeek]

@greg0ire PR is open 👍