@weierophinney

A new class, MiddlewareContainer, implementing PSR-11's ContainerInterface and decorating a ContainerInterface. This class ensures that middleware pulled from the container implements the PSR-15 MiddlewareInterface. It also allows retrieving middleware by class name if the class does not require constructor arguments.

Do we need it? Maybe we should just register these type of middlewares as invokable in container?

A new class, MiddlewareContainer, implementing PSR-11's ContainerInterface and decorating a ContainerInterface. This class ensures that middleware pulled from the container implements the PSR-15 MiddlewareInterface. It also allows retrieving middleware by class name if the class does not require constructor arguments.

Do we need it? Maybe we should just register these type of middlewares as invokable in container?

Ideally, that's what we'd do. However, that poses a few problems:

• For users relying on that functionality currently, their code would break after upgrading, until they add theseinvokables declarations.
• From a usability perspective, if you're declaring a one-off class that has no dependencies, requiring adding configuration for it is more work that is prone to error (forgetting to add the factory).
• From a context perspective: if we are pulling middleware services from the container, we want to be assured that we don't accidentally pull a non-middleware service due to misconfiguration. Having a check on the type fetched is useful in that regard. (And analogous to the scopedControllerManager in zend-mvc.)

We can largely solve the second use case via the zend-expressive-tooling project; in fact,there is an open issue requesting creation of a factory when middleware:create is called. The first is harder, however; unless we can write a tool to solve that migration problem, it's simpler to have the dedicated container.

The third case, however, is probably the more winning argument here, as it ensures that weonly fetch middleware in the context ofmarshaling middleware for the pipeline and/or routing.

@Ocramius ,@froschdesign ,@webimpress,@ezimuel,@xtreamwayz — any feedback on design/implementation?

Thoughts I've had since implementing (some of which are due to discussions with@ezimuel and@xtreamwayz):

• Creating factories for both theMiddlewareContainer andMiddlewareFactory classes. This would allow us to re-use each (though the former would not likely get used outside the latter), and allow us to provide stronger type-hinting on theMiddlewareFactory, resolving the discussion with@webimpress, above.

• Pushing all routing-related methods, the router itself, and aMiddlewareFactory instance, into theRouteMiddleware. Users would register route-based middleware with that instance:

  $routes->get('/', HomeMiddleware::class,'home');$routes->post('/api/books', [    CorsMiddleware::class,    AuthenticationMiddleware::class,    CreateBookMiddleware::class]);

  My inclination is also to have the$middleware argument be strongly typed againstMiddlewareInterface (more on that below).

  This has the benefit of simplifying theApplication API, and moving the routing functionality closer to the middleware that uses it.

• HaveApplication implementMiddlewarePipeInterface directly, and only accept middleware instances. This means theApplication instance does not need to compose a container any longer, much less theMiddlewareFactory. More on this later.

• Splitting the functionality of piping/processing/handling fromrunning an application.

  What do I mean by that?

  Right now, the remaining complexity of theApplication class, once routing concerns are removed, is around therun() method, as that method requires the following:

  • A way to generate aServerRequestInterface instance if none is provided. This is currently via the static methodServerRequestFactory::fromGlobals(), but we should likely make this configurable in order to accommodate PSR-17 factories in the future.
  • A way to generate an error response in the case that the server request factory raises an exception. This is done currently in two ways, depending on collaborators present:
    • If the instance has a container, and the container has anErrorResponseGenerator, that instance is used to generate the response, by passing an emptyServerRequest instance (created on the fly) and a composed response prototype.
    • Otherwise, the response is generated by manipulating the composed response prototype.
  • An emitter, for emitting the response.

  If we were to split responsibilities, theApplication class, as it is currently named would compose only:

  • a default handler, for times whenhandle() is called.

  We could then create a new class, perhaps anApplicationRunner, that could compose the following:

  • An emitter. The container would define anEmitterInterface service that resolves to anEmitterStack with theSapiEmitter composed by default.
  • AServerRequestErrorResponseGenerator service; this would be used when unable to marshal the server request, in order to generate the response. It should be capable of receiving only an exception. (A factory could close over theErrorResponseGenerator in order to provide a defaultServerRequest and response prototype, so as to allow re-use of existing functionality.)

• Create a proxy class that composes theRouteMiddleware,Application, andApplicationRunner, along with aMiddlewareFactory. This class wouldmimic the current v2 API with regards to piping and routing. Its primary use case would be for applicationsupgrading from v2.

Essentially, for usersupgrading from v2, we'd update theirApplication service to resolve to the proxy class. This would allow them to gradually upgrade to the v3 APIs. In v3, we'd alter thepublic/index.php self-executing function: we'd import the application, routing middleware, and middleware factory as variables before calling therequire statements:

$app =$container->get(Application::class);$factory =$container->get(MiddlewareFactory::class);$routes =$container->get(RouteMiddleware::class);require'config/pipeline.php';require'config/routes.php';

These files would then look like the following:

