-
Notifications
You must be signed in to change notification settings - Fork 120
Support query inference with multiple entity managers #756
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
base: 2.0.x
Are you sure you want to change the base?
Changes from all commits
5c3489d
4166de0
0153771
b10343c
84d7812
3568a87
badaaae
0194cef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,20 @@ | |
| use Doctrine\ORM\EntityManagerInterface; | ||
| use Doctrine\ORM\Mapping\ClassMetadata; | ||
| use Doctrine\ORM\Mapping\MappingException; | ||
| use Doctrine\Persistence\ManagerRegistry; | ||
| use Doctrine\Persistence\ObjectManager; | ||
| use PHPStan\Doctrine\Mapping\ClassMetadataFactory; | ||
| use PHPStan\ShouldNotHappenException; | ||
| use ReflectionException; | ||
| use Throwable; | ||
| use function array_merge; | ||
| use function class_exists; | ||
| use function count; | ||
| use function is_file; | ||
| use function is_readable; | ||
| use function method_exists; | ||
| use function preg_match_all; | ||
| use function reset; | ||
| use function sprintf; | ||
| use const PHP_VERSION_ID; | ||
|
|
||
|
|
@@ -23,8 +29,8 @@ final class ObjectMetadataResolver | |
|
|
||
| private ?string $objectManagerLoader = null; | ||
|
|
||
| /** @var ObjectManager|false|null */ | ||
| private $objectManager; | ||
| /** @var ObjectManager|ManagerRegistry|false|null */ | ||
| private $objectManagerLoaderResult; | ||
|
|
||
| private ?ClassMetadataFactory $metadataFactory = null; | ||
|
|
||
|
|
@@ -47,23 +53,99 @@ public function hasObjectManagerLoader(): bool | |
| /** @api */ | ||
| public function getObjectManager(): ?ObjectManager | ||
| { | ||
| if ($this->objectManager === false) { | ||
| $objectManagerLoaderResult = $this->getObjectManagerLoaderResult(); | ||
| if (!$objectManagerLoaderResult instanceof ManagerRegistry) { | ||
| return $objectManagerLoaderResult; | ||
| } | ||
|
|
||
| return $objectManagerLoaderResult->getManager(); | ||
| } | ||
|
|
||
| /** | ||
| * @param class-string $className | ||
| */ | ||
| public function getObjectManagerForClass(string $className): ?ObjectManager | ||
| { | ||
| $objectManagerLoaderResult = $this->getObjectManagerLoaderResult(); | ||
| if (!$objectManagerLoaderResult instanceof ManagerRegistry) { | ||
| return $objectManagerLoaderResult; | ||
| } | ||
|
|
||
| $objectManager = $objectManagerLoaderResult->getManagerForClass($className); | ||
| if ($objectManager instanceof ObjectManager) { | ||
| return $objectManager; | ||
| } | ||
|
|
||
| return $this->getObjectManager(); | ||
| } | ||
|
|
||
| public function getObjectManagerByName(string $name): ?ObjectManager | ||
| { | ||
| $objectManagerLoaderResult = $this->getObjectManagerLoaderResult(); | ||
| if (!$objectManagerLoaderResult instanceof ManagerRegistry) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($this->objectManager !== null) { | ||
| return $this->objectManager; | ||
| try { | ||
| return $objectManagerLoaderResult->getManager($name); | ||
| } catch (Throwable $e) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public function getObjectManagerForDql(string $dql): ?ObjectManager | ||
| { | ||
| $objectManagerLoaderResult = $this->getObjectManagerLoaderResult(); | ||
| if (!$objectManagerLoaderResult instanceof ManagerRegistry) { | ||
| return $objectManagerLoaderResult; | ||
| } | ||
|
|
||
| preg_match_all('~\b(?:FROM|UPDATE)\s+([\\\\A-Za-z_][\\\\A-Za-z0-9_]*)(?:\s+|$)~i', $dql, $matches); | ||
| preg_match_all('~\bDELETE\s+(?:FROM\s+)?([\\\\A-Za-z_][\\\\A-Za-z0-9_]*)(?:\s+|$)~i', $dql, $deleteMatches); | ||
|
VincentLanglet marked this conversation as resolved.
Comment on lines
+103
to
+104
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if you don't use doctrine ; I saw you worked a lot with regex, so I'd like your opinion on this strategy @staabm
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do I see this correctly, that we need this regex-magic, because we cannot get a AST for the DQL since we are in the process of creating a entity manager?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will the DQL always contain a full-qualified class-name in the query?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see a test which contains a DELETE statement.. is this line tested?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the reason for the pre-parse step. At this point we need a candidate entity class before handing the DQL to Doctrine's EM-bound parser/metadata validation; otherwise the default manager can reject a tenant entity before we know which manager should parse it. The regex is intentionally a narrow heuristic to find an autoloadable root entity class, then the actual manager choice is delegated to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, not universally. This only supports the autoloadable class-string path. QueryBuilder/repository-generated DQL uses class metadata names, and direct
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I added focused coverage in Verification run in Docker with PHP 8.4:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stof do you forsee whether this regex-matching is "good enough" for most DQL to identify the used entity-class? in case we can think about more DQL syntax edge cases, this might help to add more tests and harden the regex pattern |
||
| foreach (array_merge($matches[1], $deleteMatches[1]) as $className) { | ||
| if (!class_exists($className)) { | ||
| continue; | ||
| } | ||
|
|
||
| $objectManager = $objectManagerLoaderResult->getManagerForClass($className); | ||
| if ($objectManager !== null) { | ||
| return $objectManager; | ||
| } | ||
| } | ||
|
|
||
| return $this->getObjectManager(); | ||
| } | ||
|
|
||
| /** | ||
| * @return ObjectManager|ManagerRegistry|null | ||
| */ | ||
| private function getObjectManagerLoaderResult() | ||
| { | ||
| if ($this->objectManagerLoaderResult === false) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($this->objectManagerLoaderResult !== null) { | ||
| return $this->objectManagerLoaderResult; | ||
| } | ||
|
|
||
| if ($this->objectManagerLoader === null) { | ||
| $this->objectManager = false; | ||
| $this->objectManagerLoaderResult = false; | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| $this->objectManager = $this->loadObjectManager($this->objectManagerLoader); | ||
| $objectManagerLoaderResult = $this->loadObjectManager($this->objectManagerLoader); | ||
| if ($objectManagerLoaderResult instanceof ManagerRegistry) { | ||
| $objectManagers = $objectManagerLoaderResult->getManagers(); | ||
| if (count($objectManagers) === 1) { | ||
| $objectManagerLoaderResult = reset($objectManagers); | ||
| } | ||
| } | ||
|
|
||
| $this->objectManagerLoaderResult = $objectManagerLoaderResult; | ||
|
|
||
| return $this->objectManager; | ||
| return $this->objectManagerLoaderResult; | ||
| } | ||
|
|
||
| public function isNativeLazyObjectsEnabled(): bool | ||
|
|
@@ -99,7 +181,7 @@ public function isTransient(string $className): bool | |
| return true; | ||
| } | ||
|
|
||
| $objectManager = $this->getObjectManager(); | ||
| $objectManager = $this->getObjectManagerForClass($className); | ||
|
|
||
| try { | ||
| if ($objectManager === null) { | ||
|
|
@@ -143,7 +225,7 @@ public function getClassMetadata(string $className): ?ClassMetadata | |
| return null; | ||
| } | ||
|
|
||
| $objectManager = $this->getObjectManager(); | ||
| $objectManager = $this->getObjectManagerForClass($className); | ||
|
|
||
| try { | ||
| if ($objectManager === null) { | ||
|
|
@@ -172,7 +254,10 @@ public function getClassMetadata(string $className): ?ClassMetadata | |
| return $ormMetadata; | ||
| } | ||
|
|
||
| private function loadObjectManager(string $objectManagerLoader): ?ObjectManager | ||
| /** | ||
| * @return ObjectManager|ManagerRegistry|null | ||
| */ | ||
| private function loadObjectManager(string $objectManagerLoader) | ||
| { | ||
| if (!is_file($objectManagerLoader)) { | ||
| throw new ShouldNotHappenException(sprintf( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Type\Doctrine; | ||
|
|
||
| use PHPStan\Testing\TypeInferenceTestCase; | ||
|
|
||
| class MultipleEntityManagersQueryTypeInferenceTest extends TypeInferenceTestCase | ||
| { | ||
|
|
||
| /** @return iterable<mixed> */ | ||
| public function dataFileAsserts(): iterable | ||
| { | ||
| yield from $this->gatherAssertTypes(__DIR__ . '/data/QueryResult/multipleEntityManagers.php'); | ||
| } | ||
|
|
||
| /** | ||
| * @dataProvider dataFileAsserts | ||
| * @param mixed ...$args | ||
| */ | ||
| public function testFileAsserts( | ||
| string $assertType, | ||
| string $file, | ||
| ...$args | ||
| ): void | ||
| { | ||
| $this->assertFileAsserts($assertType, $file, ...$args); | ||
| } | ||
|
|
||
| /** @return string[] */ | ||
| public static function getAdditionalConfigFiles(): array | ||
| { | ||
| return [__DIR__ . '/data/QueryResult/config-multiple-entity-managers.neon']; | ||
| } | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.