Refactor pipe

[snapshotpl]

My proposition to refactor pipe method. Split to separate methods but with strict and simple usage - improve readability and predictability.

[snapshotpl]

@weierophinney My comment hide after commit so I past it again:

You have right. However IMO it drop readability after usage this method. Look at https://docs.zendframework.com/zend-stratigility/middleware/ . If path will be second parameter it will be hard to determine path or which middleware has defined path. With two separate method it's cleaner

[weierophinney]

If path will be second parameter it will be hard to determine path or which middleware has defined path. With two separate method it's cleaner

Then the appropriate solution would be:

public function pipe(MiddlewareInterface $middleware) : void;
public function pipeForPath(string $path, MiddlewareInterface $middleware) : void

Perhaps you could do a poll on the forums to see which solution users would prefer? I could tweet it from the zfdevteam twitter account for reach.

[snapshotpl]

Good compromise, but my code make less mess (look at changed tests) ;)

[danizord]

I guess that's my chance to suggest utility functions again? 😛

use function Zend\Stratigility\path;

$pipeline->pipe($middleware);
$pipeline->pipe(path('/foo', $middleware));

[snapshotpl]

@danizord How path method will be works? As middleware decorator?

[danizord]

@snapshotpl yep, that function would be just a syntax sugar on top of:

$pipeline->pipe(new PathMiddlewareDecorator('/foo', $middleware));

[weierophinney]

I like the suggestion @danizord makes here; it would allow us to remove the $path argument entirely, and move that functionality into middleware. Users could either decorate manually:

$app->pipe(new PathMiddlewareDecorator('/foo', $middleware, $container));

or do it with a utility function. Another option would be a closure exported from the application:

public function createPathMiddlewareDecorator()
{
    $container = $this->container;
    return function (string $path, $middleware) {
        if (! $middleware instanceof MiddlewareInterface) {
            $middleware = $this->prepareMiddleware($middleware);
        }
        return new PathMiddlewareDecorator($path, $middleware);
    }
}

Which would then operate like this:

$path = $app->createPathMiddlewareDecorator();

$app->pipe($path('/foo', SomeMiddlewareName::class));

Thoughts?

[danizord]

@weierophinney I like the createPathMiddlewareDecorator() idea, but it would be specific to Zend\Expressive since it knows the container. 🤔

[weierophinney]

@danizord Then I'd suggest:

• We add the PathMiddlewareDecorator, which would accept a string path and MiddlewareInterface middleware, and only execute the middleware if the beginning of the request path matches the path composed.
• We add the method I suggest above only to Expressive, and do not have it here; we would add the utility function you suggested earlier instead. Expressive users could use either, but the closure created by Expressive would offer lazy-loading of middleware.

[danizord]

@weierophinney Looks perfect for me 👌

[snapshotpl]

Looks perfect. I this case we remove "dispatching" functionality from Stratigility (Next class) and leave Stratigility pure PSR-15 implementation (as middleware dispatcher).

[weierophinney]

Exactly!

On Dec 14, 2017 5:25 PM, "Witold Wasiczko" notifications@github.com wrote:

Looks perfect. I this case we remove "dispatching" functionality from Stratigility (Next class) and leave Stratigility pure PSR-15 implementation (as middleware dispatcher).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-stratigility/pull/134#issuecomment-351867127, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlV-mBh7VCDyytbWte0pz4X9z7rOc0ks5tAa6CgaJpZM4RCuxO .

[snapshotpl]

@weierophinney @danizord I made prototyped changes we have discussed. For me it looks awesome.

[danizord]

@snapshotpl @weierophinney now that RequestHandlerInterface and MiddlewareInterface defines different method names and we don't have this dispatching logic anymore, I think MiddlewarePipe can implement both interfaces.

Here's an example implementation of this approach: https://github.com/danizord/mid/blob/0.2/src/MiddlewarePipeline.php

[weierophinney]

I think MiddlewarePipe can implement both interfaces.

I like this idea; it would mean that zend-diactoros's Server could implement RequestHandlerInterface and accept another RequestHandlerInterface to which it would delegate. The only difference I might make with regards to your approach is to perhaps have a MiddlewarePipe compose a RequestHandlerInterface as well; it could then pass that to Next, which would invoke that if the middleware pipeline is exhausted.

Thoughts?

[danizord]

@weierophinney Mhmm I can't see the use case for that 🤔 . If that's the desired behavior, you can call process() instead and pass the request handler as argument. Otherwise if you call handle() it would throw an exception if the pipeline is exhausted.

[weierophinney]

@danizord —

if that's the desired behavior, you can call process() instead and pass the request handler as argument. Otherwise if you call handle() it will throw an exception if the pipeline is exhausted.

Let's take an example from Node. If you build an HTTP server in Node, you pass it the application, and the application is responsible for writing to the response:

let server = new http.Server();
server.listen(app);

Internally, the http.Server instance creates the incoming request and a prototype response, and passes them to the app instance.

This is similar in Rack:

