Provide http-interop compatibility #75

weierophinney · 2016-11-02T20:08:13Z

This patch provides compatibility with http-interop 0.2.0, via the following features:

MiddlewarePipe:
- now implements ServerMiddlewareInterface.
- now allows piping http-interop ServerMiddlewareInterface and MiddlewareInterface instances.
- now allows piping callable middleware with the signature:
```
function ($request, DelegateInterface $delegate) : ResponseInterface
```
- Now exposes a setResponsePrototype() method; when set, callable middleware implementing the legacy interface will be decorated as ServerMiddlewareInterface instances.
Next:
- Now implements DelegateInterface.
Dispatch:
- Now provides a process() method mimicing the DelegateInterface; this method is used by Next when dispatching ServerMiddlewareInterface instances.
- Now identifies http-interop middleware and dispatches them properly.

Additionally, this patch provides decorator classes for decorating legacy callable middleware, callable middleware compatible with ServerMiddlewareInterface, and callable delegates. This latter feature can be used to adapt existing callable middleware classes to implement ServerMiddlewareInterface:

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Psr\Http\Middleware\DelegateInterface;
use Psr\Http\Middleware\ServerMiddlewareInterface;
use Zend\Diactoros\Response\JsonResponse;
use Zend\Stratigility\Delegate\CallableDelegateDecorator;

class PingMiddleware implements ServerMiddlewareInterface
{
    public function __invoke(
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) {
        return $this->process($request, new CallableDelegateDecorator($next, $response));
    }

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        return new JsonResponse(['ack' => time()]);
    }
}

Finally, the patch provides migration documentation to both v1.3 and v2.0. Another patch will be submitted shortly against the dev-2.0.0 branch that incorporates the patches from this branch, with updates to provide the final 2.0.0 functionality around http-interop compatibility.

`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.

…esent

`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.