// config/pipeline.php:usefunctionZend\Stratigility\path;$app->pipe($factory->lazy(ErrorMiddleware::class));$app->pipe(path('/api',$factory->pipeline(    CorsMiddleware::class,    AuthenticationMiddleware::class,    AuthorizationMiddleware::class));// Since we already have it instantiated, we can pipe the routing middleware as// an instance:$app->pipe($routes);$app->pipe($factory->lazy(ImplicitHeadMiddleware::class));$app->pipe($factory->lazy(ImplicitOptionsMiddleware::class));$app->pipe($factory->lazy(DispatchMiddleware::class));$app->pipe($factory->lazy(NotFoundMiddleware::class));// config/routes.php:$routes->get('/',$factory->lazy(HomeMiddleware::class));$routes->post('/api/books',$factory->pipeline(    BookValidationMiddleware::class,    CreateBookMiddleware::class));

Essentially, separating these concerns further means we can have more type-safety, less overall code, and more flexibility for users.

The proxy class provides both a migration tool, but also potentially ausability layer for developers who do not want to always use the factory to prepare middleware for piping or routing, but instead just provide the arguments.

Updates

• 2018-01-30: Updated last example to use$factory->lazy(<class name>) instead of<class name> to demonstrate suggested design, vs. current.

About everything you write sounds good. I'm not sure about theApplicationRunner though. I mean right now I need to grab the application from the container and execute$app->run(). That's pretty easy. How would this work with theApplicationRunner? On the other hand, if the complexity of the Application class itself is reduced that might be worth it.

The proxy class for backward compatibility sounds good as well.

I mean right now I need to grab the application from the container and execute $app->run().

TheApplicationRunner would compose theApplication instance as well, and exposerun():

$runner =$container->get(ApplicationRunner::class);$runner->run();

Internally,run() would marshal the request and response (if not provided), call on the composed application'shandle() method (passing it the request), and then pass the response to the emitter.

May i suggest a following change:

(require'config/pipeline.php')($app,$factory,$router);(require'config/routes.php')($app,$factory,$router);

useZend\Expressive\Application;usefunctionZend\Stratigility\path;returnfunction (Application$app,MiddlewareFactory$factory,RouteMiddleware$routes) {$app->pipe(ErrorMiddleware::class);$app->pipe(path('/api',$factory->pipeline(        CorsMiddleware::class,        AuthenticationMiddleware::class,        AuthorizationMiddleware::class    ));// Since we already have it instantiated, we can pipe the routing middleware as// an instance:$app->pipe($routes);$app->pipe(ImplicitHeadMiddleware::class);$app->pipe(ImplicitOptionsMiddleware::class);$app->pipe(DispatchMiddleware::class);$app->pipe(NotFoundMiddleware::class);}

May i suggest a following change:

I really like that idea!

We could then create a new class, perhaps an ApplicationRunner, that could compose the following:

• An emitter. The container would define an EmitterInterface service that resolves to an EmitterStack with the SapiEmitter composed by default.
• A ServerRequestErrorResponseGenerator service; this would be used when unable to marshal the server request, in order to generate the response. It should be capable of receiving only an exception. (A factory could close over the ErrorResponseGenerator in order to provide a default ServerRequest and response prototype, so as to allow re-use of existing functionality.)

This is a good functionality to be split into separate package to be used with any request handler.
In zend-mvc refactoring for PSR I made Application a request handler and could reuse ApplicationRunner and emitter stack.

May i suggest a following change:

You have shown me this before. I really like it.

You have shown me this before. I really like it.

The pattern used to be very common in node, before theimport construct was added for ES6. As such, it's quite familiar to me; just not in aPHP context!

@weierophinney I really like the idea of having routes specified on aRouteMiddleware instance ($routes). In this way the routing configuration is related to a route object and not the application itself.

I see the need for apath() function that returns a middleware but I would like to simplify the $factory usage for the middleware pipeline. IMHO, the syntax that you proposed it's too verbose:

$app->pipe(path('/api',$factory->pipeline(    CorsMiddleware::class,    AuthenticationMiddleware::class,    AuthorizationMiddleware::class)));

I think we should continue to supportarray as a way to specify a pipeline. I think this is a very intuitive idea to think about anordered sequence of middleware (a pipeline). I would like to propose this change:

$app->pipe(path('/api, [    CorsMiddleware::class,    AuthenticationMiddleware::class,    AuthorizationMiddleware::class]));

We can accomplish this managing the array of middleware in thepath() function itself, without changing theApplication::pipe function. The same idea can be used for the variousget,post,put, ... functions of routes. Again, the usage of array is a very intuitive way to define a pipeline, and people using Expressive are already familiar with that, we should not change this API.

This routing is very intuitive, even for people not familiar with Expressive:

$routes->post('/api/books', [    BookValidationMiddleware::class,    CreateBookMiddleware::class]);

Regarding the other proposals, I'm in favor of it. It will be awesome to see a complete skeleton application to analyze the changes from a user API point of view.

I'm just about done handling the changesas suggested in my previous large design comment, and in doing so, discovered something interesting: separating the routing concerns, removing the application running concerns, and making thepipe() method strongly typed means that theApplication class is essentially just aMiddlewarePipe instance.

As such, I'm re-creating theApplication class to compose the following:

• aMiddlewareFactory instance
• aMiddlewarePipeInterface implementation
• aRouteMiddleware instance
• anApplicationRunner instance

The class then implements bothRequestHandlerInterface andMiddlewareInterface, and keeps most public methods from the current v2 implementation (minusgetContainer(),getEmitter(), andgetDefaultHandler() methods). The methods it exposes all proxy to one or more of the above classes.

What's interesting about this approach is that it keeps the existing signatures and APIentirely.

This then moves the discussion to: how do we want to write the skeleton?

It could literally stay exactly the same as it is currently. Alternately, we could do like@Xerkus suggested above, and have the routes and pipeline files return callbacks that accept specific arguments (likely the middleware factory, and then one of either the middleware pipeline or route middleware).

What's interesting to me is that this refactoring exercise leaves us with far more flexibility; simpler, more maintainable code; stronger typing; and choice in how we have users consume the project.

Once the changes are complete, I'll push here.

All major refactors are now complete. The only bits I'm mulling over at this time are:

• Moving the functionality ofApplicationConfigInjectionTrait into another class using static methods:ApplicationConfigInjector::injectPipelineFromConfig(Application $application, array $config), etc.
• Removing theMiddlewareFactory::path() method.

Thoughts on these proposals and/or the shape of the refactor at this time?

@weierophinney

Removing the MiddlewareFactory::path() method.

I'm 👍 for this. IMHO theMiddlewareFactory should focus only on creating middleware fromstring|callable|array, otherwise we would end up adding various other methods for each decorator likehost()doublePass() and such.

For the shape of the refactor, I need to review it with more time tomorrow, if you can wait one more day before merging. I missed the notification of this PR. 😄

Just a few comments from what I've read so far:

• I love the idea of extracting all theApplication behaviors into separate participants and making them usable outside ofApplication. That makes Expressive even more flexible and composable. Now I can switch to another middleware pipeline implementation, use a 3rd party router/dispatch middleware, or even build my own framework on top of Expressive components, which is great :)
• What about renamingApplicationRunner toHttpServer or something like that? Also, if you could extract it to a separate package, it would be really nice :)
• Please renameMiddlewareFactory:: prepare() to__invoke() so that we can use it like a function:$middleware(function ($req, $handler) {...}).
• Not sure, but I think route configuration methods (post(),get(), ...) could be moved toZend\Expressive\Router\RouterInterface directly. That would simplifyRouterMiddleware implementation and allow using a custom router middleware that consumes the Expressive router but behaves diferently. For instance, I'd like to have a router that returns 404 response directly instead of calling the next middleware.

btw, congrats for the great work done in PSR-15 and Expressive 3 :)

What about renaming ApplicationRunner to HttpServer or something like that? Also, if you could extract it to a separate package, it would be really nice :)

