Refactor: extract middleware marshaling, routing, and app running #543

weierophinney · 2018-01-25T21:48:54Z

This patch implements the changes as outlined in the RFC: Expressive 3 Design Changes, as well as those as outlined in this comment and this comment, below.

In particular:

Updates to consume and implement PSR-15.
The following changes were made to middleware marshaling:
- 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.
- A new class, MiddlewareFactory, accepts a PSR-11 ContainerInterface to its constructor, which it decorates internally as a MiddlewareContainer. It then exposes methods for marshaling middleware:
  - callable(callable $middleware) : CallableMiddlewareDecorator
  - lazy(string $middleware) : LazyLoadingMiddleware
  - pipeline(...$middleware) : MiddlewarePipeline
  - path(string $path, $middleware) : PathMiddlewareDecorator
  - prepare($middleware) : MiddlewareInterface; this one proxies to the other methods.
  Double-pass middleware is not supported.
A new class, ApplicationRunner, composes:
- A RequestHandlerInterface instance (typically, a MiddlewarePipeInterface)
- An EmitterInterface instance (typically, an EmitterStack composing a SapiEmitter)
- A callable factory for generating a ServerRequestInterface instance
- A callable response generator, used when the server request factory raises a throwable or exception.
When run() is called, the class marshals a request, potentially generates an error response, and otherwise passes handling of the request to the handler; it then passes the returned response to the emitter.

This class can potentially be extracted to its own library.
The RouteMiddleware has been updated to add the methods route, get, post, etc. from Application. Unlike Application, they only accept MiddlewareInterface instances.
New factories (in the Zend\Expressive\Container namespace):
- RouteMiddlewareFactory
- DispatchMiddlewareFactory
- EmitterFactory should be mapped to Zend\Diactros\Response\EmitterInterface.
- ServerRequestFactoryFactory should be mapped to Zend\Expressive\ServerRequestFactory
- ServerRequestErrorResponseGeneratorFactory should be mapped to Zend\Expressive\ServerRequestErrorResponseGenerator.
Application has been completely rewritten. It now composes:
- A MiddlewareFactory instance
- A MiddlewarePipeInterface instance, generally a MiddlewarePipe
- A RouteMiddleware instance
- An ApplicationRunner instance
It implements RequestHandlerInterface and MiddlewareInterface, and composes the ApplicationConfigInjectionTrait. It defines the same public API as previously, minus the various getters (as none of them are applicable anymore). Each method now proxies to the appropriate collaborator, pre-processing arguments as necessary:
- handle() delegates to the MiddlewarePipeInterface
- process() delegates to the MiddlewarePipeInterface
- run() delegates to the ApplicationRunner
- pipe() delegates to the MiddlewarePipeInterface, after first passing its arguments to the MiddlewareFactory, and, in the case of two arguments, the Zend\Stratigility\path() utility function.
- route() and related methods delegate to the RouteMiddleware, after first passing the $middleware argument to the MiddlewareFactory
- pipeRoutingMiddleware and pipeDispatchMiddleware are removed; users will now pipe these middleware just like any other middleware.
Zend\Expressive\Delegate\NotFoundDelegate was renamed to Zend\Expressive\Handler\NotFoundHandler.
Zend\Expressive\Middleware\NotFoundHandler was renamed to Zend\Expressive\Middleware\NotFoundMiddleware; its related factory in the Zend\Expressive\Container namespace was similarly renamed.
Tests were updated to reflect the changes, as necessary.

Updates

2018-01-30: Updated to reflect refactors of routing and app running.

geerteltink · 2018-01-25T22:14:06Z

src/ApplicationConfigInjectionTrait.php

@@ -9,6 +9,7 @@

 namespace Zend\Expressive;

+use Psr\Http\Server\MiddlewareInterface;


MiddlewareInterface is not used.

geerteltink · 2018-01-25T22:15:45Z

src/Container/ApplicationFactory.php

            : null;

        $emitter = $container->has(EmitterInterface::class)
            ? $container->get(EmitterInterface::class)
            : null;

-        $app = new Application($router, $container, $delegate, $emitter);
+        $response = $container->has(ResponseInterface::class)
+            ? $container->get(ResponseInterface::class)


ResponseInterface needs import.

geerteltink · 2018-01-25T22:21:52Z

src/Middleware/DispatchMiddleware.php

@@ -9,11 +9,11 @@

 namespace Zend\Expressive\Middleware;

