Lazily resolve ManagerRegistry in EntityManagerProvider #664
Conversation
| public function getDefaultManager(): EntityManagerInterface | ||
| { | ||
| $entityManager = $this->managerRegistry->getManager(); | ||
| $entityManager = $this->getManagerRegistry()->getManager(); |
There was a problem hiding this comment.
@LegendEffects
Following the doctrine filosophy you shouldn't cache the get manager.
Just do the app(ManagerRegistry::class) or $this->container->make(ManagerRegistry::class) where you need it.
The make not gonna create another object, it's defined as singleton it's gonna inject previously created manager registry.
Why?
Because you gonna have control over the injected Manager Registry object.
You want to have this control for tests.
There was a problem hiding this comment.
Good point, I was just keeping the same code flow as before, I avoided using the app helper as there was no other instances of it being used in the package
|
I've updated the test runner in #665 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.1.x #664 +/- ##
=========================================
Coverage 80.93% 80.93%
Complexity 495 495
=========================================
Files 73 73
Lines 1631 1631
=========================================
Hits 1320 1320
Misses 311 311 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Tests fail because the new code in this patch isn't covered |
|
There are no tests present for the entire Console namespace, adding tests to this would also pretty much just be testing the PSR container so feels pointless. |
|
Thanks for the feedback. I agree. |
Fixes an issue where a connection will always be attempted when running any command (including package discovery) if there are custom types installed.
#663