- `$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.

…g a Delegate

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.

weierophinney · 2016-11-02T20:22:40Z

Pinging @zendframework/community-review-team , @michaelmoussa , and @xtreamwayz for review, please.

michaelmoussa

Had a few comments but overall LGTM.

michaelmoussa · 2016-11-02T20:36:22Z

doc/book/api.md

+>
+> Using the `$response` argument is unsafe when using delegation, as an inner
+> layer could return an entirely different response, ignoring any changes you
+> may have introduced previously. Additionally, hen manipulating the response


ITYM "Additionally, when" unless poultry. :)

Thanks - fixed locally, pushing shortly.

michaelmoussa · 2016-11-02T20:49:39Z

src/Dispatch.php

+     */
+    private function isNotInteropMiddleware($handler, RequestInterface $request)
+    {
+        if ($handler instanceof ServerMiddlewareInterface || $handler instanceof InteropMiddlewareInterface) {


Considered this for readability?

if ($this->isInteropMiddleware($handler) { return false; }

Agreed - nice suggestion!

michaelmoussa · 2016-11-02T20:59:48Z

src/Dispatch.php

+            );
+        }
+
+        printf("[%s] %s (%d)\n", get_class($throwable), $throwable->getMessage(), $throwable->getCode());


Would it not be better to record this as a user warning / error (perhaps using error_log(...) rather than outputting it? Seems like an unlikely case, but in the event that it's hit, it could leak something sensitive as as plain output.

oof -- that was debug code. I'm a little surprised I missed it, which means that area is likely not tested. Removing now.

Sounds like an invitation to bring the 100% test coverage hammer down in this repository too, muahahahaha! :)

@michaelmoussa

Thanks, @michaelmoussa!

@michaelmoussa

Suggested by @michaelmoussa

@michaelmoussa

Forward port fixes due to feedback from @michaelmoussa on zendframework#75 Conflicts: src/Dispatch.php

geerteltink

Just the CallableInteropMiddlewareWrapper needs a fix and probably some testing. But overall LGTM.

geerteltink · 2016-11-03T07:02:07Z

doc/book/migration/to-v2.md

+
+Starting in version 2.0.0, `MiddlewarePipe` *will no longer implement
+`Zend\Stratigility\MiddlewareInterface`, and only implement the http-interop
+`ServerMiddlewareInterface`*. This has several repurcussions.


repercussions ?

geerteltink · 2016-11-03T07:09:58Z

src/Delegate/CallableDelegateDecorator.php

+    /**
+     * Proxies to the underlying callable delegate to process a request.
+     *
+     * {@inheritDocs}


Should be {@inheritDoc}.

geerteltink · 2016-11-03T07:14:51Z

src/Dispatch.php

@@ -57,37 +65,213 @@ public function __invoke(
        ResponseInterface $response,
        callable $next
    ) {
-        $handler  = $route->handler;
+        if (! $this->responsePrototype) {


Missing method level docblock:

@return ResponseInterface @throws Exception\InvalidRequestTypeException @throws Exception\MissingResponsePrototypeException

In this particular case, neither of those two exceptions can occur:

Since were passing the $request provided to __invoke(), InvalidRequestTypeException will not be raised, as we have a server-side request.

Since we received a response, the response prototype will be present, so MissingResponseProtototypeException will not be thrown, either.

I will add the missing @return annotation, however!

Also, @throws only makes sense when the method itself throws that exception, not some dependency.

geerteltink · 2016-11-03T07:16:54Z

src/Middleware/CallableInteropMiddlewareWrapper.php

+     */
+    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
+    {
+        return $middleware($request, $delegate);


return $this->middleware($request, $delegate);

Probably lacks testing as well.

geerteltink · 2016-11-03T07:22:03Z

src/Middleware/ErrorHandler.php

 use Zend\Stratigility\Exception\MissingDelegateException;
 use Zend\Stratigility\Exception\MissingResponseException;
-use Zend\Stratigility\MiddlewareInterface;
 use Zend\Stratigility\Utils;


Unused imports

use Zend\Escaper\Escaper; use Zend\Stratigility\Exception\MissingDelegateException; use Zend\Stratigility\Utils;

geerteltink · 2016-11-03T07:36:55Z

src/Route.php

     */
-    public function __construct($path, callable $handler)
+    public function __construct($path, $handler)


I notice an inconsistent use of @throws in dockblocks so I'm not sure if they are required or not.
e.g. in this class the constructor lacks @throws in its dockblock whereas __get() has it.

Per @xtreamwayz

- Per @xtreamwayz

…aled by test Updated `process()` to assign local `$middleware` variable to `$middleware` property value.

- per @xtreamwayz

Ensured all behaviors work as expected.

Forward port updates from zendframework#75 Conflicts: src/Dispatch.php src/Route.php

weierophinney · 2016-11-03T13:34:16Z

@xtreamwayz I've addressed all your points of feedback; thanks! Additionally, wrote tests for the CallableMiddlewareWrapper as well as the CallableInteropMiddlewareWrapper to ensure behavior is as expected.

geerteltink · 2016-11-03T13:36:29Z

LGTM.

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.

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

Close #75

Merge develop, including #75 changes.

weierophinney added 20 commits October 24, 2016 16:32

Use colors always with QA scripts

7c5c635

Added http-interop/http-middleware as a dependency

386bd9e

Updated Dispatch to use Prophecy and remove $phpunit binding

1b20899

Ensure __invoke/process can each handle both types of middleware

6c8b317

Extracted actual handler processing into: - dispatchCallableMiddleware() - dispatchInteropMiddleware() Each now tests for the handler type received and proxies to the appropriate method.

Memoize first response passed to __invoke if no response prototype pr…

e0ff957

…esent

Rename $done to $nextDelegate

2ff5988

- `$done` verbiage is retained *within* the `__invoke()` and `process()` methods, but the property name and `dispatchDone()` methods are renamed.

Do not close over Next instances

9c47415

If a Next instance is passed to CallableMiddlewareWrapper, do not close over it, as it is already invokable with the expected arguments.

Do not wrap error middleware

4b9f140

Error middleware will be removed in 2.0.0, and will be handled specially anyways.

Backported CallableDelegateDecorator to 1.3-dev branch

947c953

Will not be used internally, but can be used by existing middleware in order to adapt it for http-interop.

responsePrototype is not required to decorate middleware if decoratin…

db08218

…g a Delegate

Documented migration for 1.3/2.0

685be3d

Completed migration guide for 1.3/2.0

a1d27eb

Added extensive section on http-interop.

Update ErrorHandler and NotFoundHandler to implement http-interop

3353a63

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.

Provide callable implementation for both ErrorHandler and NotFoundHan…

64aaef5

…dler Each now implements `__invoke()`, and has it proxy to `process()` after first wrapping the `$next` argument in a `CallableDelegateDecorator`.

Update documentation for 1.3.0 features

6d186ad

Primarily, to demonstrate http-interop middleware wherever possible.

weierophinney added the enhancement label Nov 2, 2016

weierophinney added this to the 1.3.0 milestone Nov 2, 2016

weierophinney mentioned this pull request Nov 2, 2016

http-interop support for dev-2.0.0 #76

Merged

michaelmoussa reviewed Nov 2, 2016

View reviewed changes

weierophinney added 3 commits November 2, 2016 16:05

Fixed typo

aeaf5ae

Thanks, @michaelmoussa!

Use existing method for conditional

70386f8

Suggested by @michaelmoussa

Removed debug code.

5cc396b

weierophinney added a commit to weierophinney/zend-stratigility that referenced this pull request Nov 2, 2016

Merge branch 'feature/psr-15-compat' into feature/http-interop-v2

6a94376

Forward port fixes due to feedback from @michaelmoussa on zendframework#75 Conflicts: src/Dispatch.php

geerteltink reviewed Nov 3, 2016

View reviewed changes

weierophinney added 7 commits November 3, 2016 08:07

Fix typo in migration doc

15e7a28

Per @xtreamwayz

s/inheritDocs/inheritDoc/

3e6d574

- Per @xtreamwayz

Added missing return annotation in Dispatch

97c9d51

Added test for CallableInteropMiddlewareWrapper, and fixed issue reve…

0d44ec6

…aled by test Updated `process()` to assign local `$middleware` variable to `$middleware` property value.

Remove unused imports

858df04

- per @xtreamwayz

Added missing @throws annotation

ba9c301

- per @xtreamwayz

Added tests for CallableMiddlewareWrapper

e831a70

Ensured all behaviors work as expected.

weierophinney added a commit to weierophinney/zend-stratigility that referenced this pull request Nov 3, 2016

Merge branch 'feature/psr-15-compat' into feature/http-interop-v2

9ddaa53

Forward port updates from zendframework#75 Conflicts: src/Dispatch.php src/Route.php

weierophinney added 2 commits November 3, 2016 09:20

Discovered an error with array callables

296810a

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.

Make $responsePrototype protected

3f6df04

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.

weierophinney added a commit to weierophinney/zend-stratigility that referenced this pull request Nov 3, 2016

Merge branch 'feature/psr-15-compat' into feature/http-interop-v2

cddbdf5

Forward port `$responsePrototype` visibility from zendframework#75

DASPRiD approved these changes Nov 3, 2016

View reviewed changes

weierophinney merged commit 3f6df04 into zendframework:develop Nov 7, 2016

weierophinney added a commit that referenced this pull request Nov 7, 2016

Added CHANGELOG entries for #75

f558983

weierophinney added a commit that referenced this pull request Nov 7, 2016

Merge branch 'feature/psr-15-compat' into develop

23cb80a

Close #75

weierophinney deleted the feature/psr-15-compat branch November 7, 2016 19:32

weierophinney added a commit that referenced this pull request Nov 7, 2016

Merge branch 'develop' into feature/http-interop-v2

3386522

Merge develop, including #75 changes.

geerteltink mentioned this pull request Nov 7, 2016

Remove unused imports #72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide http-interop compatibility #75

Provide http-interop compatibility #75

weierophinney commented Nov 2, 2016

weierophinney commented Nov 2, 2016

michaelmoussa left a comment

michaelmoussa Nov 2, 2016

weierophinney Nov 2, 2016

michaelmoussa Nov 2, 2016

weierophinney Nov 2, 2016

michaelmoussa Nov 2, 2016

weierophinney Nov 2, 2016

michaelmoussa Nov 2, 2016

geerteltink left a comment

geerteltink Nov 3, 2016

geerteltink Nov 3, 2016

geerteltink Nov 3, 2016

weierophinney Nov 3, 2016

DASPRiD Nov 3, 2016

geerteltink Nov 3, 2016

geerteltink Nov 3, 2016

geerteltink Nov 3, 2016

weierophinney commented Nov 3, 2016

geerteltink commented Nov 3, 2016

Provide http-interop compatibility #75

Provide http-interop compatibility #75

Conversation

weierophinney commented Nov 2, 2016

weierophinney commented Nov 2, 2016

michaelmoussa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geerteltink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Nov 3, 2016

geerteltink commented Nov 3, 2016