@Xerkus has indicated he could use this for the zend-mvc refactor as well, so we will definitely extract it. My thought is to also extract the emitter interface and emitters from zend-stratigility either into their own package, or into the same package as the runner (which will consume them).

Please rename MiddlewareFactory:: prepare() to __invoke() so that we can use it like a function: $middleware(function ($req, $handler) {...}).

This makes usage more difficultwithin theApplication class andApplicationConfigInjectionTrait, however, as we either need to wrap a property (($this->factory)($middleware)) or call it directly ($this->factory->__invoke($middleware)).

I'll think on it.

Not sure, but I think route configuration methods (post(), get(), ...) could be moved to Zend\Expressive\Router\RouterInterface directly. That would simplify RouterMiddleware implementation and allow using a custom router middleware that consumes the Expressive router but behaves diferently. For instance, I'd like to have a router that returns 404 response directly instead of calling the next middleware.

TheRouterInterface is purposely small, to make it simpler to use. Because it's an interface, we cannot require features such as route duplication detection, which is currently something theApplication class handles (and, in this patch, theRouteMiddeware. Additionally, the routing methods we expose are aroundpath-based routing specifically; routingcould happen around such things as the scheme, host/authority, and even query parameters, as routes are passed a PSR-7 request itself.

My thought is that we could bring theRouteMiddleware from this patch into the zend-expressive-router package, and maybe call itPathBasedRoutingMiddleware to indicate that it's specific to that purpose. It could even expose a method to addRoute instances directly, for those cases when you want to add routes that are not examining the path.

If we were to go that route, I'd suggest:

• A new minor release of zend-expressive-router that brings over the current v2RouteMiddleware as-is, but renaming it as suggested above, and providing a factory for it.
• A new minor release of zend-expressive that hasRouteMiddleware extend the zend-expressive-router version.
• In zend-expressive-router v3, we add the routing methods as we have them in this patch.
• In zend-expressive v3, we removeRouteMiddleware entirely, and consume the middleware from zend-expressive-router.

Thoughts on that approach,@danizord?