require 'rack'
Rack::Handler::WEBrick.run app

And with WSGI:

from wsgiref.simple_server import make_server
httpd = make_server(hostname, hostport, application)
httpd.serve_forever()

The Server::listen() method in Diactoros was modelled after these implementations, but it needs to evolve to address the emerging standards. Based on your suggestion that MiddlewarePipe implement RequestHandlerInterface, I'm suggesting that Server be a MiddlewareInterface instance. Essentially, a "server" then looks something like:

$request = ServerRequestFactory::fromGlobals(); // or from the async server...
$response = $server->handle($request, $application);
$emitter->emit($response);

However, for that to be reliable, we'd need to have some guarantee that the middleware pipeline will return a response; i.e., MiddlewarePipe::handle() must have some way of returning a response if the middleware pipeline is exhausted without producing a response.

Alternately, we ditch the idea of MiddlewarePipe implementing RequestHandlerInterface, and potentially ditch Zend\Stratigility\Server entirely. The workflow would then look something like this:

$request = ServerRequestFactory::fromGlobals(); // or from the async server...
$finalHandler = new class implements RequestHandlerInterface {
    public function handle(ServerRequestInterface $request) : ResponseInterface
    {
        return new EmptyResponse(500);
    }
};
$response = $app->process($request, $finalHandler);
$emitter->emit($response);

This is quite similar to what the Server class currently does, but more explicit; it also more clearly demonstrates the role of middleware vs request handlers at the application level.

Thoughts?

[weierophinney]

If that's the desired behavior, you can call process() instead and pass the request handler as argument. Otherwise if you call handle() it would throw an exception if the pipeline is exhausted.

I've pulled this branch from @snapshotpl locally, and started playing with it. The problem is: what code throws the exception?

Right now, as implemented, Next expects a request handler to its constructor; this is a fallback handler for when the queue is exhausted.

If MiddlewarePipe implements RequestHandlerInterface, my first inclination was to have it essentially return $this->process($request, $this). The problem is that this then becomes recursive; if Next::handle() discovers the queue is exhausted, it's then calling MiddlewarePipe::handle(), which starts the whole process all over again.

The linked suggestion you made also doesn't work: what happens when the queue is exhausted? We're now back to the question of where is the exception raised?

For that, I suggest one of two possibilities:

1. We compose a fallback handler in MiddlewarePipe. That handler is used when handle() is called: return $this->process($request, $this->fallbackHandler).
2. We create an anonymous class implementation of RequestHandlerInterface that raises an exception when called. This is then passed to process(): return $this->process($request, $anonymousHandlerImplementation).

And, of course, the third possibility is that we do not implement RequestHandlerInterface in MiddlewarePipe at all.

[danizord]

@weierophinney

The linked suggestion you made also doesn't work: what happens when the queue is exhausted?

Did you check https://github.com/danizord/mid/blob/0.2/src/MiddlewarePipeline.php#L37-L39?

If you call $pipeline->process($request, $handler);, it converts the passed $handler into a middleware and adds it to the end of pipeline. In this case you can make sure that this pipeline will always return a response.

If you call $pipeline->handle($request), it calls every middleware until a response is returned directly. If the queue is exhausted without a response, it throws a RuntimeException.

However, for that to be reliable, we'd need to have some guarantee that the middleware pipeline will return a response; i.e., MiddlewarePipe::handle() must have some way of returning a response if the middleware pipeline is exhausted without producing a response.

Why? I think it's out of scope for the Server. It should just rely on the contract of passed RequestHandlerInteface. The exhausted pipeline case should be handler by the framework, for instance Zend\Expressive\Application::handle($request) would always call $this->pipeline->process($request, $this->notFoundHandler); internally.

Or I'm missing something?

[weierophinney]

Why? I think it's out of scope for the Server. It should just rely on the contract of passed RequestHandlerInteface. The exhausted pipeline case should be handler by the framework, for instance Zend\Expressive\Application::handle($request) would always call $this->pipeline->process($request, $this->notFoundHandler); internally.

Or I'm missing something?

Zend\Stratigility\Server::listen() should always produce and/or emit a response, period. That's the part you're missing. If an exception happens within the handler provided to it, it needs to be handled in such a way that a response is produced. The question is: do we want to be able to pass a MiddlewarePipe directly to a Server instance and have an expectation that it will "just work"? The answer to that determines whether raising an exception within MiddlewarePipe or one of its collaborators will be okay.

Regarding:

Did you check https://github.com/danizord/mid/blob/0.2/src/MiddlewarePipeline.php#L37-L39?

I somehow missed that you were checking the current queue when you called handle(); that makes a ton of sense, and I'll try and test that later today. That said, it will be tempered by an answer to my above question.

[danizord]

Zend\Stratigility\Server::listen() should always produce and/or emit a response, period.

You meant Diactoros? If so, I think it should wrap the pipeline call in a try-catch block.

The question is: do we want to be able to pass a MiddlewarePipe directly to a Server instance and have an expectation that it will "just work"?

