Skip to content

fix: infer repositories from named managers#757

Open
ItsOtherMauridian wants to merge 1 commit into
phpstan:2.0.xfrom
ItsOtherMauridian:fix/named-manager-repository-inference
Open

fix: infer repositories from named managers#757
ItsOtherMauridian wants to merge 1 commit into
phpstan:2.0.xfrom
ItsOtherMauridian:fix/named-manager-repository-inference

Conversation

@ItsOtherMauridian
Copy link
Copy Markdown

Summary

  • supports ManagerRegistry object-manager loaders when resolving repository metadata
  • uses the constant second argument of ManagerRegistry::getRepository($class, $managerName) to resolve metadata from the named manager
  • adds a regression test where the same entity class has different custom repositories on the default and tenant managers

Why

When an application has multiple managers that know about the same entity class, the repository type can depend on the explicit manager name passed to ManagerRegistry::getRepository(). Without considering that argument, PHPStan can infer the repository class from the default manager even when the call explicitly requests another manager.

Local validation

Executed in Docker with Composer dependencies installed via:

composer install --no-interaction --no-progress --prefer-dist --ignore-platform-req=ext-mongodb

Validation commands:

php -l src/Type/Doctrine/ObjectMetadataResolver.php
php -l src/Type/Doctrine/GetRepositoryDynamicReturnTypeExtension.php
php -l tests/Type/Doctrine/NamedManagerRepositoryTypeInferenceTest.php
php -l tests/Type/Doctrine/data/namedManagerRepository.php
php -l tests/Type/Doctrine/data/named-manager-registry.php
php vendor/bin/phpunit --filter NamedManagerRepositoryTypeInferenceTest
php vendor/bin/phpunit tests/Type/Doctrine
git diff --check

Observed results:

OK (2 tests, 2 assertions)
OK (271 tests, 8064 assertions)

Disclosure

This PR was prepared with AI assistance and manually checked before submission.

@VincentLanglet
Copy link
Copy Markdown
Contributor

Hi @ItsOtherMauridian, this feels kinda similar to the already opened by @LubuSeb PR
#756 (with less place where the ManagerRegistry is used)

The main discussion #756 (comment) still stands and need a decision first.

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.

2 participants