Open zmitic opened 3 months ago
It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.
Also, please take a look at tests/StaticAnalysis
, it could give you ideas on how to demonstrate usage.
@greg0ire
Thanks, I was looking for the contribution guide and then forgot, sorry.
The fixes has been applied, and I tried to match the rules from tests/StaticAnalysis
.
I'm pretty sure, these doc blocks oversimplify how the query builder works. I wonder if inferring types for the query builder is better placed in the Doctrine plugins for Psalm and PHPStan.
@derrabus it's not obvious to me... Can you give an example of what subtleties are being missed here?
@derrabus There is at least one use-case where these stubs are not 100% correct:
$qb = $productRepository->createQueryBuilder('p');
$qb->innerJoin('p.category', 'c')
->select('c'); // becomes QueryBuilder<mixed> instead of QueryBuilder<Category>
However, this will fallback to mixed
so there shouldn't be any BC problems. Stubs cover only most used case.
Due to string nature of from
and select
methods, making a plugin would probably require inner calls to Doctrine to avoid writing a parser again. I am not sure it would be worth the effort as it would slow down things for little benefit, and TBH, it is above my skills.
I'm pretty sure, these doc blocks oversimplify how the query builder works. I wonder if inferring types for the query builder is better placed in the Doctrine plugins for Psalm and PHPStan.
There is already a pretty complex logic in PHPStan-doctrine plugin. https://github.com/phpstan/phpstan-doctrine/tree/1.4.x
Adding template here is not a bad thing IMHO, but it should be at least compatible with the template defined in the PHPStan-doctrine, and some PR will maybe be needed in the plugin to.
Currently:
Query<TResult>
when Phpstan already use Query<TIndex, TResult>
, see
QueryBuilder<TQuery>
when Phpstan doesn't use any but rely on a dynamicReturnTypeExtension instead for the createQuery method
All of this started from @arnaud-lb work https://github.com/phpstan/phpstan-doctrine/pull/232 (and many other PR), it would be nice to not break this with different behavior on doctrine/orm.
If template are added in doctrine/orm I would recommend:
Query<TIndex, TResult>
rather than Query<T>
QueryBuilder<T>
doesn't conflict on PHPStan side (and maybe it could help to simplify things ?)QueryBuilder::select
phpdoc change doesn't conflict on PHPStan side (it would be bad if PHPStan cannot determine anymore the result of a query, because such phpdoc lose the template type when it could have been kept/computed).@derrabus There is at least one use-case where these stubs are not 100% correct:
$qb = $productRepository->createQueryBuilder('p'); $qb->innerJoin('p.category', 'c') ->select('c'); // becomes QueryBuilder<mixed> instead of QueryBuilder<Category>
However, this will fallback to
mixed
so there shouldn't be any BC problems. Stubs cover only most used case.
There is sort of BC-break if the plugin could already compute the result, and is not able after the PR @zmitic
Due to string nature of
from
andselect
methods, making a plugin would probably require inner calls to Doctrine to avoid writing a parser again. I am not sure it would be worth the effort as it would slow down things for little benefit, and TBH, it is above my skills.
I assume you're a psalm-user because PHPStan-plugin already offer lot of feature on doctrine side. It's not "little benefit"` to implement the same in a psalm-plugin.
Thanks @VincentLanglet , plenty of info here. I will check the plugin by Wednesday and see if there are conflicts. It is true that I am using psalm but I would argue that if Doctrine itself can be stubbed, it should and not rely on external plugins.
To explain the "little benefit" part: these are stubs only for EntityRepository
, not for EntityManager
. So example like this:
$query = $entityManager->createQuery('SELECT u FROM Acme\User u');
$query->getResult(); // array<Acme\User>
would not be affected by stubs because these queries start from $em
. But if user starts from repository, stubs only put the default result (Acme\User); I still think phpstan would not be making problems.
AboutQuery<TIndex, TEntity>
; can you elaborate on TIndex? I can't find anything here other than it is int|string
. I also just tested indexing by association, and Doctrine used ID value as a key which is to be expected; objects can't be used as array keys.
Am I missing something here? It would be best if you could paste some real code using it, I don't mind adding it if seemed useful.
Thanks @VincentLanglet , plenty of info here. I will check the plugin by Wednesday and see if there are conflicts. It is true that I am using psalm but I would argue that if Doctrine itself can be stubbed, it should and not rely on external plugins.
I agree, it's better when it's coming from Doctrine itself ; but PHPStan added lot of rule/feature that require some level of knowledge about PHPStan internal tool, so it's understandable to not have this maintained by doctrine maintainers.
Having the template phpdoc in doctrine seems ok to me ; it could just be useful to ping ondrejmirtes when needed to check it won't conflict.
Maybe the best way to see if the PR (after the template of Query is updated with the Index too) doesn't conflict would be to open a PR on PHPStan-doctrine with all this PHPDoc/stub changes, and see if all the existing tests from PHPStan-doctrine is still green.
To explain the "little benefit" part: these are stubs only for
EntityRepository
, not forEntityManager
. So example like this:$query = $entityManager->createQuery('SELECT u FROM Acme\User u'); $query->getResult(); // array<Acme\User>
would not be affected by stubs because these queries start from
$em
. But if user starts from repository, stubs only put the default result (Acme\User); I still think phpstan would not be making problems.
PHPStan does work for EntityRepository too. There is this extension https://github.com/phpstan/phpstan-doctrine/blob/0871900872abde0d93e4bbd8c681a9985bcd927e/src/Type/Doctrine/QueryBuilder/EntityRepositoryCreateQueryBuilderDynamicReturnTypeExtension.php#L17 which basically say
$this->createQueryBuilder()
is the same than
$this->getEntityManager()->createQueryBuilder()->select()->from()
About
Query<TIndex, TEntity>
; can you elaborate on TIndex? I can't find anything here other than it isint|string
. I also just tested indexing by association, and Doctrine used ID value as a key which is to be expected; objects can't be used as array keys.Am I missing something here? It would be best if you could paste some real code using it, I don't mind adding it if seemed useful.
There are tests like
$result = $em->createQueryBuilder()
->select('m')
->from(Many::class, 'm', 'm.stringColumn')
->getQuery() // Query<string, QueryResult\Entities\Many>
->getResult();
assertType('array<string, QueryResult\Entities\Many>', $result);
or
$result = $em->createQueryBuilder()
->select(['m.intColumn', 'm.stringNullColumn'])
->from(Many::class, 'm')
->indexBy('m', 'm.stringColumn')
->getQuery() // Query<string, array{intColumn: int, stringNullColumn: string|null}>
->getResult();
assertType('array<string, array{intColumn: int, stringNullColumn: string|null}>', $result);
in the PHPStan-doctrine codebase.
Also, be aware that it's TIndex, TValue and none TEntity, because as shown in the last example there are query with non-object result which are correctly understood by PHPStan. But for sure, with simple phpdoc it might be too hard to understand this.
There is at least one use-case where these stubs are not 100% correct:
That's an understatement. There's only one use-case where the out-of-the-box experience for the query builder is actually improved and that's calling EntityRepository::createQueryBuilder()
without any calls to select()
afterwards. That's it.
On the other hand, downstream projects will get tons of new static analysis errors because they dare to use the now-generic query builder in method signatures without the proper docblocks. That's a lot of busy-work for a little gain in a trivial use-case.
Furthermore, it is quite unusual that an object changes its type. A Collection<int, Foo>
is a Collection<int, Foo>
perpetually and any attempts in adding something other than an instance of Foo
to that collection would be rendered invalid (although that's not enforced at runtime).
But that's exactly what any call to QueryBuilder::select()
would do. Or QueryBuilder::addSelect()
or QueryBuilder::add()
for that matter, both missed by this PR btw. A call to QueryBuilder<Foo>::select('f.id')
would not just return a new QueryBuilder<int>
, it would transform the actual instance into a QueryBuilder<int>
(assuming that Foo::$id
is of type int
and f
is the alias of Foo
). Generics are a really bad fit for this kind of behavior.
So, let's please not fiddle with QueryBuilder
here and leave inferring dynamic types on that class to the corresponding plugins which will always do a better than any generic doc block on QueryBuilder
.
There's only one use-case where the out-of-the-box experience for the query builder is actually improved and that's calling EntityRepository::createQueryBuilder() without any calls to select() afterwards. That's it.
Isn't that the most common use-case? Other functionalities are not affected and they resort to mixed
, as it was before. phpstan plugin will keep working as before.
A call to
QueryBuilder<Foo>::select('f.id')
would not just return a newQueryBuilder<int>
, it would transform the actual instance into aQueryBuilder<int>
It would become QueryBuilder<mixed>
to keep BC but yes, QB mutates itself (hence the @psalm-this-out static<mixed>
).
As explained by @VincentLanglet , this precise type resolving is done by phpstan plugin. I yet have to install it and try, can't do it in next 2 days, but I am confident that the 2 will nicely cooperate.
My argument is that if something can be done within Doctrine, it should be done within Doctrine and not rely on external plugins especially when there is no BC problem (or at least I can't see it).
On the other hand, downstream projects will get tons of new static analysis errors because they dare to use the now-generic query builder in method signatures without the proper docblocks. That's a lot of busy-work for a little gain in a trivial use-case.
To explain this part @zmitic, PHPStan generally ask to fill generic phpdoc. So Phpstan users will get errors, asking to rewrite every "QueryBuilder" phpdoc to "QueryBuilder<mixed". That's one example of possibly created issue/extra work
It can also create covariant/invariant issue if the template is not covariant.
@VincentLanglet If you have some opened project that doesn't show any errors, can you make a quick test? You can copy&paste this into composer.json:
"require": {
"doctrine/orm": "dev-psalmify-query-builder as 3.1.1"
},
"repositories": [
{
"type": "github",
"url": "https://github.com/zmitic/orm.git"
}
],
There's only one use-case where the out-of-the-box experience for the query builder is actually improved and that's calling EntityRepository::createQueryBuilder() without any calls to select() afterwards. That's it.
Isn't that the most common use-case?
No, it's just the most trivial one.
A call to
QueryBuilder<Foo>::select('f.id')
would not just return a newQueryBuilder<int>
, it would transform the actual instance into aQueryBuilder<int>
It would become
QueryBuilder<mixed>
to keep BC but yes
My example was merely theoretical, from the PoV of an omniscient observer. But you're right, in this PR we fall back to mixed
. Not for BC, but because we simply can't do better with doc blocks. And that's part of the reason why I would delegate this task to the Psalm and PHPStan plugins.
My argument is that if something can be done within Doctrine, it should be done within Doctrine and not rely on external plugins especially when there is no BC problem (or at least I can't see it).
I gave you my counter argument in the one paragraph that you've ignored. ✌🏻
On the other hand, downstream projects will get tons of new static analysis errors because they dare to use the now-generic query builder in method signatures without the proper docblocks. That's a lot of busy-work for a little gain in a trivial use-case.
To explain this part @zmitic, PHPStan generally ask to fill generic phpdoc. So Phpstan users will get errors, asking to rewrite every "QueryBuilder" phpdoc to "QueryBuilder<mixed". That's one example of possibly created issue/extra work
It can also create covariant/invariant issue if the template is not covariant.
Thank you @VincentLanglet, that's exactly what I want to avoid. Putting generics on the QueryBuilder
class will be a big clusterfuck downstream.
Added templates for QueryBuilder, Query and EntityRepository.
With repository defined like:
Using it:
If at any point user makes a call to
QueryBuilder::select
, it will be converted tomixed
via@psalm-this-out self<mixed>
. For example:The reasoning is that almost all queries are done with object hydration and this would help in later processing. Custom
select
will break out of static analysis and it is up to user to assert returned results.