feat: add container handling to ResourceRelationServiceProxy #4198
Conversation
bartlomiejmarszal
requested review from
a team,
tikhanovichA,
KirylHatalski,
pnal,
Karol-Stelmaczonek and
viktar-dzmitryieu-tao
and removed request for
a team
Front-end summary Node 18
|
bartlomiejmarszal
changed the title
| if ( | ||
| $this->getServiceManager()->has($serviceId) | ||
| || $this->getServiceManager()->getContainer()->has($serviceId) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
@bartlomiejmarszal there is no need for double checking, the DI container implementation also supports checking the legacy registry.
| if ( | |
| $this->getServiceManager()->has($serviceId) | |
| || $this->getServiceManager()->getContainer()->has($serviceId) | |
| ) { | |
| return true; | |
| } | |
| if ($this->getServiceManager()->getContainer()->has($serviceId)) { | |
| return true; | |
| } |
| @@ -65,6 +67,11 @@ public function findRelations(FindAllQuery $query): ResourceRelationCollection | |||
| } | |||
|
|
|||
| foreach ($services as $serviceId) { | |||
| // Instead of throwing error when service does not exist, we just skip it | |||
| if (!$this->serviceExist($serviceId, $type)) { | |||
There was a problem hiding this comment.
For better performance, rather than creating a new method, I suggest the following:
1 - create a variable $container = $this->getServiceManager()->getContainer() on the line 68.
2 - on the like 70, just to the following
if (!$container->has($serviceId)) {
continue;
}In this way, you do not need to keep calling service manager and container inside the foreach.
There was a problem hiding this comment.
Btw, why do we need to do this check at all?
Better thinking about this, the service existing check is not required at all. Either we add or remove those services via DI or existing migrations, so the services must always exist.
If there is a corrupted installation that left a wrong service registered, the exception will be triggered, which is better than masking the error, but this is highly unlink to happen.
| try { | ||
| return $this->getServiceManager()->get($serviceId); | ||
| } catch (ServiceNotFoundException $exception) { | ||
| return $this->getServiceManager()->getContainer()->get($serviceId); | ||
| } |
There was a problem hiding this comment.
No need for it, the container does it automatically for you :)
| try { | |
| return $this->getServiceManager()->get($serviceId); | |
| } catch (ServiceNotFoundException $exception) { | |
| return $this->getServiceManager()->getContainer()->get($serviceId); | |
| } | |
| return $this->getServiceManager()->getContainer()->get($serviceId); |
| } | ||
| } | ||
|
|
||
| private function serviceExist(string $serviceId, string $type): bool |
There was a problem hiding this comment.
You do not need this method as per better performance, you can apply this logic here https://github.com/oat-sa/tao-core/pull/4198/files#r1944649031
| return true; | ||
| } | ||
|
|
||
| common_Logger::w( |
There was a problem hiding this comment.
If you still want to log it (in the new place I mentioned), please use the proper LoggerService as this static call is not recommended. Thank you!
There was a problem hiding this comment.
Better thinking about this, the service existing check is not required at all. Either we add or remove those services via DI or existing migrations, so the services must always exist.
If there is a corrupted installation that left a wrong service registered, the exception will be triggered, which is better than masking the error, but this is highly unlink to happen.
Version
There are 0 BREAKING CHANGE, 3 features, 1 fix |
This change will allow using containers and configurable services in
ResourceRelationServiceProxy