Skip to content

Allow to use both ORM and ODM #218

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

Closed
wants to merge 2 commits into from

Conversation

VincentLanglet
Copy link
Contributor

Might solve #53 (comment)
I need to check tomorrow.

@VincentLanglet VincentLanglet marked this pull request as ready for review October 7, 2021 06:49
@VincentLanglet
Copy link
Contributor Author

@ondrejmirtes I tested this on my project and it works.

The idea is that if you use a custom object manager the getResolvedRepositoryClass is not working and if you use both ORM and ODM, the method shouldn't be used since it cache the result and you can use two different repository classes.

So instead the repository class is directly determined from the metadata.

@ondrejmirtes
Copy link
Member

I need a test here so I can understand what are the ramifications.

@VincentLanglet
Copy link
Contributor Author

I need a test here so I can understand what are the ramifications.

I added one. Don't hesitate if you need more explanation or the test to be presented differently, because it wasn't easy to me to reproduce my issue.

If you implement your own ObjectManager in order to support both ORM and ODM depending on the class of the entity, you need the repository class to also depends on the entities. The object manager is correctly finding the classmetadata, so if the entity/document has a custom repository there is no issues. But in case there is not, prior to these change the default value was always the same repository and it could depend on the classmetadata.

I think it won't impact the perf of basics users since ?? has basically no cost, and it works for me (and for the tests).

@VincentLanglet
Copy link
Contributor Author

Friendly ping @ondrejmirtes ; is this PR ok for you ? :)

@ondrejmirtes
Copy link
Member

@VincentLanglet Sorry, I'm busy with PHPStan 1.0, I'll have a look at this after the release.

@VincentLanglet
Copy link
Contributor Author

Friendly ping @ondrejmirtes. Do you have time to take a look ? This would help me to migrate on phpstan 1 :)

@ondrejmirtes
Copy link
Member

@VincentLanglet My top priority right now is PHP 8.1 support (https://phpstan.org/blog/plan-to-support-php-8-1). The main action is now happening in https://github.com/ondrejmirtes/BetterReflection/tree/ng and https://github.com/phpstan/phpstan-src/tree/ng-better-reflection. Once this is finished and released, the operations will return to normal eventually. I now have 187 emails in my inbox worthy of my attention (all related to PHPStan), so please stay tuned.

@ondrejmirtes
Copy link
Member

I'm gonna fix it myself, stay tuned.

@VincentLanglet
Copy link
Contributor Author

Just saw that getResolvedRepositoryClass is only returning DocumentRepository when an objectManager was provided
https://github.com/phpstan/phpstan-doctrine/blob/master/src/Type/Doctrine/ObjectMetadataResolver.php#L163-L167 which is currently the blocking point.

Feel free to ping me if you want that I test your branch

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