-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Psalmify QueryBuilder, Query and EntityRepository. #11406
base: 3.2.x
Are you sure you want to change the base?
Conversation
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 |
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 |
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 $qb = $productRepository->createQueryBuilder('p');
$qb->innerJoin('p.category', 'c')
->select('c'); // becomes QueryBuilder<mixed> instead of QueryBuilder<Category> However, this will fallback to Due to string nature of |
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:
All of this started from @arnaud-lb work phpstan/phpstan-doctrine#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:
There is sort of BC-break if the plugin could already compute the result, and is not able after the PR @zmitic
I assume you're a psalm-user because PHPStan-plugin already offer lot of feature on doctrine side. |
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 $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 About 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. |
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.
PHPStan does work for EntityRepository too.
is the same than
There are tests like
or
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. |
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 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 But that's exactly what any call to So, let's please not fiddle with |
Isn't that the most common use-case? Other functionalities are not affected and they resort to
It would become 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). |
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 "require": {
"doctrine/orm": "dev-psalmify-query-builder as 3.1.1"
},
"repositories": [
{
"type": "github",
"url": "https://github.com/zmitic/orm.git"
}
], |
No, it's just the most trivial one.
My example was merely theoretical, from the PoV of an omniscient observer. But you're right, in this PR we fall back to
I gave you my counter argument in the one paragraph that you've ignored. ✌🏻
Thank you @VincentLanglet, that's exactly what I want to avoid. Putting generics on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking the merge for the reasons I've given.
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.