Added support for route-specific middleware pipe via configuration

[michaelmoussa]

The ApplicationFactory will check to see if the specified middleware is
an array before injecting it into the Route. If it is, it will attempt
to turn the contents of the array (which must be callable or retrievable
from the DI container) into a MiddlewarePipe, which it will inject
instead.

This change should be backwards compatible unless an application's functionality currently depends on failing if an array is specified in routes.[].middleware.

If acceptable, this would resolve #190

[danizord]

That prevents people from specifiyng [SomeClass::class, 'someMethod'] as callable middleware.

[weierophinney]

@michaelmoussa The point @danizord makes could be addressed by changing the condition to ! is_callable($spec['middleware']) && is_array($spec['middleware'])

[michaelmoussa]

Thanks. I'll make the change, and add a regression test for that condition.

[weierophinney]

@michaelmoussa build failures are due to CS checks.

[weierophinney]

Reviewing this, I wonder if it might make sense to do a "container-aware middleware pipe" which would lazy-load the middleware only when calling it. Looking into the Stratigility codebase, this would require both an alternate Next as well as Dispatch implementation — both of which would be easier to accomplish if we make a few properties protected and/or allow injection. I'll try to look into this this week.

[weierophinney]

@michaelmoussa In reviewing this for merge... I think the code above is in the wrong place! Everywhere else, we delay fetching from the container unless we're dispatching the middleware. As such, I think this needs to be moved to routeMiddleware().

However, for that to happen, a few other changes are necessary:

• RouteResult::getMatchedMiddleware() (and its accompanying property, $matchedMiddleware, and the $middleware argument for fromRouteMatch()) would need to adjust the typehint to string|array|callable.
• Route::getMiddleware() (and its accompanying property $middleware, and the $midleware argument to the constructor) would need to adjust the typehint to string|array|callable.
• Similarly, we also need to update Application::route() (this needs updating anyways with your change!)

The above bit that creates a middleware pipe would then move into routeMiddleware(), between the is_callable() and is_string() checks. We could either inline it as you've done here, or create a method that returns the MiddlewarePipe instance, and call that.

If you go that route, you could likely move the bits from is_string() up to return $callable() into a method that could be used by both methods, for marshaling the individual middlewares.

So, now the question: do you want to tackle those changes, or do you want me to do a PR against your branch? This approach would mean we don't have to worry about changes to Stratigility (as I think it's fine to fetch all the middleware for a given MiddlewarePipe at once in this circumstance), and I think the feature has some cool benefits; I'm already thinking about how we could have the ApplicationFactory also do global routed-middleware, into which the matched middleware would be injected. 😄

[michaelmoussa]

@weierophinney Makes sense. Why go through the effort of potentially expensive middleware retrievals from the container if you don't even know if those middlewares will be used in this request?

I think 3ecc821 takes care of this in the way you had in mind. If not, hopefully it's a good start. Let me know if I understood correctly and/or if this needs additional polish.

[michaelmoussa]

@weierophinney I noticed that the changes thus far would not (yet) allow this type of array configuration in the pre- and post-routing pipelines. Of course, one could just specify them separately, like this:

return [
    'middleware_pipeline' => [
        'pre_routing' => [
            [
                'middleware' => 'Foo',
                'path' => '/foobar',
            ],
            [
                'middleware' => 'Bar',
                'path' => '/foobar',
            ],
            [
                'middleware' => 'Foobar',
                'path' => '/foobar',
            ],
        ],
    ],
];

but something like this would be much more concise and potentially easier to read:

return [
    'middleware_pipeline' => [
        'pre_routing' => [
            [
                'middleware' => [
                    'Foo',
                    'Bar',
                    'FooBar'
                ],
                'path' => '/foobar',
            ],
        ],
    ],
];

Do you agree? Would you want to try to cram that into this PR as well or do it as a future PR hopefully still targeting 1.0.0?

[weierophinney]

Interestingly, this comment doesn't show on the website; I think because
its on code you changed in a later commit. Hopefully you'll see this
message!

Yes, I think we should add this into the current pull request, as it makes
the behavior and configuration syntax consistent.

On Tue, Nov 24, 2015 at 11:28 PM, Michael Moussa [email protected]
wrote:

In src/Container/ApplicationFactory.php
#192 (comment)
:

         $methods = (null === $methods) ? Route::HTTP_METHOD_ANY : $methods;

•        $route   = new Route($spec['path'], $spec['middleware'], $methods, $name);
  

•        if (!is_callable($spec['middleware']) && is_array($spec['middleware'])) {
  

•            $middlewarePipe = new MiddlewarePipe();
  

•            foreach ($spec['middleware'] as $middleware) {
  

•                $middlewarePipe->pipe(is_callable($middleware) ? $middleware : $container->get($middleware));
  

•            }
  

•            $spec['middleware'] = $middlewarePipe;
  

•        }
  

