psalm / psalm-plugin-doctrine

Stubs to let Psalm understand Doctrine better
86 stars 43 forks source link

fix query stub #114

Closed rgrassian closed 2 years ago

rgrassian commented 2 years ago

Hi @orklah, this pr allows a query of MyEntity to return something else than Query, if I select myEntity.label for instance. A queryBuilder could return any type of query. That is why there is no template in phpstan: https://github.com/phpstan/phpstan-doctrine/blob/master/stubs/ORM/QueryBuilder.stub

Btw the type of the hydrationMode was wrong: https://github.com/doctrine/orm/blob/2.11.x/lib/Doctrine/ORM/AbstractQuery.php#L864.

orklah commented 2 years ago

Thanks!

@dimajolkin this was a change you proposed not too long ago, do you agree with the proposed change?

(I don't use doctrine a lot so I prefer reaching a consensus before merging things like this :) )

dimajolkin commented 2 years ago

Hello) Sorry but your code will break my project)

  1. @param int $hydrationMode to @param int|string $hydrationMode it's ok) the rest is bad for my project...

For example i'm use as


    /**
     * @return list<Theme>
     */
    public function fetchTeachers(): iterable
    {
        $qb = $this->createQueryBuilder('t');
       .....
        return $qb->getQuery()->getResult();
    }
orklah commented 2 years ago

Yeah but that's not quite the question :)

The question is to be correct in what the method can return. If it can return something else than an entity, it's not correct to leave an entity in the return type

dimajolkin commented 2 years ago

Sorry) Meybe you try it "@template T as mixed" ? can this solve your problem?

VincentLanglet commented 2 years ago

Hello) Sorry but your code will break my project)

For example i'm use as

    /**
     * @return list<Theme>
     */
    public function fetchTeachers(): iterable
    {
        $qb = $this->createQueryBuilder('t');
       .....
        return $qb->getQuery()->getResult();
    }

I would say that as long you dont use psalm level 1, it will still work.

On psalm level 1, you will need to add phpdoc after the getQuery to say it's a Query<Teacher>. Or you'll need to implements a more complexe feature in psalm-doctrine (like there is one in phpstan I think which use the model manager and multiple related things).

The question is to be correct in what the method can return. If it can return something else than an entity, it's not correct to leave an entity in the return type

@orklah This PR is totally correct to me

    public function fetchTeacherIds(): iterable
    {
        $qb = $this->createQueryBuilder('t')->select('t.id');
       .....
        return $qb->getQuery()->getResult();
    }

will be a list of id, and not a list of Teacher.

I can even do

    public function fetchPupils(): iterable
    {
        $qb = $this->createQueryBuilder('t')->select('p')->join('t.pupils', 'p');
       .....
        return $qb->getQuery()->getResult();
    }

And I will get a list of Pupils in the TeacherRepository.

The createQueryBuilder inside a EntityRepository is not limited to a QueryBuilder at all.

Then, it makes no sens to add a template param to the QueryBuilder class, since it can change as soon as I use select or addSelect on it. Moreover it's always a bad idea to add a template to a class in only one of repository psalm/phpstan, because users of both will get issues. And phpstan doesn't add a template to QueryBuilder https://github.com/phpstan/phpstan-doctrine/blob/master/stubs/ORM/QueryBuilder.stub which is the right thing IMHO.

On the contrary, Query can have a template. Because as soon as the getQuery method is called, you cannot modify it and you're sure that Query<Teacher> will give you a list of teacher if you call getResult on it.

dimajolkin commented 2 years ago

I see it) Thanks) in that case everything is ok

orklah commented 2 years ago

Thanks everyone :)