Remove FinalHandler, ErrorMiddlewareInterface, and Dispatch #67
Conversation
weierophinney
force-pushed
the
feature/final-handler-removal
branch
from
571013a to
b2f84db
Compare
|
I've merged #66 to develop, and rebased dev-2.0.0 and this branch from there, as it is a continuation of the features started on that PR. Ready for review! Pinging @zendframework/community-review-team @michaelmoussa and @xtreamwayz ! |
| @@ -112,6 +96,15 @@ function ($request, $response, $next) | |||
| } | |||
| ``` | |||
|
|
|||
| > ### Do not pass an altered response | |||
There was a problem hiding this comment.
This threw me off a bit. The example demonstrated passing an altered response, but then we recommend not doing that without showing how to do it the recommended way.
Perhaps, after "or returning a brand new response entirely" we add:
[...] as shown below:
function ($request, $response, $next) { $nextResponse = $next($request, $response); return $nextResponse->withAddedHeader('Cache-Control', [ 'public', 'max-age=18600', 's-maxage=18600', ]); }
?
There was a problem hiding this comment.
Note: The $updated = $response->addHeader('Cache-Control', [ line under Providing an altered response: should be ->withAddedHeader.
There was a problem hiding this comment.
$response->addHeader should be $response->withAddedHeader for all 3 examples under Providing an altered response, Providing both an altered request and response and Returning a response to complete the request.
Since this code is also in 1.3, a PR to fix this might be in place.
There was a problem hiding this comment.
Created PR #69 to address this and some more.
There was a problem hiding this comment.
@michaelmoussa Thanks for the feedback! I've updated the examples to only demonstrate manipulating the returned response, instead of the response at invocation.
There was a problem hiding this comment.
@xtreamwayz thanks for the PR; merged, and I've rebased this PR with the changes.
| @@ -163,56 +161,6 @@ And, if not calling `$next()`, returning the response instance: | |||
| return $response; | |||
| ``` | |||
|
|
|||
| ### Raising an error condition | |||
There was a problem hiding this comment.
I know error handling is covered elsewhere, but should we at least mention here that raising an error condition is as simple as throwing an exception, and then link to the documentation on error handling?
There was a problem hiding this comment.
Great feedback; I've re-instated this section, but now with an example demonstrating raising an exception from middleware.
These changes are tied together to an extent, as the `FinalHandler` was
required by `MiddlewarePipe` due to the fact that the `$next` (formerly
`$out`) argument was optional.
Accomplished here:
- Removed the `FinalHandler` implementation and its tests.
- Updated `MiddlewareInterface` to:
- Rename `$out` to `$next`
- Make `$next` required
- Fix docblocks to reflect current interface
- Updated `MiddlewarePipe`:
- Rename `$done` to `$next`
- Make `$next` required
- Remove marshaling of a `FinalHandler`, as `$next` is never null now.
- Return the results of `$next` always, without type checking. That
can be done in the `ErrorHandler` now.
- Updated both `ErrorHandler` and `NotFoundHandler` to make `$next`
required.
- Updated `ErrorHandler` to no longer check for a null `$next`; it
cannot happen now.
- Fixed any failing tests; most of these were due to missing `$next` arguments,
or obsolete assumptions.
- Removed tests that were testing features of the `FinalHandler` and/or
presence/absence of `$next`.
- Removed `MissingDelegateException` as it's no longer used.
This patch removes the Dispatch functionality, as it becomes obsolete with the `ErrorHandler` addition from 1.3, and with the pending removal of `ErrorMiddlewareInterface`. `Next` now inlines the following code: ```php $middleware = $route->handler; return $middleware($request, $response, $this); ``` This also involves two other changes to `Next`: - Removal of the `$done` property and constructor argument. - If no middleware remains in the queue, `Next` returns the passed response. - MiddlewarePipe now executes `Next()`, but then passes the result to the `$next` argument provided to it. This allows invoking the next layer, if provided; the `NoopFinalHandler` will return the response verbatim in that instance.
- Removes references to `FinalHandler` and `ErrorMiddlewareInterface` - Demonstrates piping `ErrorHandler` earlier - Documents 2.0.0 changes in the migration guide
- Remove references to ErrorMiddlewareInterface. - Remove references to FinalHandler.
weierophinney
force-pushed
the
feature/final-handler-removal
branch
from
b2f84db to
6b7e993
Compare
- No longer demonstrating updating the response provided at invocation; instead, demonstrating manipulating the response *returned*. - Demonstrate raising an exception to report an error condition.
|
👍 LGTM now @weierophinney. Sad to see |
Me too, but as it's not used anymore, no need to keep it! |
This pull request is issued against the dev-2.0.0 branch, and builds directly on the changes in #66, with the following additional changes:
FinalHandlerclass.ErrorMiddlewareInterface.Dispatchclass; since all middleware is assumed to have the same signature now, and error handling can be done in middleware such as theErrorHandler, it's redundant.MiddlewareInterfaceto make the third argument required, and rename it to$next.Next:$doneconstructor argument and property.$dispatchproperty.ErrorHandler.)MiddlewarePipe:$nextargument.$nextargument.All tests were audited to check for features no longer supported, and documentation has been updated.