3.next: add DIC support to OrmResolver #275
Conversation
| @@ -146,7 +159,13 @@ protected function findPolicy(string $class, string $name, string $namespace): m | |||
| throw new MissingPolicyException([$class]); | |||
| } | |||
|
|
|||
| return new $policyClass(); | |||
| if ($this->container && $this->container->has($policyClass)) { | |||
| $policy = $this->container->get($policyClass); | |||
There was a problem hiding this comment.
Per the interface, get() can throw a variety of exceptions. Should this be handled by this method (i.e. fallback style) or leave it entirely to userland code?
There was a problem hiding this comment.
Well get($policyClass) won't throw any more since we checked the container with has($policyClass).
This is the same procedure as we do when parsing controller action arguments
just with the difference that the container could not be set in our case to be as BC as possible.
There was a problem hiding this comment.
Technically, $this->container->get() can still throw ContainerExceptionInterface however you're correct in that the has() check will prevent the NotFoundException from being thrown.
At least, that's my understanding from reading ContainerInterface. I checked out the ControllerFactory code and see what you mean...but I must ask: should we be catching/handling the container exception there as well?
There was a problem hiding this comment.
We use the league/container as an implementation for the PSR-11 Container Interface. The only reason this exception could be thrown is here.
So technically, one could add a service provider which lies about providing a specific service.
But then I'd say its fine if we just let that exception bubble up to the top and not catch it because it's a configuration problem of the user (or a plugin), not something we should actively catch and handle for the user.
|
Seems like a good change to me. |
|
@jamisonbryant anything to add from your side? If not I'd merge this and add support for DI to the MapResolver as well |
|
No, this looks good to me. Well done! |
This is a proposal initiated by @jamisonbryant to make at least the OrmResolver more DI aware.
Instead of having to pass on services to
->can()etc. one can easily inject the needed services into the policies which actually need them to do whatever logic is required to authorize the current user on the given action.To get this working from a user perspective, all they would have to adjust in their
Application::getAuthorizationServicemethod isto
After that all policies retrieved by the OrmResolver can leverage constructor injection.
If we want to go this way then we should also make the MapResolver DI aware as well.