search

zendframework / zend-expressive

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

Way back out of the onion triggers no error middleware #356

Closed cottton closed 7 years ago

cottton commented 8 years ago

Error middleware gets triggered if the script is on its way into the onion. But not on its way back/out of the onion.

Working (triggers error middleware):

middleware(
    // do some

    return next middleware(

        // do some
        // error happen -- should invoke error middleware now (and it does)

        return next middleware( 

            // do some

            return next middleware(

                // do some

                return response
            )
        )
    )
)

Not working (triggers no error middleware):

middleware(
    // do some

    return next middleware(

        // do some

        response = next middleware( 

            // do some

            return next middleware(

                // do some

                return response
            )
        )

        // error happen -- should invoke error middleware now (and it does not)

    )
)

config:

[
    'middleware' => [
        App\Error\ErrorMiddleware::class,
    ],
    'error'      => true,  
    'priority'   => PHP_INT_MAX * -1, 
],

Error middleware code:

namespace App\Error;

use Zend\Stratigility\ErrorMiddlewareInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Message\ResponseInterface;

class ErrorMiddleware implements ErrorMiddlewareInterface
{
    protected $errorAction;

    public function __construct(ErrorAction $errorAction)
    {
        $this->errorAction = $errorAction;
    }

    public function __invoke(
        $error,
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $out = null
    ) {
        // test logging
        file_put_contents('data/test.txt', __METHOD__ . PHP_EOL, FILE_APPEND);

        // some error handling ...

        if ($out !== null) {
            return $out($request, $response, $error);
        }
        return $response;
    }
}

Error middleware gets triggered from a simple Action:

into the onion (working case):

// ...
public function __invoke(
    ServerRequestInterface $request,
    ResponseInterface $response,
    callable $next = null
) { 
    // triggers an InvalidArgumentException
    // "Invalid header value; must be a string or array of strings"
    // error middleware gets triggerd
    $response = $response->withHeader('test', 1);    

    // ...
}

out of the onion (not working case):

