Resolving middleware breaks if resolver throws unexpected exception type

[Rarst]

CallableResolver call is wrapped in a catch that expects RuntimeException

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 to Exception might be a good idea for resilience?

[dakujem]

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.

[Rarst]

@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.

[l0gicgate]

@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.

[Rarst]

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.