Open Rarst opened 3 years ago
I'm not sure if silencing all exceptions thrown by callable resolver is the best idea, personally.
Having a dedicated interface for this would be better, but it would also introduce a possible breaking change for callable resolver implementors.
something like this:
if ($this->callableResolver instanceof CallableResolverInterface) {
try {
$callable = $this->callableResolver->resolve($this->middleware);
} catch (UnableToResolveCallableExceptionInterface $e) {
// Do Nothing
}
}
A better interface name is needed.
@dakujem note that there is already AdvancedCallableResolverInterface
, I am guessing CallableResolverInterface
is retained for backwards compatibility so breaking changes to it are probably not an option.
@Rarst
I am guessing CallableResolverInterface is retained for backwards compatibility so breaking changes to it are probably not an option.
This is exactly why. Ultimately in Slim 5 we should get rid of AdvancedCallableResolver
and unify the 2.
AdvancedCallableResolver
came in after the initial release of Slim 4 at which point we couldn't make changes to CallableResolver
without breaking things.
This is unfortunately technical debt that needs to be resolved in Slim 5.
I am not sure what's the reasoning for specifically RuntimeException here, but broadening it to Exception might be a good idea for resilience?
I don't think that this is correct. We need to specifically ensure that the exception thrown is because the callable could not be resolved and not do a catch-all because we have no idea what's happening downstream.
We will need a named exception like CannotResolveCallableException
or something like that.
We need to specifically ensure that the exception thrown is because the callable could not be resolved
I follow the sentiment. The current implementation neither ensures that distinction or lets fallback code be consistently reached.
My initial thought was to prioritize reaching fallback code since that would make more cases work.
Making more cases fail clearly does not look possible without introducing a new exception and having to rewrite existing interface implementers. At which point they could as well update to advanced interface.
CallableResolver
call is wrapped in a catch that expectsRuntimeException
https://github.com/slimphp/Slim/blob/8294d20e016b01ba57eb2cdb8c0f59cde502d1ab/Slim/MiddlewareDispatcher.php#L214-L222
If a resolver throws exception that doesn't extend from
RuntimeException
this halts the execution and fallback code below is never reached.Real example of the issue with third party resolver: https://github.com/PHP-DI/Slim-Bridge/issues/51
I am not sure what's the reasoning for specifically
RuntimeException
here, but broadening it toException
might be a good idea for resilience?