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

fix query stub #114

Merged
merged 1 commit into from
Mar 3, 2022
Merged

fix query stub #114

merged 1 commit into from
Mar 3, 2022

Conversation

rgrassian
Copy link
Contributor

@rgrassian rgrassian commented Mar 3, 2022

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

orklah commented Mar 3, 2022

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

dimajolkin commented Mar 3, 2022

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

orklah commented Mar 3, 2022

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

dimajolkin commented Mar 3, 2022

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

@VincentLanglet
Copy link
Contributor

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

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

@orklah
Copy link
Collaborator

orklah commented Mar 3, 2022

Thanks everyone :)

@orklah orklah merged commit 93fa799 into psalm:2.x Mar 3, 2022
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

Successfully merging this pull request may close these issues.

4 participants