zendframework / zend-expressive

PSR-15 middleware in minutes!
BSD 3-Clause "New" or "Revised" License
711 stars 197 forks source link

Router method failure gives deprecation notice when using raiseThrowables #419

Closed moderndeveloperllc closed 7 years ago

moderndeveloperllc commented 7 years ago

Not sure if this is something that can/will be worked out in the 1.0.x series.

Setup: expressive 1.0.5 stratigility 1.3.1 Procedural configuration with raiseThrowables() Using ErrorHandler-style catching - no ErrorMiddleware

If I have a request that hits Application.php:423, it returns a $next($request, $response, 405); which in Next.php will display the deprecation notice. This is the stacktrace from the point of failure:

"/blah/vendor/zendframework/zend-stratigility/src/Next.php:435",
"/blah/vendor/zendframework/zend-stratigility/src/Next.php:422",
"/blah/vendor/zendframework/zend-stratigility/src/Next.php:119",
"/blah/vendor/zendframework/zend-expressive/src/Application.php:426",

The code for 1.1.x is checking against $this->raiseThrowables. Not sure this could be backported without breaking those who are using stratigility 1.2.x.

weierophinney commented 7 years ago

This one is tricky.

My original thought is that we change the invocation of $next to instead raise an exception. However, that could be a BC break for existing users. By invoking legacy-style error middleware, technically a consumer could write error middleware specifically for handling 405 conditions:

function ($err, $request, $response, $next)
{
    if ($err !== 405) {
        return $next($request, $response, $err);
    }
    // handle the 405 condition...
}

This may or may not be useful; however, it would allow doing something like injecting a custom response body (such as Problem Details).

Raising an exception would break that use case.

Another use case that would be broken is for developers who have written legacy error middleware for general purpose error handling, instead of configuring or overriding the final handler. In such cases, again, raising an exception would bypass this middleware, breaking the application.

What we may be able to do is call swallowDeprecationNotices() immediately before invoking $next() within that block. What I'm a bit surprised at, however, is the fact that the tests are not picking up the deprecation notice. I don't see any code within RouteMiddlewareTest that performs error handling on deprecation notices, which means it should display the behavior you're indicating. I'll see if I can resolve that, which should allow me to then test a fix using swallowDeprecationNotices().

weierophinney commented 7 years ago

I don't see any code within RouteMiddlewareTest that performs error handling on deprecation notices, which means it should display the behavior you're indicating. I'll see if I can resolve that

They don't display the deprecation notice... because the $next argument in each is a closure. I'll create a new test to recreate the issue.

moderndeveloperllc commented 7 years ago

@weierophinney Thanks, I hadn't noticed the swallowDeprecationNotices() method. I imagine you will be happy when you don't have to deal with all this hybrid 1.0.x/1.3.x junk. I imagine that with http-middleware solidifying, there will be a Stratigility 1.4.x release that will free up 1.1 here or something. I look forward to testing it out!