-
Notifications
You must be signed in to change notification settings - Fork 197
When a middleware instantiation leads to a Interop\Container\Exception\ContainerException
, the exception is silenced, and execution continues
#416
Comments
Interop\Container\Exception\ContainerException
, the exception is silenced, and execution continues
Turns out that this is a bug caused by the stratigility Specifically, stratigility will simply catch any This makes sense if I'm unsure if this is expressive configuring the middleware pipeline incorrectly, or if stratiglity is wrong upfront. Here's an expressive application showing the issue in practice: <?php
use Zend\Expressive\Application;
use Zend\Expressive\Container\ApplicationFactory;
use Zend\ServiceManager\ServiceManager;
require_once __DIR__ . '/../vendor/autoload.php';
$config = [
'debug' => false,
'dependencies' => [
'factories' => [
Application::class => ApplicationFactory::class,
'failing-middleware' => function () {
throw new \Exception('This will fail at instantiation');
},
'index' => function () {
die('this middleware should NEVER be reached');
},
],
],
'middleware_pipeline' => [
'foo' => [
'middleware' => [
'failing-middleware',
],
'priority' => 100,
],
'routing' => [
'middleware' => [
ApplicationFactory::ROUTING_MIDDLEWARE,
ApplicationFactory::DISPATCH_MIDDLEWARE,
],
'priority' => 1,
],
'error' => [
'middleware' => [
],
'error' => true,
'priority' => -10000,
],
],
'routes' => [
[
'name' => 'home',
'path' => '/',
'middleware' => 'index',
'allowed_methods' => ['GET'],
],
],
];
$config['dependencies']['services']['config'] = $config;
(new ServiceManager($config['dependencies']))->get(Application::class)->run(); Note: adding an error middleware doesn't seem to make any difference |
As discussed with @xtreamwayz, these are the versions involved in this bug:
Some wild guesses:
|
Going to Stratigility 1.3 can cause issues if you are using a config-driven setup vs. a procedural approach. Config-driven
|
Indeed, hijacking In my opinion, the entire idea of the error middleware being piped with all the others is kinda useless: probably to be deprecated in favor of just a |
This is the direction we're moving for 1.1, actually, though without using request attributes. What will happen instead is that the Error conditions will typically just be exceptions: function ($request, $response, $next)
{
throw new RuntimeException('Oh, no, I cannot handle this!');
} You'll then have middleware in an outer layer that handles exceptions. We will ship a default implementation, but you can also use your own, or stack multiple such middleware each capable of handling specific exceptions: function ($request, $response, $next)
{
try {
$response = $next($request, $response);
return $response;
} catch (UnauthorizedException $e) {
} catch (Throwable $e) {
throw $e;
}
// Handle UnauthorizedException; maybe display a login form?
// ...
return $someNewResponse;
} Stratigility's "error middleware" concept from 1.0 is deprecated starting in 1.3, and will be removed entirely in 2.0 in favor of this concept. Regarding your specific error, I was able to reproduce it. For those that wonder, the results of executing the above application are:
What SHOULD happen is that the Thanks for the detailed report, @Ocramius! |
Found the issue, and it's in Stratigility. The crux of the issue is that internally, Expressive's The solution will likely be to test in if ($route->handler instanceof ServerMiddlewareInterface
&& ! $route->handler instanceof MiddlewarePipe
) {
return $this->dispatchInteropMiddleware(/* ... */);
} I'll open an issue on Stratigility, and, once fixed, we'll do another 1.0.X release of Expressive that requires that version or above (or versions prior to the 1.3 series). |
Confirmed: the change I suggested does work. I'll get a test case written for Stratigility, and submit a patch there. |
…esent As reported in zendframework/zend-expressive#416, error middleware nested inside a `MiddlewarePipe` was not being dispatched. This was due to the fact that `Dispatch` was identifying the pipeline as http-interop middleware, and thus dropping the `$err` argument (as interop middleware cannot accept that argument). Theis patch updates `Dispatch` to check if the middleware is a `MiddlewarePipe` and a non-null `$err` is present; if so, it now dispatches it as callable middleware instead of as interop middleware.
@weierophinney will this be fixed by zendframework/zend-stratigility#95? Specifically: does the example application above crash correctly when run against that patch? (sorry, not on a dev environment atm) |
@Ocramius yes, application crashes correctly when run against that patch; I've tested against your example both with and without the patch, and the patch makes it behave correctly. |
Fixed in https://github.com/zendframework/zend-stratigility/releases/tag/1.3.2 via zendframework/zend-stratigility#95. Closing as |
Test case will come, but defining a middleware pipeline config with invalid string services as middleware, or strings pointing to factories that throw exceptions, expressive simply ignores those exceptions and keeps going.
Test case will come, but basically, I'd expect:
Expressive did:
Also: sorry for the half-submitted initial issue: switching from windows to mac to windows to mac is messing with my ability to use a keyboard.
The text was updated successfully, but these errors were encountered: