Support query inference with multiple entity managers#756
Conversation
|
Looking at https://github.com/phpstan/phpstan-doctrine/pull/757/changes#diff-e1bfea71addecdb2a3f73f91d95ed61b789458e5107d9efbbc39937d35b880fd some updates are also needed in |
|
Addressed the repository return-type path as well: the extension now reads the optional manager name argument and resolves the repository class through the matching object manager. I also fixed the lowest-dependency ObjectManager fixture compatibility issue and the coding-standard findings from the last run. CI is green on the latest head. |
|
Addressed the latest resolver simplification feedback in Changes made:
Validation:
|
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
will the DQL always contain a full-qualified class-name in the query?
There was a problem hiding this comment.
I don't see a test which contains a DELETE statement.. is this line tested?
There was a problem hiding this comment.
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 ManagerRegistry::getManagerForClass().
There was a problem hiding this comment.
No, not universally. This only supports the autoloadable class-string path. QueryBuilder/repository-generated DQL uses class metadata names, and direct createQuery() inference can resolve those FQCN-style class strings. If the DQL uses a short alias or anything not autoloadable, this falls back to the default configured manager rather than guessing.
There was a problem hiding this comment.
Good catch. I added focused coverage in badaaae using Doctrine's no-FROM form, DELETE QueryResult\MultipleEntityManagers\Tenant\App a, so it exercises the DELETE-specific extraction branch rather than the generic FROM branch.
Verification run in Docker with PHP 8.4:
vendor/bin/phpunit tests/Type/Doctrine/MultipleEntityManagersQueryTypeInferenceTest.php-> 9 tests / 9 assertionsvendor/bin/phpunit tests/Type/Doctrine-> 280 tests / 8073 assertionsgit diff --check-> clean
There was a problem hiding this comment.
@stof do you forsee whether this regex-matching is "good enough" for most DQL to identify the used entity-class?
(I am no DQL expert and don't know which syntax people use/is supported)
in case we can think about more DQL syntax edge cases, this might help to add more tests and harden the regex pattern
|
Quick check-in on this. The latest head 0194cef expands the DQL coverage for multiple entity managers, and all CI checks are passing. I do not see any remaining unresolved review request on my side, but I am happy to adjust anything else if there is still a blocker before this can move forward. |
I was waiting the answer from stof since staabm asked him in this comment |
Summary
Fixes return type inference for projects that use multiple Doctrine entity managers by allowing
objectManagerLoaderto return a DoctrineManagerRegistry.When the loader returns a registry, PHPStan Doctrine now resolves the object manager from the entity class found in the DQL before validating or inferring query result types. This keeps the existing single-manager loader behavior intact while supporting Symfony applications where each entity namespace is owned by a different manager.
Details
ManagerRegistrysupport toObjectMetadataResolver.getManagerForClass()when possible.objectManagerLoadersetups.Verification
vendor/bin/phpunitobjectManagerLoaderand running PHPStan againsttedyno/phpstan-multiple-em-issue; it reports no errors with this branch.Fixes #655