public function __invoke(
    ServerRequestInterface $request,
    ResponseInterface $response,
    callable $next = null
) { 
    // invoke next
    $response = $next($request, $response);

    // triggers an InvalidArgumentException 
    // "Invalid header value; must be a string or array of strings"
    // error middleware gets NOT triggerd
    return $response->withHeader('test', 3);

To see if the InvalidArgumentException gets triggered i added logging at \Zend\Diactoros\MessageTrait::withHeader

// \Zend\Diactoros\MessageTrait::withHeader
public function withHeader($header, $value)
{
    if (is_string($value)) {
        $value = [$value];
    }

    if (! is_array($value) || ! $this->arrayContainsOnlyStrings($value)) {

        // test logging
        file_put_contents('data/test.txt', __METHOD__ . PHP_EOL, FILE_APPEND);

        throw new InvalidArgumentException(
            'Invalid header value; must be a string or array of strings'
        );
    }

    // rest of \Zend\Diactoros\MessageTrait::withHeader
}

Perhaps related to https://github.com/zendframework/zend-expressive/issues/355 (Cannot use multiple error middleware)

When could this happen? see https://github.com/zendframework/zend-expressive/issues/264 (inject a header into every response)

function ($request, $response, $next)
{
    $response = $next($request, $response);

    $myVal = 1;
    // ...

    // inject a header into every response
    return $response
        ->withHeader(
            'X-Clacks-Overhead', 
            $myVal  // triggers an InvalidArgumentException 
                    // but no error middleware
        );
}

EDIT: using zend expressive skeleton minimal, zend router, zend service manager, no template engine, no whoops / no default error middleware

geerteltink commented 8 years ago

To trigger an error you tried this:

    // triggers an InvalidArgumentException 
    // "Invalid header value; must be a string or array of strings"
    // error middleware gets NOT triggerd
    return $response->withHeader('test', 3);

You need to send the error back into the "onion" with the third error parameter added.

// $out($error, $response, $error)
return $out($request, $response->withStatus(404), 'Not found'); 

// Alternatively throw an exception
throw new Exception('test', 3);

Edit: Other solution is to throw an Exception.

cottton commented 8 years ago

The Exception got thrown by \Zend\Diactoros\MessageTrait::withHeader. I checked this now using a try/catch:

code:

config

[
    'middleware' => [
        App\Error\ErrorMiddleware::class,
    ],
    'error'      => true,  
    'priority'   => PHP_INT_MAX * -1, 
],

error middleware 

public function __invoke(
    $error,
    ServerRequestInterface $request,
    ResponseInterface $response,
    callable $out = null
) {
    // test logging
    file_put_contents('data/test.txt', __METHOD__ . PHP_EOL, FILE_APPEND);

    // some error handling ...

    if ($out !== null) {
        return $out($request, $response, $error);
    }
    return $response;
}

action

public function __invoke(
    ServerRequestInterface $request,
    ResponseInterface $response,
    callable $next = null
) {
    file_put_contents('data/test.txt', __METHOD__ . PHP_EOL, FILE_APPEND);
    try{
        $response = $next($request, $response);

        // triggers an InvalidArgumentException
        return $response->withHeader('test', 1);
    }
    catch (\Exception $e) {
        file_put_contents('data/test.txt', __METHOD__ . ' -- catch Exception instance of ' . get_class($e) . PHP_EOL, FILE_APPEND);
        // throw further as it would without try/catch
        throw $e;
    }

test.txt

// case one - into the onion:
App\Action\PostAction::__invoke
Zend\Diactoros\MessageTrait::withHeader -- throws InvalidArgumentException
App\Action\PostAction::__invoke -- catch Exception instance of InvalidArgumentException
App\Error\ErrorMiddleware::__invoke

// case two - back out of the onion:
App\Action\PostAction::__invoke
Zend\Diactoros\MessageTrait::withHeader -- throws InvalidArgumentException
App\Action\PostAction::__invoke -- catch Exception instance of InvalidArgumentException

case two - no error middleware invoked

EDIT: I cannot use return $out($request, $response->withStatus(404), 'Not found'); because i would not ~know that an error happen. The error happen "unexpected"

weierophinney commented 7 years ago

Error middleware is only invoked when $next is called with the third argument.

Inside Zend\Stratigility\Dispatch, if an exception is thrown by middleware it is dispatching, it will call on the next error middleware with the caught exception — which is the same as calling $next with the third argument.

I'm having trouble following your examples, but I'll try and recreate the issue, posting my findings so that you can comment.

weierophinney commented 7 years ago

Also, a quick note: I'm not sure how much effort I'll spend on this, as we're deprecating the error middleware concept (Stratigility 1.3 deprecates it, and the upcoming Expressive 1.1 uses programmatic pipelines without error middleware, in favor of middleware containing try/catch blocks around calls to $next). As 1.1 is slated for the next couple of weeks, there may not be much point in making this work.

weierophinney commented 7 years ago

I've tried to re-create this based on your "Not working (triggers no error middleware)" example from above, and have written the following unit test within test/IntegrationTest.php:

    public function testErrorMiddlewareCanTriggerOnWayOutOfOnion()
    {
        $flowMiddleware = function ($request, $response, $next) {
            $counter = $request->getAttribute('counter', 0);
            return $next(
                $request->withAttribute('counter', $counter + 1),
                $response
            );
        };

        $errorTriggeringMiddleware = function ($request, $response, $next) {
            $counter = $request->getAttribute('counter', 0);
            $response = $next($request->withAttribute('counter', $counter + 1), $response);
            return $response->withHeader('test', 1);
        };

        $notExpected = new Response();
        $innermostMiddleware = function ($request, $response, $next) use ($notExpected) {
            return $notExpected;
        };

        $triggered = false;
        $errorMiddleware = function ($error, $request, $response, $next) use (&$triggered) {
            $triggered = $error;
            return $response;
        };

        $app = new Application(new FastRouteRouter(), null, null, $this->getEmitter());
        $app->pipe($flowMiddleware);
        $app->pipe($errorTriggeringMiddleware);
        $app->pipe($flowMiddleware);
        $app->pipe($innermostMiddleware);
        $app->pipeErrorHandler($errorMiddleware);

        $request  = new ServerRequest([], [], 'https://example.com/foo', 'GET');
        $response = new Response();

        $app->run($request, $response);

        $this->assertInstanceOf(InvalidArgumentException::class, $triggered);
        $this->assertContains('Invalid header value', $triggered->getMessage());
    }

This test passes at this time. The flow should be exactly the nest of middleware you described above; the $errorTriggeringMiddleware does work, calls on $next, and then makes a method call that results in an exception. That exception is caught by Dispatch, which then triggers $errorMiddleware,raises an exception. which, in my case, simply updates the $triggered reference and returns the response.

The above demonstrates that error middleware can be triggered on the way back out the onion. My suspicion is that you are not registering the error middleware late enough. Error middleware should be registered after any regular middleware for exactly this reason.

I'm going to close this for now, as I'm unable to reproduce. Feel free to re-open or open a new issue if you can provide new information, preferable a test case that demonstrates how to reproduce the issue.