http-interop support for dev-2.0.0 #76
Conversation
Updated branch aliases
`Next` will be called via its `process()` method when invoked by http-interop middleware; as such, it can invoke a separate method on Dispatch. This patch introduces `Dispatch::process()`, which then determines how to delegate the route provided. For non-http-interop middleware, it will raise an exception if the `Dispatch` instance does not have a response prototype injected, or if the request is a non-server-side request. When dispatching http-interop middleware, if an exception is caught, it raises exceptions for the same conditions, but otherwise delegates to the next error handler.
Extracted actual handler processing into: - dispatchCallableMiddleware() - dispatchInteropMiddleware() Each now tests for the handler type received and proxies to the appropriate method.
`Next` now implements `DelegateInterface`, to allow usage with
PSR-15/http-interop middleware.
For this to work, the following changes were made:
- `Next` now optionally allows setting a response prototype. If none is
set when `__invoke()` is first called, the instance will memoize the
response.
- `setResponsePrototype()` also sets the response prototype on the
composed `Dispatch` instance.
- `process()` was added, and programmed to operate exactly as
`__invoke()`, with the following additional behaviors/behavior
changes:
- it calls `Dispatch::process()` instead of `Dispatch::__invoke()`.
- if the queue is empty:
- if no response prototype exists, an exception is thrown
- if the request is not a server request, an exception is thrown
- if one exists, that value will be passed to the `$done` handler
- if delegating to the next in the queue, it uses its own `process()`
method, and not the `__invoke()` method.
- if the middleware dispatched does not return a response, and no
response prototype is present, it raises an exception.
- internal methods typehinting on `ServerRequestInterface` were updated
to hint on `RequestInterface` instead, as they were only operating on
methods defined in that looser interface.
`Next`:
- Allows composing a `DelegateInterface` instance as the `$done`
property. Invokes this differently based on whether it is callable or
a delegate, and now only raises exceptions for missing response
prototype or invalid request type when invoking a callable.
`MiddlewarePipe`:
- Now implements http-interop `ServerMiddlewareInterface`
- Added response prototype composition; can set it, as well as test if
one is already set.
- Updated `pipe()` to allow piping http-interop middleware.
- `__invoke()` now injects its `$response` argument as the `Next`
response prototype.
- Added `process()`:
- Creates a `Next` instance using the `$delegate` provided.
- Sets the `Next` response prototype if it has one composed itself.
- Calls `Next::process()` with the provided request.
- Returns the result if it is a response, but otherwise the composed
response prototype.
`Dispatch`:
- Prior to dispatching `MiddlewarePipe` middleare, it now injects a
`MiddlewarePipe` with its own response prototype if the pipeline does
not yet compose one.
PSR-15 implementation. Conflicts: src/Dispatch.php src/MiddlewarePipe.php src/Next.php test/DispatchTest.php test/MiddlewarePipeTest.php test/NextTest.php
- `$done` verbiage is retained *within* the `__invoke()` and `process()` methods, but the property name and `dispatchDone()` methods are renamed.
…when piping when possible Backports the `CallableMiddlewareWrapper` from the dev-2.0.0 branch; `MiddlewarePipe::pipe()` will now decorate callable middleware *if a response prototype is present*; if not, it continues as normal.
If a Next instance is passed to CallableMiddlewareWrapper, do not close over it, as it is already invokable with the expected arguments.
Error middleware will be removed in 2.0.0, and will be handled specially anyways.
…ferently `pipe()` now detects if callable middleware has two arguments, with the second type-hinting on `DelegateInterface`; if so, it wraps it in a new class, `CallableInteropMiddlewareWrapper`, which simply proxies to the callable middleware.
Will not be used internally, but can be used by existing middleware in order to adapt it for http-interop.
Added extensive section on http-interop.
Since `MiddlewarePipe` now allows piping http-interop middleware, and version 2.0.0 will remove the `MiddlewareInterface`, these can be written as http-interop middleware instead.
…dler Each now implements `__invoke()`, and has it proxy to `process()` after first wrapping the `$next` argument in a `CallableDelegateDecorator`.
Primarily, to demonstrate http-interop middleware wherever possible.
Handle callable http-interop middleware, and ensure all shipped middleware implementations comply to http-interop. Conflicts: doc/book/api.md doc/book/executing-middleware.md doc/book/intro.md doc/book/migration/to-v2.md src/Middleware/CallableMiddlewareWrapper.php src/Middleware/ErrorHandler.php src/Middleware/NotFoundHandler.php src/MiddlewarePipe.php src/Next.php test/Middleware/NotFoundHandlerTest.php test/MiddlewarePipeTest.php
|
Pinging @zendframework/community-review-team , @michaelmoussa , and @xtreamwayz for review, please. |
Thanks, @michaelmoussa!
Suggested by @michaelmoussa
| */ | ||
| private function decorateCallableMiddleware(callable $middleware) | ||
| { | ||
| $r = $this->getReflectionFunction($middleware); |
There was a problem hiding this comment.
I understand the need for this in the #75 for BC, but for dev-2.0.0 could we not require a particular format for the callable middleware signature in order to avoid needing to use reflection here?
There was a problem hiding this comment.
I'd like to be able to accommodate existing callable middleware using the legacy interface, as there's a lot of it out there currently. Having this ability will allow us to continue interoperating with these legacy projects so that folks can migrate to http-interop.
Aside: As it stands, there's some indication that PSR-15 may in fact only cover server middleware, which means that some of the code may end up needing to change again anyways (e.g.,
DelegateInterfacemay end up type-hinting againstServerRequestInterface, andServerMiddlewareInterfacemay not need theServerprefix).
There was a problem hiding this comment.
Would we be able to accomplish that via a separate decorator (that we provide) which folks could use to wrap the legacy callable middleware before the MiddlewarePipe gets a hold of it? That way we can still support it, but we don't need to use / maintain the extra Reflection logic to figure out how to consume it (since the user would have already wrapped it with a consistent decorator).
Just a thought - I don't have any particularly strong feelings about Reflection in this case, but it jumped out at me so I figured I'd throw the idea out there.
There was a problem hiding this comment.
So, I've been playing with this on my website codebase, to see what works and what doesn't.
Interestingly, I only need to make three changes to Zend\Expressive\Application at this point to make it work with the Stratigility v2 codebase, and everything I've already written continues to work. As in: no need to update any existing middleware at all. It "just works". I think the bigger takeaway with that, though is: should we emit a deprecation warning in v2 for these? :)
As it is, this patch (and the one in #75) already provide the decorator you mention. However, it's painful to use, as you also need to retain a ResponseInterface instance for it:
$app->pipe(new CallableMiddlewareWrapper($middleware, $response));Because it's the second argument, if you're doing things inline, it becomes easy to forget:
$app->pipe(new CallableMiddlewareWrapper(function ($req, $res, $next) {
/* ... */
}, $response));Internally, this is what pipe() is doing now, based on reflection. If performance of reflection becomes an issue, developers can decorate manually.
My feeling at this point is that http-interop is likely to change in one way or another, which may necessitate changes in Stratigility as well: interfaces and signatures may change, which would require additional rewrites later. If we can allow developers to defer updating their signatures until http-interop stabilizes and later gets ratified as PSR-15, I'd like to support that.
There was a problem hiding this comment.
Good to hear that with only some changes Expressive can be made compatible.
I'm wondering about the Reflection performance. Are we talking about only a few microseconds extra or a lot more?
There was a problem hiding this comment.
Microseconds or fractions thereof. And all losses there are made up within Next, as the logic is simpler and more performant, as the patch removes the extra Dispatch layer (as all middleware is compatible with http-interop in this 2.0-facing patch).
Forward port fixes due to feedback from @michaelmoussa on zendframework#75 Conflicts: src/Dispatch.php
| */ | ||
| public function process(ServerRequestInterface $request, DelegateInterface $delegate) | ||
| { | ||
| return $middleware($request, $delegate); |
There was a problem hiding this comment.
Missing $this:
return $this->middleware($request, $delegate);
…aled by test Updated `process()` to assign local `$middleware` variable to `$middleware` property value.
Ensured all behaviors work as expected.
Forward port updates from zendframework#75 Conflicts: src/Dispatch.php src/Route.php
These needed to be passed to ReflectionMethod, with each element of the array as discrete arguments; previously, they were passed to ReflectionFunction, which failed as it cannot handle arbitrary callables.
Forward port fixes for array callables as middleware.
Will likely set the response prototype during instantation in Expressive, and then use that as the default response if none is provided to `run()`; as such, it cannot be private visibility.
Forward port `$responsePrototype` visibility from zendframework#75
|
As a point of reference, locally I updated zend-expressive as follows:
With those minimal changes, I tested against my own website code. The only code I needed to change internally were several places where I was calling One caveat: I'd already updated my code to essentially prototype the new error handling paradigm from Stratigility 1.3/2.0. As such, the absence of a final handler did not affect my code, but it will affect other users. As such, I'd likely recommend updating to Stratigility 1.3 for the Expressive 1.1 release, while updating the Expressive skeleton to use the new error handling facilities for its next minor release. This would allow users to gradually update their code to remove reliance on the |
Since it affects others without making changes, wouldn't that qualify as a BC break? Maybe it's better to create a Expressive 2.0 release? I'm pretty sure some users will just run For the skeleton a major or minor release doesn't matter since, in theory, you can never cause a BC break in that package. |
I've observed the following with composer. First, let's set some conditions:
Further, we'll assume that you've already run Now, let's say version 1.2 of foo now depends on bar 2.0, and you run
In the first case, you would need to run In the second case, you can do one of:
I could be completely wrong about this, but IIRC, this was the precise problem we ran into that led to us creating the upgrade script in zf-apigility-admin, which removes both the All that said, I'm going to test this theory out before setting a concrete roadmap for updating Expressive. |
|
I've just done the following to test my theory about using
This seemed to verify my assertion. However, it is not exactly the scenario we'd have with Expressive and Stratigility, so I decided to try something else. I created two packages:
I then created a project that depends on "test/foo". To begin, I tagged both packages as 1.0.0, and had "test/foo" depend on "test/bar" Next, I tagged "test/bar" as 2.0.0. Running I then updated "test/foo" to depend on "test/bar" at When I returned to the project and ran As such, I need to reevaluate how we will start folding these features into Expressive. I have a few ideas, but need to prototype them. I'll outline these below to give some ideas, but they should not affect how and when this particular patch for Stratigility is evaluated, merged, and eventually tagged. (I'll copy this information to a gist or a project card soon as well.) Register error middleware by defaultThe first option is to simply register the various error middleware by default. We could override the The main problem with the above is that doing so makes this less configurable; we cannot have an alternate response generator if we hard-code creation of one into the To solve that problem, we can handle the error handler aspect in the $application->pipe(OriginalMessages::class);
$application->pipe($container->get(ErrorHandler::class);Notice the second The problem with this approach is ensuring that the $errorHandler = $container->has(ErrorHandler::class)
? $container->get(ErrorHandler::class)
: (new ErrorHandlerFactory())($container, ErrorHandler::class);(Will need to check if the above is valid in PHP 5.6.) My take is that this approach is likely the most backwards compatible. Delegator factoriesOn the subject of delegator factories, we could also implement the new functionality completely via delegator factories, instead of updating the Essentially, the
The problem with this approach is that users then need to register those delegator factories within their existing applications in order to ensure the functionality is properly registered. This essentially makes the approach, while technically correct, a no-go for existing users. |
This patch incorporates #75, providing comprehensive http-interop support for version 2.0.
In particular:
MiddlewarePipe:MiddlewareInterface, though it retains its__invoke()method.$outargument to__invoke()was renamed to$delegate, removes its typehint, and is no longer optional. The argument may be a callable, in which case it is assumed to have the signaturefunction ($request, $response); it may also be an http-interopDelegateInterfaceimplementation.Zend\Stratigility\Middleware\CallableMiddlewareWrapperFactoryinstance to be composed in order to decorate piped callable middleware using the legacy signature; piping such middleware without one of these being present now raises an exception.Next:$doneconstructor argument to$nextDelegate, makes it optional, and now typehints against the http-interopDelegateInterface.$responseand$errarguments from__invoke()entirely; the method now proxies toprocess().Dispatchis removed completely.