-use Interop\Http\Server\MiddlewareInterface;
-use Interop\Http\Server\RequestHandlerInterface;
 use Psr\Container\ContainerInterface;


ContainerInterface not used.

geerteltink · 2018-01-25T22:22:18Z

src/Middleware/DispatchMiddleware.php

 use Psr\Container\ContainerInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
 use Zend\Expressive\MarshalMiddlewareTrait;


MarshalMiddlewareTrait doesn't exist.

geerteltink · 2018-01-25T22:22:37Z

src/Middleware/DispatchMiddleware.php

 use Psr\Container\ContainerInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
 use Zend\Expressive\MarshalMiddlewareTrait;
 use Zend\Expressive\Router\RouteResult;
 use Zend\Expressive\Router\RouterInterface;


RouterInterface isn't used.

michalbundyra · 2018-01-26T00:18:23Z

composer.json

        "psr/container": "^1.0",
        "psr/http-message": "^1.0.1",
+        "psr/http-server-middleware": "^1.0.0",


Usually we use just ^1.0.

michalbundyra · 2018-01-26T00:20:24Z

src/Application.php

    {
-        return $this->route($path, $middleware, ['POST'], $name);
+        try {
+            $request  = $request ?: ServerRequestFactory::fromGlobals();


One space before = please

michalbundyra · 2018-01-26T00:22:23Z

src/Application.php

    }

    /**
-     * @param callable|string $middleware Middleware (or middleware service name) to associate with route.
-     * @param null|string $name The name of the route.
+     * @param string|array|callable|MiddlewareInterface $middleware Middleware


I would argue for the following order:

null

all other scalar types, in alphabetical order

complex built-in types (array, callable, resource) in alphabetical order

instance types, in alphabetical order

So, clearly we have differing opinions, and we'll need to make a decision. In the meantime, this is not the main point of this PR at this time; I need feedback on architecture and design.

michalbundyra · 2018-01-26T00:26:28Z

src/Container/DispatchMiddlewareFactory.php

+ * @copyright Copyright (c) 2018 Zend Technologies USA Inc. (https://www.zend.com)
+ * @license   https://github.com/zendframework/zend-expressive/blob/master/LICENSE.md New BSD License
+ */
+


No new line before strict_type declaration please.

@webimpress let's try reducing noise over CS by moving these discussions to the CS checks repo instead 👍

I'm also little bit unhappy with this type of comments. Because the first comments are always related to the coding standard. After the noise, there are often no other comments. CS is important, but code formatting should not have the first place in this and other discussions. (imo)

Coding Standard Police 😉

This is why we need proper CS for zf and then we are not going to have these comments at all. Sorry, I'm adding comments about everything I can see at the beginning because otherwise (it happened already couple times) it could be merged without fixing...

@webimpress
Sorry, my comment was not only for you. It was a hint to: zendframework/zend-coding-standard/pull/1

ping @weierophinney

Sorry guys, actually I was wrong... Probably we should keep the line. See my comments there:
zendframework/zend-expressive-aurarouter#30 (comment)

michalbundyra · 2018-01-26T00:27:03Z

src/Container/DispatchMiddlewareFactory.php

+{
+    public function __invoke(ContainerInterface $container) : DispatchMiddleware
+    {
+        return new DispatchMiddleware();


Do we need that factory? Isn't it invokable?

It is invokable, but it may not always be. Additionally, having it here simplifies how we refer to it when writing configuration for different container types.

michalbundyra · 2018-01-26T00:29:55Z

src/Exception/InvalidMiddlewareException.php

+    }
+
+    /**
+     * @var mixed $service The actual service created by the container.


@param instead of @var please

michalbundyra · 2018-01-26T00:34:17Z

src/MiddlewareFactory.php

+            return $this->pipeline(...$middleware);
+        }
+
+        if ((! is_string($middleware) || empty($middleware))) {


remove one pair of parentheses and the second part can be just ! $middleware

michalbundyra · 2018-01-26T00:35:18Z

src/MiddlewareFactory.php

+    {
+        if (! $this->container) {
+            throw new Exception\ContainerNotRegisteredException(sprintf(
+                'Cannot marshal middleware by service name "%s"; no container registered',


Maybe it should be moved to the static method in the exception?

michalbundyra · 2018-01-26T00:37:20Z

src/MiddlewareFactory.php

+
+    public function __construct(ContainerInterface $container = null)
+    {
+        $this->container = $container ? new MiddlewareContainer($container) : null;


For better testability we should pass MiddlewareContainer as param instead of creating one in the constructor.

It makes usability harder, however, as developers who are creating an instance on the fly now need to do the following:

$factory = new MiddlewareFactory(new MiddlewareContainer($app->getContainer()));

Additionally, the above example I present would fail with an E_FATAL if $app->getContainer() returns null (which is possible currently), which means it technically needs to become:

$factory = new MiddlewareFactory( $app->getContainer() ? new MiddlewareContainer($app->getContainer()) : null );

which is not user friendly at all. (FTR, if the middleware in a specification or a pipeline resolves to an constructor-less class, a callable, or a MiddlewareInterface instance, the factory can still do its work; it's only if a service name is used that the container becomes necessary. At that point, it raises an exception when a container is missing.)

I've since changed this to accept only a MiddlewareContainer instance.

michalbundyra · 2018-01-26T00:39:20Z

test/Container/NotFoundMiddlewareFactoryTest.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-expressive for the canonical source repository
+ * @copyright Copyright (c) 2016-2017 Zend Technologies USA Inc. (https://www.zend.com)


2018 only, please

This file was renamed from NotFoundHandler; date range is correct.

Did you use git mv to rename the file? We are loosing the connection between new and old file, it looks like new file...
Shouldn't we update year range as the file is updated (renamed)?

best way is to move file 100% unchanged, and then edit it in another commit as git does not really have "move" type of change. Simple diff might not detect it anyway as it uses similarity threshold. It is more for a 3-way merge benefit.

... or use PhpStorm :D

It keeps track of moved files (most of the time) so you can move and edit it in the same commit. Not sure how to do that in git. But as PhpStorm can do it, it must be doable with git as well.

I did a git mv, but then also added the new file in the same commit, which is why this happened.

Note to self: be more granular in commits.

michalbundyra · 2018-01-26T09:08:28Z

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

michalbundyra · 2018-01-26T09:46:09Z

src/MiddlewareFactory.php

+     *
+     * @param string|array|MiddlewareInterface $middleware
+     */
+    public function path(string $path, $middleware) : PathMiddlewareDecorator


Maybe the same way we should add method host?

I'm looking again through the code, and it doesn't to be right, imho. Looks like we can do the same things in multiple ways now, and it's confusing. I think we should deliver simple interface for devs. Need a bit more time to think about it...

I'm considering removing the path() method, as the same can be accomplished with the following:

use function Zend\Stratigility\path; $app->pipe(path('/api', $factory->prepare($middleware));

If we keep the methods down to only those needed for preparing middleware of different types (prepare(), callable(), lazy()) and pipelines (pipeline()), we can use these with any of the various decorator classes and/or their related utility functions. That means we can add new decorators and utility functions in Stratigility or Expressive, and they're automatically supported.

weierophinney · 2018-01-29T20:15:30Z

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 these invokables 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 scoped ControllerManager 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 we only fetch middleware in the context of marshaling middleware for the pipeline and/or routing.

weierophinney · 2018-01-29T21:07:33Z

@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 the MiddlewareContainer and MiddlewareFactory 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 the MiddlewareFactory, resolving the discussion with @webimpress, above.
Pushing all routing-related methods, the router itself, and a MiddlewareFactory instance, into the RouteMiddleware. 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 against MiddlewareInterface (more on that below).

This has the benefit of simplifying the Application API, and moving the routing functionality closer to the middleware that uses it.
Have Application implement MiddlewarePipeInterface directly, and only accept middleware instances. This means the Application instance does not need to compose a container any longer, much less the MiddlewareFactory. More on this later.
Splitting the functionality of piping/processing/handling from running an application.

What do I mean by that?

Right now, the remaining complexity of the Application class, once routing concerns are removed, is around the run() method, as that method requires the following:
- A way to generate a ServerRequestInterface instance if none is provided. This is currently via the static method ServerRequestFactory::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 an ErrorResponseGenerator, that instance is used to generate the response, by passing an empty ServerRequest 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, the Application class, as it is currently named would compose only:
- a default handler, for times when handle() is called.
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.)
Create a proxy class that composes the RouteMiddleware, Application, and ApplicationRunner, along with a MiddlewareFactory. This class would mimic the current v2 API with regards to piping and routing. Its primary use case would be for applications upgrading from v2.

Essentially, for users upgrading from v2, we'd update their Application service to resolve to the proxy class. This would allow them to gradually upgrade to the v3 APIs. In v3, we'd alter the public/index.php self-executing function: we'd import the application, routing middleware, and middleware factory as variables before calling the require 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:
use function Zend\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 a usability 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.

geerteltink · 2018-01-29T21:50:42Z

About everything you write sounds good. I'm not sure about the ApplicationRunner 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 the ApplicationRunner? 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.

weierophinney · 2018-01-29T21:54:06Z

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

The ApplicationRunner would compose the Application instance as well, and expose run():

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

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

Xerkus · 2018-01-29T23:12:12Z

May i suggest a following change:

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

use Zend\Expressive\Application;

use function Zend\Stratigility\path;

return function (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);
}

weierophinney · 2018-01-29T23:16:21Z

May i suggest a following change:

I really like that idea!

Xerkus · 2018-01-29T23:21:24Z

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.

geerteltink · 2018-01-30T06:10:50Z

May i suggest a following change:

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

weierophinney · 2018-01-30T14:11:33Z

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

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

ezimuel · 2018-01-30T17:36:05Z

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

I see the need for a path() 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 support array as a way to specify a pipeline. I think this is a very intuitive idea to think about an ordered 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 the path() function itself, without changing the Application::pipe function. The same idea can be used for the various get, 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.

weierophinney · 2018-01-30T20:33:25Z

I'm just about done handling the changes as 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 the pipe() method strongly typed means that the Application class is essentially just a MiddlewarePipe instance.

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

a MiddlewareFactory instance
a MiddlewarePipeInterface implementation
a RouteMiddleware instance
an ApplicationRunner instance

The class then implements both RequestHandlerInterface and MiddlewareInterface, and keeps most public methods from the current v2 implementation (minus getContainer(), getEmitter(), and getDefaultHandler() 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 API entirely.

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.

weierophinney · 2018-01-30T22:47:08Z

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

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

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

Xerkus · 2018-01-30T23:17:29Z

src/MiddlewareFactory.php

+     * Creates and returns a PathMiddlewareDecorator instance after first
+     * passing the $middleware argument to prepare().
+     *
+     * @param string|array|MiddlewareInterface $middleware


missing @param for first parameter. I don't know if all IDEs will interpret this properly.

This is standard across major IDEs, at this point, and phpdocumentor infers types from typehints as well. We've already decided for zend-coding-standard to require annotations only if they're adding information, or the type is unspecified.

Xerkus · 2018-01-30T23:31:34Z

src/Container/ApplicationRunnerFactory.php

+        return new ApplicationRunner(
+            $container->get(Application::class),
+            $container->get(EmitterInterface::class),
+            $container->get(ServerRequestFactory::class),


ServerRequestFactory and ServerRequestErrorResponseGenerator are not real classes, shouldn't they be used as string literals?

I go back and forth on this, particularly when the service namespace doesn't match the current one.

We can address later.

Xerkus · 2018-01-30T23:33:45Z

src/Container/RouteMiddlewareFactory.php

+use Psr\Container\ContainerInterface;
+use Psr\Http\Message\ResponseInterface;
+use Zend\Expressive\Middleware\RouteMiddleware;
+use Zend\Expressive\Router\RouteInterface;


Typo, should be RouterInterface

Xerkus · 2018-01-31T01:59:28Z

src/Middleware/RouteMiddleware.php

+     * Add a route for the route middleware to match.
+     *
+     * Accepts either a Route instance, or a combination of a path and
+     * middleware, and optionally the HTTP methods allowed.


This description is wrong, Route instance is not accepted

danizord · 2018-01-31T02:24:27Z

@weierophinney

Removing the MiddlewareFactory::path() method.

I'm 👍 for this. IMHO the MiddlewareFactory should focus only on creating middleware from string|callable|array, otherwise we would end up adding various other methods for each decorator like host() 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 the Application behaviors into separate participants and making them usable outside of Application. 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 renaming ApplicationRunner to HttpServer or something like that? Also, if you could extract it to a separate package, it would be really nice :)
Please rename MiddlewareFactory:: 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 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.

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

weierophinney · 2018-01-31T14:33:10Z

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 difficult within the Application class and ApplicationConfigInjectionTrait, 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.

The RouterInterface 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 the Application class handles (and, in this patch, the RouteMiddeware. Additionally, the routing methods we expose are around path-based routing specifically; routing could 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 the RouteMiddleware from this patch into the zend-expressive-router package, and maybe call it PathBasedRoutingMiddleware to indicate that it's specific to that purpose. It could even expose a method to add Route 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 v2 RouteMiddleware as-is, but renaming it as suggested above, and providing a factory for it.
A new minor release of zend-expressive that has RouteMiddleware 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 remove RouteMiddleware entirely, and consume the middleware from zend-expressive-router.

Thoughts on that approach, @danizord?

…onseGenerator This is what I'd intended originally.

This patch provides a ConfigProvider, which will allow offloading much of the initial configuration provided in the skeleton to the component itself.

Instead of using `path()` to create a `PathMiddlewareDecorator`, just `pipe()` both arguments to the `Application` instance. To make this work properly, the patch also does some additional logic to no longer create a `PathMiddlewareDecorator` if the the `$path` is `/`.

Also, discovered test case file did not reflect its class name.

This delegator factory decorates the application instance created by the callback by injecting a pipeline and routes based on configuration pulled from the container. The functionality is directly from the ApplicationConfigInjectionTrait, which we can deprecate and remove now that this functionality is in place.

This functionality is now in the ApplicationConfigInjectionDelegator.

This patch marks the two public injection methods in the delegator factory as static, allowing them to be used without instantiating the factory. It also correctly places the tests for the delegator factory, and moves required test assests in a tree beneath it.

This is better accomplished using `Zend\Stratigility\path()` + the factory.

The return type could potentially NOT be an `Application` in the situation that `$callback` returns something else.

…ssive-router v3 This patch updates zend-expressive to consume the `DispatchMiddleware` and `PathBasedRoutingMiddleware` classes from zend-expressive-router v3.0.0alpha1 and up. Doing so allows removal of those classes and related tests from this package, but requires: - `Application` now needs to depend on `PathBasedRoutingMiddleware` - `ApplicationConfigInjectionDelegator` needs to use the new classes when injecting routing and dispatch middleware. - tests needed updating to refer to the new classes.

This patch removes the `ApplicationRunner`, and replaces it with zend-httphandlerrunner's `RequestHandlerInterface` and emitters.

This patch updates the `MiddlewareContainer` and `MiddlewareFactory` to allow them to detect PSR-15 request handlers, and decorate them using zend-stratigility's `RequestHandlerMiddleware`. This ensures type-safety when used in either pipelines or with routed middleware, but also ensures we can use the full-spectrum of available handlers with the system. A new method was also added to `MiddlewareFactory`: `handler()`. This method accepts a `RequestHandlerInterface` instance and returns a `RequestHandlerMiddleware` instance decorating it, making it simple to use request handlers with the explicit functionality of zend-expressive-router's `PathBasedRoutingMiddleware` API.

Close #543

Removes the zendframework#543 entry indicating the changed `NotFoundDelegate`, instead adding a "Removed" entry for that class. Updates the "Changed" entry for the `NotFoundMiddleware` to indicate its new constructor arguments.

@Xerkus

This patch implements an idea [proposed by @Xerkus](zendframework/zend-expressive#543 (comment)): having the `pipeline.php` and `routes.php` files return _closures_, and updating `public/index.php` to capitalize on that in order to pass in arguments. In this particular case, we pass the application, middleware factory, and PSR-11 container instances as arguments. This provides developers with all the tools they should need in order to create the pipeline and/or routes.

@Xerkus

This patch implements an idea [proposed by @Xerkus](zendframework/zend-expressive#543 (comment)): having the `pipeline.php` and `routes.php` files return _closures_, and updating `public/index.php` to capitalize on that in order to pass in arguments. In this particular case, we pass the application, middleware factory, and PSR-11 container instances as arguments. This provides developers with all the tools they should need in order to create the pipeline and/or routes.

@Xerkus

This patch implements an idea [proposed by @Xerkus](zendframework/zend-expressive#543 (comment)): having the `pipeline.php` and `routes.php` files return _closures_, and updating `public/index.php` to capitalize on that in order to pass in arguments. In this particular case, we pass the application, middleware factory, and PSR-11 container instances as arguments. This provides developers with all the tools they should need in order to create the pipeline and/or routes.

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

geerteltink reviewed Jan 25, 2018

View reviewed changes

michalbundyra suggested changes Jan 26, 2018

View reviewed changes

michalbundyra reviewed Jan 26, 2018

View reviewed changes

froschdesign mentioned this pull request Jan 29, 2018

Question about marked a class as final zendframework/zend-stratigility#149

Closed

weierophinney added the WIP label Jan 29, 2018

michalbundyra mentioned this pull request Jan 30, 2018

PSR-15 progress tracker in Expressive modules #528

Closed

14 tasks

weierophinney changed the title ~~Refactor: extract middleware marshaling~~ Refactor: extract middleware marshaling, routing, and app running Jan 30, 2018

weierophinney added enhancement BC Break and removed WIP labels Jan 30, 2018

Xerkus reviewed Jan 30, 2018

View reviewed changes

Xerkus reviewed Jan 31, 2018

View reviewed changes

weierophinney added 13 commits February 5, 2018 16:12

Resolves issues reported by @webimpress

a9c950e

Renamed ServerFactoryErrorResponseGenerator to ServerRequestErrorResp…

a79cba3

…onseGenerator This is what I'd intended originally.

Provides ConfigProvider for library

6dd9cb5

This patch provides a ConfigProvider, which will allow offloading much of the initial configuration provided in the skeleton to the component itself.

Have NotFoundHandlerFactory pull ResponseInterface service

a5ed347

Also, discovered test case file did not reflect its class name.

Removes the ApplicationConfigInjectionTrait

b99b5be

This functionality is now in the ApplicationConfigInjectionDelegator.

Removes the path() method from the MiddlewareFactory

d0bfba1

This is better accomplished using `Zend\Stratigility\path()` + the factory.

Remove return type hint from ApplicationConfigInjectionDelegator

1d94484

The return type could potentially NOT be an `Application` in the situation that `$callback` returns something else.

Replace ApplicationRunner with zend-httphandlerrunner

9db3610

This patch removes the `ApplicationRunner`, and replaces it with zend-httphandlerrunner's `RequestHandlerInterface` and emitters.

weierophinney force-pushed the feature/stratigility-bc-breaks branch from 4d5ea20 to 4d5fb79 Compare February 5, 2018 22:13

Adds CHANGELOG entries for zendframework#543

6514d54

weierophinney merged commit 6514d54 into zendframework:release-3.0.0 Feb 5, 2018

weierophinney added a commit that referenced this pull request Feb 5, 2018

Merge branch 'feature/stratigility-bc-breaks' into release-3.0.0

789f772

Close #543

weierophinney mentioned this pull request Feb 6, 2018

NotFoundDelegate and NotFoundHandler #441

Closed

weierophinney deleted the feature/stratigility-bc-breaks branch February 6, 2018 16:05

geerteltink mentioned this pull request Feb 7, 2018

Remove disabling shared ResponseInterface #547

Merged

weierophinney mentioned this pull request Feb 7, 2018

Have pipeline and route files return closures zendframework/zend-expressive-skeleton#213

Merged

michalbundyra mentioned this pull request Feb 26, 2018

Avoid object creation by class name #572

Closed

harikt mentioned this pull request Apr 17, 2019

Calling PSR-17 middleware? radarphp/Radar.Adr#39

Closed

thexpand mentioned this pull request Mar 2, 2020

Remove call to removed method in documentation mezzio/mezzio-session#4

Merged

		@@ -9,6 +9,7 @@

		namespace Zend\Expressive;

		use Psr\Http\Server\MiddlewareInterface;

Refactor: extract middleware marshaling, routing, and app running #543

Refactor: extract middleware marshaling, routing, and app running #543

Conversation

weierophinney commented Jan 25, 2018 • edited Loading

Updates

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

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

froschdesign Jan 26, 2018 • edited Loading

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

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

michalbundyra commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Jan 29, 2018 • edited Loading

weierophinney commented Jan 29, 2018 • edited Loading

geerteltink commented Jan 29, 2018

weierophinney commented Jan 29, 2018

Xerkus commented Jan 29, 2018

weierophinney commented Jan 29, 2018

Xerkus commented Jan 29, 2018

geerteltink commented Jan 30, 2018

weierophinney commented Jan 30, 2018

ezimuel commented Jan 30, 2018 • edited Loading

weierophinney commented Jan 30, 2018

weierophinney commented Jan 30, 2018

Xerkus Jan 30, 2018 • edited Loading

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 Jan 31, 2018

weierophinney commented Jan 31, 2018

weierophinney commented Jan 25, 2018 •

edited

Loading

froschdesign Jan 26, 2018 •

edited

Loading

weierophinney commented Jan 29, 2018 •

edited

Loading

weierophinney commented Jan 29, 2018 •

edited

Loading

ezimuel commented Jan 30, 2018 •

edited

Loading

Xerkus Jan 30, 2018 •

edited

Loading