IMHO yes. That would make it flexible for Zend\Diactoros consumers to use their custom error handling strategies.

Edit: to clarify, I think Zend\Diactoros should not do any error handling and just rely on the interface of passed $handler.

[weierophinney]

@snapshotpl On a local branch based on your own, I've done the following:

• Incorporated the ideas presented by @danizord with regards to making MiddlewarePipe also act as a RequestHandlerInterface implementation.
• Updated the tests for both MiddlewarePipe and Next to reflect the refactored code.
• Added the class (with tests) Zend\Stratigility\Middleware\PathMiddlewareDecorator, which performs the path segregation tasks originally accomplished by the combination of Route + Next. This implementation is cleaner, as we can pass the original request directly to the original request handler if the decorated middleware invokes it. I've also created an "integration" test that demonstrates nesting such path-segregated middleware within pipelines.
• Added the convenience function Zend\Stratigility\path(), which is a wrapper for new PathMiddlewareDecorator($path, $middleware).

Would you like me to push them to your branch?

[snapshotpl]

@weierophinney Sure :+1:

[weierophinney]

And now we're green again, @snapshotpl !

There are a few last things we need to do before merging:

• [x] We need to tag an alpha1 release of the current release-3.0.0 branch. This will be necessary for people testing the Expressive 3.0-dev series, as otherwise this patch will break their ability to test (as MiddlewarePipe is now final, and the signature of pipe() has changed).
• [x] We need to version the docs. I'll take care of that in a separate patch, and we can the rebase off that one to begin changes for this patch. (Turns out I already did this!)
• [x] We will need a migration document detailing both the public API changes, as well as likely changes to consumer code.
• From the above, we will need to identify:
  • areas where we can build forwards-compatibility features for a final minor version of the v2 series in order to assist in migrations.
  • features that are removed in this patch need to be marked @deprecated in that same v2 release.
  • documentation we can write in that version to help users be forwards-compatible and anticipate the upgrade path.

Finally, either once this is merged, or as part of this particular patch, we should consider adding the following utility functions:

• callable(callable $middleware, ResponseInterface $response = null) to wrap creation of a CallableMiddlewareDecorator: $pipeline->pipe(callable($middleware)); (note: callable MAY be a reserved word, so we may need a different name; perhaps interopCallable())
• doublePass(callable $middleware, ResponseInterface $response = null) to wrap creation of a DoublePassMiddlewareDecorator: $pipeline->pipe(doublePass($middleware))

I'll get to work on the documentation versioning; let me know what you feel you can get to, and we can coordinate tasks.

[weierophinney]

@snapshotpl I think we can merge this; I have a list of things to address in the current develop branch, and will start opening issues and/or PRs to do so. The new utility functions can be part of a new PR.

[snapshotpl]

@weierophinney please give me one day to review it, ok?

[weierophinney]

@snapshotpl Sure; will push the CHANGELOG in a few minutes, but will wait until tomorrow to merge.

[weierophinney]

@snapshotpl In backporting the PathMiddlewareDecorator and related changes to MiddlewarePipe and Next to the develop branch, I've discovered one issue: technically, path-segregated middleware should be allowed to propagate changes to the request to the next middleware (e.g., parsing the request body, adding attributes, etc.). The changes I have proposed here silently drop those.

The fix is easy: I just need to grab the path from the original request URI instance and inject it into the request URI instance provided to the handler. I'll provide tests and a patch shortly for this branch.

Let me know when you've had a chance to review.

[snapshotpl]

I have time in the weekend

[franzliedke]

@weierophinney @snapshotpl This is awesome! I am looking forward to 3.0. :smile:

May I humbly suggest naming this something other than PathMiddlewareDecorator? I highly doubt anybody cares that this is using the decorator pattern; and anybody who knows it, will recognize it. Most importantly, though, something like this would read much more nicely - and, after all, reading code is what we do most of the day...

$pipeline->pipe(new OnlyForPath('/mypath', $middleware));

This might go hand-in-hand with #136.

[weierophinney]

May I humbly suggest naming this something other than PathMiddlewareDecorator? I highly doubt anybody cares that this is using the decorator pattern; and anybody who knows it, will recognize it.

The point in the name is to indicate it cannot be used by itself, but instead decorates other middleware. We have other such decorators already as well:

• CallableMiddlewareDecorator
• DoublePassMiddlewareDecorator

As such, the naming is consistent.

In terms of convenience and readability, this patch already includes the function Zend\Stratigility\path:

$pipeline->pipe(path('/mypath', $middleware));

(The functions in #136 are inspired by the utility function included in this patch.)

[franzliedke]

The point in the name is to indicate it cannot be used by itself, but instead decorates other middleware.

Isn't that already clear based on the constructor signature?

We have other such decorators already as well.

They could be renamed. :wink:

That said, as long as the convenience method is there, I am happy. Just wanted to bring this up to discuss.

[weierophinney]

Thanks, @snapshotpl!