@weierophinney https://github.com/weierophinney I noticed that the
changes thus far would not (yet) allow this type of array configuration in
the pre- and post-routing pipelines. Of course, one could just specify them
separately, like this:

return [ 'middleware_pipeline' => [ 'pre_routing' => [ [ 'middleware' => 'Foo', 'path' => '/foobar', ], [ 'middleware' => 'Bar', 'path' => '/foobar', ], [ 'middleware' => 'Foobar', 'path' => '/foobar', ], ], ],];

but something like this would be much more concise and potentially easier
to read:

return [ 'middleware_pipeline' => [ 'pre_routing' => [ [ 'middleware' => [ 'Foo', 'Bar', 'FooBar' ], 'path' => '/foobar', ], ], ],];

Do you agree? Would you want to try to cram that into this PR as well or
do it as a future PR hopefully still targeting 1.0.0?

—
Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-expressive/pull/192/files#r45830835
.

Matthew Weier O'Phinney
[email protected]
https://mwop.net/

[michaelmoussa]

@weierophinney I imported those few classes and updated my new test case, but the same could be done all throughout the test class. I didn't, though, because it seemed out of scope for this PR. Should I do so anyway?

[weierophinney]

@michaelmoussa Rebase against latest master, and then use $this->injectServiceInContainer($this->container, <service name>, <service/value/mock>) to inject services in the container. (You'll see examples of this in the test class already). This will simplify your tests, and reduce the white noise, making the test setup easier to understand. If you need help, let me know.

[michaelmoussa]

All set. Nice helper! Really cleans up that test class.

[weierophinney]

@michaelmoussa You'll need to rebase again (sorry!!!), but also, please review this comment: #192 (comment)

Let me know if you want me to do a PR against your branch, or if you want to tackle the changes proposed.

[weierophinney]

@michaelmoussa — Also, regarding this:

I imported those few classes and updated my new test case, but the same could be done all throughout the test class. I didn't, though, because it seemed out of scope for this PR. Should I do so anyway?

Don't worry about it. Most of them are updated at this point, and we'll get the remaining ones in later PRs.

[weierophinney]

@michaelmoussa added this comment to a code line that was later changed, so I'm adding it to the discussion thread to ensure there's public awareness of the discussion:

@weierophinney I noticed that the changes thus far would not (yet) allow this type of array configuration in the pre- and post-routing pipelines. Of course, one could just specify them separately, like this:

return [
    'middleware_pipeline' => [
        'pre_routing' => [
            [
                'middleware' => 'Foo',
                'path' => '/foobar',
            ],
            [
                'middleware' => 'Bar',
                'path' => '/foobar',
            ],
            [
                'middleware' => 'Foobar',
                'path' => '/foobar',
            ],
        ],
    ],
];
but something like this would be much more concise and potentially easier to read:

return [
    'middleware_pipeline' => [
        'pre_routing' => [
            [
                'middleware' => [
                    'Foo',
                    'Bar',
                    'FooBar'
                ],
                'path' => '/foobar',
            ],
        ],
    ],
];

Do you agree? Would you want to try to cram that into this PR as well or do it as a future PR hopefully still targeting 1.0.0?

My response was:

Yes, I think we should add this into the current pull request, as it makes the behavior and configuration syntax consistent.

[michaelmoussa]

@weierophinney Shouldn't be too much extra work I hope so I'll try to tackle this soon. Does everything else look OK to you as-is?

[weierophinney]

Does everything else look OK to you as-is?

Absolutely; very pleased with this improvement/feature!

[ojhaujjwal]

+1 on the new type of array configuration in the pre and post-routing pipelines

[samsonasik]

@michaelmoussa documentation need to be updated too imo ;)

[weierophinney]

I can take care of documentation once the code is ready.
On Nov 25, 2015 8:12 PM, "Abdul Malik Ikhsan" [email protected]
wrote:

@michaelmoussa https://github.com/michaelmoussa documentation need to
be updated too imo ;)

—
Reply to this email directly or view it on GitHub
#192 (comment)
.

[michaelmoussa]

@weierophinney I think that should take care of it.

[danizord]

return $middlewarePipe($request, $response, $next);

[michaelmoussa]

Thanks @danizord - I've made the changes you indicated.

[weierophinney]

I added documentation as part of merging.

Additionally, @michaelmoussa, see be13b0b — that commit does a slight refactor of the routeMiddleware() method to re-use the marshalMiddleware() method you created when preparing a single middleware to dispatch.