Fix return type on EntityRepository::matching() generics

[Firehed]

Fixes a regression where the collection returned by matching() would lose the generic metadata, and downstream code reading the values would get mixed.

e.g. this worked under ORM2 + this plugin, but fails under ORM3 because $feed became mixed:

        $feeds = $this->em->getRepository(Feed::class)
            ->matching($crit);

        foreach ($feeds as $feed) {
            $this->submitter->submitUpdateFeedJob($feed);
        }

This changed in ORM3, which actually has native types that are correct and were being overridden by the stub. I've updated the stub and added a test to reflect the return value.

Note: ORM3 specifies AbstractLazyCollection&Selectable as the return type, but AbstractLazyCollection already implements Selectable so PHPStan refines it out as redundant.

[VincentLanglet]

According to doctrine/orm it should be

AbstractLazyCollection<int, T>&Selectable<int, T>

NB: this was introduced in doctrine/orm 2.8+
doctrine/orm@f1219f1

Given the fact we have the correct type in doctrine/orm in 2.16+
https://github.com/doctrine/orm/blame/495cd06b9a630f9c38a21ceb249ed008edbe8414/lib/Doctrine/ORM/EntityRepository.php#L327
and the fact we only test this lib on 2.16+

phpstan-doctrine/composer.json

Line 30 in 81dac0e

"doctrine/orm": "^2.16.0",

Maybe it's better to remove the method and rely on the original signature ?

[Firehed]

I originally had it with that signature and PHPStan refined it out (as noted, &Selectable is redundant). But if removing the method from the stub works, that's preferable. I'd made an assumption that stubs were all-or-nothing with class definitions. I'll give it a try and update, thanks!

[Firehed]

@VincentLanglet if I remove the definition for matching from the stub outright to use the native type info, I get this type of output:

2) PHPStan\DoctrineIntegration\TypeInferenceTest::testFileAsserts with data set "/Users/firehed/dev/github/firehed/phpstan-doctrine/tests/DoctrineIntegration/data/repositoryMatching.php:34" ('type', '/Users/firehed/dev/github/fir...ng.php', 'Doctrine\Common\Collections\A...ntity>', 'Doctrine\Common\Collections\A...eter)>', 34)
Expected type Doctrine\Common\Collections\AbstractLazyCollection<int, RepositoryMatching\MyEntity>, got type Doctrine\Common\Collections\AbstractLazyCollection<int, T of object (class Doctrine\ORM\EntityRepository, parameter)> in /Users/firehed/dev/github/firehed/phpstan-doctrine/tests/DoctrineIntegration/data/repositoryMatching.php on line 34.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Doctrine\Common\Collections\AbstractLazyCollection<int, RepositoryMatching\MyEntity>'
+'Doctrine\Common\Collections\AbstractLazyCollection<int, T of object (class Doctrine\ORM\EntityRepository, parameter)>'

phar:///Users/firehed/dev/github/firehed/phpstan-doctrine/vendor/phpstan/phpstan/phpstan.phar/src/Testing/TypeInferenceTestCase.php:115
/Users/firehed/dev/github/firehed/phpstan-doctrine/tests/DoctrineIntegration/TypeInferenceTest.php:28

3) PHPStan\DoctrineIntegration\TypeInferenceTest::testFileAsserts with data set "/Users/firehed/dev/github/firehed/phpstan-doctrine/tests/DoctrineIntegration/data/repositoryMatching.php:23" ('type', '/Users/firehed/dev/github/fir...ng.php', 'RepositoryMatching\MyEntity', 'T of object (class Doctrine\O...meter)', 23)
Expected type RepositoryMatching\MyEntity, got type T of object (class Doctrine\ORM\EntityRepository, parameter) in /Users/firehed/dev/github/firehed/phpstan-doctrine/tests/DoctrineIntegration/data/repositoryMatching.php on line 23.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'RepositoryMatching\MyEntity'
+'T of object (class Doctrine\ORM\EntityRepository, parameter)'

This implies to me that info is getting lost in the call to getRepository(), despite both EntityManager and EntityManagerInterface having @return EntityRepository<T> per the source code.

I don't particularly understand why this is the case here - all of the types in the stubs look correct to my eyes. Perhaps something is off with a @template-covariant or something?

Given that, I left it in place (but re-added the &Selectable from upstream).

The PHPstan failure itself I don't understand at all - it says Doctrine\ORM\EntityRepository::matching() has invalid return type Doctrine\Common\Collections\AbstractLazyCollection. but that is absolutely a valid, installed, findable class. I triple-checked the imports to make sure there wasn't a copy-paste error and couldn't find anything.

[VincentLanglet]

If you use AbstractLazyCollection in a stub, a stub is need for AbstractLazyCollection too

[Firehed]

Ended up taking a different path here, trying to rectify the subtle differences between ORM v2 and v3 didn't seem to work with purely stub tweaks. That said, there are still some adjustments that correct divergence from the actual library. I tested it against a real project updated to ORMv3 (using the local composer repository symlink thing) and saw the expected behavior, and was able to remove some baselines and @var overrides.

Happy to adjust further, LMK what you think.

The platform test seems to be unrelated and failing on the default branch.

[VincentLanglet]

WDYT about #743 @Firehed ?

[Firehed]

@VincentLanglet LGTM - I tested your patch (at 9e07424) against my problem-repo and that also fixed the issue. It's closer to what I was originally aiming for, but you're far more familiar with the internals than I am so I guess you figured out the right trick. Feel free to close this in favor of your implementation.

[VincentLanglet]

Thanks, after the merge, I'll also try to reduce the stub by removing some methods.

For instance I think the issue you encounter when you tried to remove matching is because the template doesn't use the same name than the one defined in doctrine/orm. And this should be fixed IMHO.

[Firehed]

Sounds good, thanks for the assist!