Refactor pipe #134

snapshotpl · 2017-12-14T22:06:02Z

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

weierophinney · 2017-12-14T22:18:20Z

src/MiddlewarePipe.php

     */
-    public function pipe($path, $middleware = null) : self
+    public function pipe(string $path, MiddlewareInterface $middleware) : self


There's an even better way: reverse the arguments, and make the path optional:

public function pipe(MiddlewareInterface $middleware, string $path = '/') : void

(Note that I think this should be a void return as well; let's remove the fluent interface usage).

This will make it more clear that what's really optional is the path.

Yes, it's a BC break. However, it's one we should have likely done a long time ago. The only reason the arguments were in this order in the first place was because I had done a literal port from Sencha Connect, without considering whether the signatures made sense within PHP.

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 · 2017-12-14T22:18:44Z

src/MiddlewarePipe.php

+     *
+     * Each middleware should be executed every request cycle.
+     */
+    public function pipeMiddleware(MiddlewareInterface $middleware) : self


This one would no longer be necessary if we approach it the way I suggest above.

snapshotpl · 2017-12-14T22:29:09Z

@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 · 2017-12-14T22:31:39Z

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 · 2017-12-14T22:34:58Z

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

danizord · 2017-12-14T22:38:03Z

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 · 2017-12-14T22:41:00Z

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

danizord · 2017-12-14T22:45:42Z

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

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

weierophinney · 2017-12-14T22:53:04Z

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 · 2017-12-14T22:57:22Z

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

weierophinney · 2017-12-14T23:02:28Z

@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 · 2017-12-14T23:06:44Z

@weierophinney Looks perfect for me 👌

snapshotpl · 2017-12-14T23:25:53Z

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

weierophinney · 2017-12-14T23:27:52Z

Exactly!

…

On Dec 14, 2017 5:25 PM, "Witold Wasiczko" ***@***.***> 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 <#134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlV-mBh7VCDyytbWte0pz4X9z7rOc0ks5tAa6CgaJpZM4RCuxO> .

snapshotpl · 2017-12-14T23:41:30Z

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

weierophinney · 2017-12-14T23:47:28Z

src/MiddlewarePipe.php

@@ -29,16 +28,14 @@
 *
 * @see https://github.com/sencha/connect
 */
-class MiddlewarePipe implements MiddlewareInterface
+final class MiddlewarePipe implements MiddlewareInterface


Don't make this final, please. We extend it in Expressive.

I knew you would ask it. Why not use composition in Expressive instead of inheritance?

Agree, I think we can't extend it anymore in Expressive, because then pipe will accept only MiddlewareInterface. Currently we can pipe middleware, callback or string (from container) and we wrap it inside the application. If we would extend MiddlewarePipe then we can't change signature of pipe and folks would need to wrap everything on piping.

That's a good point; I'm good with this, then.

weierophinney · 2017-12-14T23:47:57Z

src/MiddlewarePipe.php

 {
    /**
     * @var SplQueue
     */
-    protected $pipeline;


This needs to be protected, for the same reason as above.

weierophinney · 2017-12-14T23:49:22Z

test/MiddlewarePipeTest.php

@@ -50,7 +50,7 @@ private function createFinalHandler() : RequestHandlerInterface

    public function testHandleInvokesUntilFirstHandlerThatDoesNotCallNext()
    {
-        $this->pipeline->pipe(new class () implements MiddlewareInterface
+        $this->pipeline->pipeMiddleware(new class () implements MiddlewareInterface


You'll need to update this, but I think you're aware of that. :-)

danizord · 2017-12-15T00:19:47Z

@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 · 2018-01-03T22:49:27Z

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 · 2018-01-03T23:11:10Z

@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 · 2018-01-04T14:50:33Z

@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 · 2018-01-08T17:37:56Z

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:

We compose a fallback handler in MiddlewarePipe. That handler is used when handle() is called: return $this->process($request, $this->fallbackHandler).
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 · 2018-01-08T18:36:16Z

@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 · 2018-01-08T18:42:25Z

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 · 2018-01-08T19:07:08Z

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 · 2018-01-09T18:31:08Z

@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 · 2018-01-09T20:17:08Z

@weierophinney Sure 👍

Modifies tests to cover _only_ the behaviors they now implement.

This replaces the path segregation features of the original stratigility MiddlewarePipe with a middleware decorator. The decorator receives the path prefix to segregate by, and the middleware to dispatch when matched. During `process()`, it checks to see if the prefix is matched by the current URI path at an appropriate boundary. If not, it acts as a pass-through to the handler. If it *does* match, it does the following: - Creates a new request that contains a URI that truncates the prefix from the path. - Prepares a request handler that decorates the original in order to process it with the original incoming request. - Dispatches the composed middleware using the new request and the decorated request handler. I have pulled relevant tests from the original test suite related to path segregation and adapted them to test against the new PathMiddlewareDecorator; I have also added an integration test demonstrating that nesting works as expected within a pipeline.

This function, when imported, simplifies creation of path-segregated middleware. It allows you to refactor this: ```php use Zend\Stratigility\Middleware\PathMiddlewareDecorator; $pipeline->pipe(new PathMiddlewareDecorator('/foo', $middleware)); ``` to: ```php use Zend\Stratigility\path $pipeline->pipe(path('/foo', $middleware)); ```

- InvalidMiddlewareException is no longer thrown - Route is no longer used internally

Too long of lines

Moves sections around to move the signature changes section first in the list of public interface changes. Also moves the documentation of PSR-15-related signature changers to the PSR-15 section. Adds signature changes for MiddlewarePipe and Next, documents the new PathMiddlewareDecorator and its associated `path()` utility function, and the removed Route and InvalidMiddlewareException classes.

Documents the `PathMiddlewareDecorator` and associated `path()` utility function, and ensures all documentation is up to date with changes in this PR and on the release-3.0.0 branch.

weierophinney · 2018-01-10T20:34:11Z

@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 · 2018-01-10T20:35:22Z

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

weierophinney · 2018-01-10T20:41:10Z

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

weierophinney · 2018-01-11T20:02:57Z

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

This patch backports the following classes from the release-3.0.0 development series: - `Zend\Stratigility\Middleware\CallableMiddlewareDecorator`, for decorating middleware that follows the PSR-15 signature. - `Zend\Stratigility\Middleware\DoublePassMiddlewareDecorator`, for decorating callable middleware that follows the double pass middleware signature. - `Zend\Stratigility\Middleware\PathMiddlewareDecorator`, for providing path-segregated middleware. This required also extracting the class `Zend\Stratigility\Middleware\PathRequestHandlerDecorator`, which is used internally in order to decorate the request handler passed to middleware it decorates. All classes and their were updated to adapt to the http-middleware-compatibility shims.

Discovered while backporting to the 2.X series. Essentially, a path segregated middleware should still be able to pass a new request on to the next layer. However, in doing so, the _path_ should be reset. This patch updates the `PathMiddlewareDecoratorIntegrationTest` to reflect this behavior, and updates the anonymous request handler created by the `PathMiddlewareDecorator` to invoke the composed handler with the runtime request after resetting its URI instance with the original path.

This patch backports the following classes from the release-3.0.0 development series: - `Zend\Stratigility\Middleware\CallableMiddlewareDecorator`, for decorating middleware that follows the PSR-15 signature. - `Zend\Stratigility\Middleware\DoublePassMiddlewareDecorator`, for decorating callable middleware that follows the double pass middleware signature. - `Zend\Stratigility\Middleware\PathMiddlewareDecorator`, for providing path-segregated middleware. This required also extracting the class `Zend\Stratigility\Middleware\PathRequestHandlerDecorator`, which is used internally in order to decorate the request handler passed to middleware it decorates. All classes and their were updated to adapt to the http-middleware-compatibility shims.

snapshotpl · 2018-01-11T23:45:54Z

I have time in the weekend

franzliedke · 2018-01-12T18:11:59Z

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

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 · 2018-01-15T14:19:05Z

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 · 2018-01-15T15:48:11Z

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

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

Close #134

weierophinney · 2018-01-15T22:24:19Z

Thanks, @snapshotpl!

weierophinney suggested changes Dec 14, 2017

View reviewed changes

danizord mentioned this pull request Dec 15, 2017

path() danizord/mid#2

Merged

weierophinney mentioned this pull request Jan 4, 2018

Route value object without magic #135

Closed

weierophinney added this to the 3.0.0alpha1 milestone Jan 4, 2018

weierophinney force-pushed the refactor-pipe branch from b6db186 to 20e4180 Compare January 9, 2018 20:40

weierophinney approved these changes Jan 9, 2018

View reviewed changes

snapshotpl and others added 8 commits January 10, 2018 11:56

Simplify path normalization and improve pipe implementation readability

8644651

Remove fluent interface

405a447

Remove dispatching from next ane middlewarepipe

f3ff37a

Tests both the MiddlewarePipe and Next implementations

94926ba

Modifies tests to cover _only_ the behaviors they now implement.

Removed classes that are no longer used/necessary

fa69824

- InvalidMiddlewareException is no longer thrown - Route is no longer used internally

CS fixes per phpcs

b7e963b

Too long of lines

weierophinney force-pushed the refactor-pipe branch from 20e4180 to b7e963b Compare January 10, 2018 17:56

weierophinney added 2 commits January 10, 2018 12:21

Completes documentation of all new features

141d6e9

Documents the `PathMiddlewareDecorator` and associated `path()` utility function, and ensures all documentation is up to date with changes in this PR and on the release-3.0.0 branch.

Adds CHANGELOG entries for zendframework#134

aa0e8d5

weierophinney mentioned this pull request Jan 10, 2018

Provide utility functions for decorating non-standard middleware #136

Merged

1 task

weierophinney modified the milestones: 3.0.0alpha1, 3.0.0alpha2 Jan 10, 2018

weierophinney force-pushed the refactor-pipe branch from ee21d90 to 127502b Compare January 11, 2018 22:24

weierophinney merged commit 127502b into zendframework:release-3.0.0 Jan 15, 2018

weierophinney added a commit that referenced this pull request Jan 15, 2018

Merge branch 'feature/134-pipe-refactor' into release-3.0.0

494b3ad

Close #134

Refactor pipe #134

Refactor pipe #134

Conversation

snapshotpl commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapshotpl commented Dec 14, 2017

weierophinney commented Dec 14, 2017

snapshotpl commented Dec 14, 2017

danizord commented Dec 14, 2017

snapshotpl commented Dec 14, 2017

danizord commented Dec 14, 2017

weierophinney commented Dec 14, 2017

danizord commented Dec 14, 2017

weierophinney commented Dec 14, 2017

danizord commented Dec 14, 2017

snapshotpl commented Dec 14, 2017

weierophinney commented Dec 14, 2017 via email

snapshotpl commented Dec 14, 2017

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

danizord commented Dec 15, 2017

weierophinney commented Jan 3, 2018

danizord commented Jan 3, 2018 • edited Loading

weierophinney commented Jan 4, 2018

weierophinney commented Jan 8, 2018

danizord commented Jan 8, 2018

weierophinney commented Jan 8, 2018

danizord commented Jan 8, 2018 • edited Loading

weierophinney commented Jan 9, 2018

snapshotpl commented Jan 9, 2018

weierophinney commented Jan 10, 2018

snapshotpl commented Jan 10, 2018

weierophinney commented Jan 10, 2018

weierophinney commented Jan 11, 2018

snapshotpl commented Jan 11, 2018 • edited Loading

franzliedke commented Jan 12, 2018

weierophinney commented Jan 15, 2018

franzliedke commented Jan 15, 2018

weierophinney commented Jan 15, 2018

danizord commented Jan 3, 2018 •

edited

Loading

danizord commented Jan 8, 2018 •

edited

Loading

snapshotpl commented Jan 11, 2018 •

edited

Loading