Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Nested error middleware does not work correctly #277

Merged
merged 5 commits into from
Jan 20, 2016

Conversation

weierophinney
Copy link
Member

Given the following middleware specification:

[
    'middleware' => [
        Unauthorized::class,
        Unauthenticated::class,
    ],
    'error' => true,
    'priority' => -10000,
]

Neither middleware listed triggers. Each will trigger, however, if you do them separately, not as part of a list:

[
    'middleware' => Unauthorized::class,
    'error' => true,
    'priority' => -10000,
],
[
    'middleware' => Unauthenticated::class,
    'error' => true,
    'priority' => -10000,
],

The error is because the MarshalMiddlewareTrait is not retaining the error flag when marshaling lazy middleware services. composes a MiddlewarePipe instance when creating an error middleware pipeline, but that class does not implement the error middleware signature.

This patch creates a Zend\Expressive\ErrorMiddlewarePipe implementation that consumes a MiddlewarePipe instance while implementing the error middleware signature. MarshalMiddlewareTrait is updated to pass the generated MiddlewarePipe instance to ErrorMiddlewarePipe when generating an error pipeline.

@weierophinney
Copy link
Member Author

I've found the issue.

When we get an array of middleware, we create a MiddlewarePipe, and pipe each item in the array to it. This is correctly creating lazy wrappers for the error middleware; that was an incorrect diagnosis.

The real problem is the MiddlewarePipe; these do not have the error middleware signature! The solution, then, is to introduce an ErrorMiddlewarePipe that has the correct signature.

@nesl247
Copy link

nesl247 commented Jan 19, 2016

So glad to have found this issue. I've just spent the last 2 hours trying to figure out why my error middleware isn't being executed.

@weierophinney
Copy link
Member Author

@nesl247 You can do as I did in the summary, and split each out into its own spec; that works perfectly. I'll get the proposed ErrorMiddlewarePipe in place likely tomorrow.

@nesl247
Copy link

nesl247 commented Jan 19, 2016

I'm doing it as mentioned until the bugfix is in place. Thanks so much. Glad it wasn't me that was the problem here.

When we create a middleware pipeline and denote it as being error
middleware, the pipeline itself must have the error middleware
signature.

Additionally, the middleware piped into that error pipeline must each
have the error middleware signature.
This patch implements an `ErrorMiddlewarePipe`. Unfortunately,
`MiddlewarePipe` implements `MiddlewareInterface`, which makes it impossible
to extend it and re-purpose it as error middleware. As such, this
implementation wraps the `MiddlewarePipe` instance, and consumes its
internal pipeline in order to dispatch a pipeline of error middleware.
This adds a test case for the ErrorMiddlewarePipe to verify that it:

- aggregates a middleware pipe instance
- dispatches the middleware composed in the middleware pipe instance
- but only those that are error middleware
Currently, the `Zend\Stratigility\Next` implementation requires that the
request and response passed to it are the Stratigility-specific
decorators. As such, the `ErrorMiddlewarePipe` is required to decorate
the incoming request and response. I chose to use reflection here so as
not to duplicate logic; ideally, we can remove that requirement from the
Stratigility library in the future.
@ezimuel ezimuel merged commit 59c76c9 into zendframework:master Jan 20, 2016
@weierophinney weierophinney deleted the hotfix/277 branch January 20, 2016 18:02
weierophinney added a commit that referenced this pull request Jan 20, 2016
weierophinney added a commit that referenced this pull request Jan 20, 2016
weierophinney added a commit that referenced this pull request Jan 20, 2016
@weierophinney
Copy link
Member Author

On testing this in a real-world application, I'm finding it's still not working. Essentially, even though error middleware is being created, when $next() is being invoked with an $error argument, these are never being hit. I've worked up a full integration test that demonstrates the expected behavior, and am now turning to isolating the root cause.

@weierophinney
Copy link
Member Author

#278 resolved the issue I was seeing, and I can report that those changes definitely allow middleware pipelines representing error middleware to work! New RC coming tomorrow!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants