Simplify the middleware pipeline configuration

[weierophinney]

This patch simplifies a number of features of Expressive:

• It simplifies the middleware pipeline configuration, by removing the pre_routing and post_routing keys; all middleware is piped at the same level of configuration, including routing.
• It splits the routing middleware into distinct routing and dispatch middleware. This allows replacing either process. This necessitates removal of autoregistration of the routing middleware; I consider this a feature, however, as it also ensures that you can form the middleware pipeline independent of when routes are added to the application.
• Because of the above, this patch also deprecates the route result observer system, as it's redundant. You can create "route result observers" as middleware that introspects the route result (which is injected as a request attribute following routing), and inject it in the pipeline immediately following the routing middleware. This simplifies route result observer registration as well, by providing exactly one way to accomplish it, and doing it in a portable fashion (middleware!).

I've noticed one potential issue with the change: configuration merging with indexed arrays means that we cannot guarantee order. This will potentially lead to issues if the middleware_pipeline is defined in multiple configuration files. To address this problem, this patch introduces a new middleware specification key, priority, which uses SplPriorityQueue semantics, and which can be used to guarantee the order in which middleware are piped to the application.

TODO

• deprecate pre/post routing keys
• remove auto-registration of routing middleware
• split routing middleware into routing + dispatch middleware
• deprecate route result observers
• update flow diagram
• solve middleware_pipeline ordering issues post-merge (implement priority/order key)

TODO post-merge:

• tag RC6 of zend-expressive.
• Update zend-expressive-helpers to modify the UrlHelper to remove the RouteResultObserverInterface implementation, and update the UrlHelperMiddleware to push the route result from the request, if present, into the UrlHelper. Once complete, a new version should be tagged.
• Update zend-expressive-router to mark the RouteResult(Observer|Subject)Interfaces as deprecated.
• Update zend-expressive-skeleton:
  • update the default middleware pipeline to follow that demonstrated in this patch.
  • update the zend-expressive-helpers version to pull in the new tag.
  • update the zend-expressive version to rc6.

[weierophinney]

Possible fix for #259 and #266; as noted; still a WIP.

[danizord]

the first time route() is called, the routing middleware is piped to the application. I'm considering removing that, and requiring developers explicitly do so

Yeah 👍, that would bring us a lot of flexibility. Btw it would be great if we have to register a Zend\Expressive\Container\ApplicationFactory::ROUTED_MIDDLEWARE, or Zend\Expressive\Container\ApplicationFactory::DISPATCHING_MIDDLEWARE as well, if we're going to split routing from dispatchig (#259).

Additionally, I noticed a potential issue with the change: configuration merging with indexed arrays means that we cannot guarantee order. This will potentially lead to issues if the middleware_pipeline is defined in multiple configuration files.

I'd suggest us to document that 3rd-party modules should not register middlewares in the pipeline themselves, but let the developer (consumer) register them in proper order.

[weierophinney]

if we're going to split routing from dispatchig

I'm not planning on doing that. That's something you can do in custom routing solutions.

[bakura10]

I'd suggest us to document that 3rd-party modules should not register middlewares in the pipeline themselves, but let the developer (consumer) register them in proper order.

Yeah, I think this will cause some hard to debug issues when merging config. Maybe a possible solution would be to use a specific key that would say: anything coming after is pre_routing until next token. For instance:

return [
   ApplicationFactory::PRE_ROUTING,
   // every middleware coming after is PRE_ROUTING until next token
   ApplicationFactory::ROUTING,
   // every middleware comign after is ROUTING until next token
   ApplicationFactory::POST_ROUTING
   // every middleware comign after is POST_ROUTING until next token
   ApplicationFactory::PRE_ROUTING
   // If an other module's config is merged, this oculd happen... but those will still be on Pre-routing
]

[bakura10]

Hhmm nevermind, I think I misunderstood the purpose of this PR, it's actually to remove this distinction I suppose...

[danizord]

I'm not planning on doing that. That's something you can do in custom routing solutions.

Why not? This would simplify a lot things, completely remove the need of "route result observers" and bring a lot of flexibility to framework users.

[weierophinney]

I'd suggest us to document that 3rd-party modules should not register middlewares in the pipeline themselves, but let the developer (consumer) register them in proper order.

I both agree and disagree.

On the one hand, requiring that users manually register the middleware gives greater control in your applications.

On the flip side, it's not just 3rd party modules, but your own configuration. As an example, you may want middleware registered only locally, for debug purposes: debug toolbars, error handlers, etc. If you put these in a *.local.php file, they'll be merged, but not necessarily in an order that will work correctly (e.g., debug toolbars may need to be in the pipeline prior to routing, while error handlers need to appear after). As such, requiring manual registration still doesn't solve the problem.

The solutions we're looking at currently:

• For config/autoload/ configurations, you can use the fact that globbing follows sort order, and have configurations that need to register middleware prior to the routing middleware occur higher in the sort list, and vice versa for error middleware.
• When using the expressive-config-manager, you can use the order of modules to a degree; however, this only works when a module defines middleware only for either pre or post routing purposes.
• Alternately, a "priority" flag in the middleware specification could be used by the application factory to sort them prior to attachment. This approach would address all strategies, but require more configuration by users. In most cases, it would be a single integer for each of pre, routing, and post, with more granularity if needed.

I'll get some commits in before long.

[weierophinney]

I'm not planning on doing that. That's something you can do in custom routing solutions.

Why not? This would simplify a lot things, completely remove the need of "route result observers" and bring a lot of flexibility to framework users.

Because the current implementation uses the results of routing to retrieve the middleware. If we separate them into distinct middleware, the changes required include:

• determining a way to pass the routing results to the dispatching middleware
• ensuring the dispatching middleware has access to the container
• ensuring the dispatching middleware has access to the same mechanisms for marshaling lazy middleware as the application

Not only does this result in complexity, it also requires documentation, and adds to the cognitive overhead of users trying to learn the system.

Keeping all of this internal to the Application instance is far simpler, and addresses the 80% use case; if you need the distinction, this patch will allow you to do that yourself.

[danizord]

@weierophinney I'm really sorry for being very insistent :( , but I think this is actually easier for framework users and would be easy to implement as well (I can even do a PR if you want).

• determining a way to pass the routing results to the dispatching middleware

$request->getAttribute(RouteResult::class);

• ensuring the dispatching middleware has access to the container
• ensuring the dispatching middleware has access to the same mechanisms for marshaling lazy middleware as the application

Just inject needed dependencies

$pipeline->pipe($cointainer->get(MiddlewareDispatcher::class));

Not only does this result in complexity, it also requires documentation, and adds to the cognitive overhead of users trying to learn the system.

Actually I'd say that it makes the workflow even more easy to understand as it becomes explicit. Also this is much more clean that route result observers IMHO.

Keeping all of this internal to the Application instance is far simpler, and addresses the 80% use case; if you need the distinction, this patch will allow you to do that yourself.

Yeah, actually it's code is simpler, but violates some SOLID principles and thus less maintainable, less replaceable, less flexible to changes...

Anyway sorry, this discussion is getting out of scope for this issue, let's go back to #259 :D

[RalfEggert]

I really like this approach. It really reduces the possible misunderstandings. What I like the most is that it will come within a RC6 and not with 1.1.0. Great work!

[weierophinney]

@danizord dcbcd09 accomplishes the split of the routing/dispatch responsibilities. I'm now working on the last patch for this PR, which will remove the route result observer system.

[weierophinney]

oops! forgot to update the routing middleware test expectations. /me heads off to fix those

[weierophinney]

@bakura10 We'll be doing an RC6 with these changes.

[weierophinney]

RouteMiddlewareTest assumptions and setups updated to reflect the code change to split the routing and dispatch into separate middleware.

[samsonasik]

I thinks features.md file need to be updated as well ( https://github.com/zendframework/zend-expressive/blob/master/doc/book/features.md )

[samsonasik]

-> Flow Overview diagram at features.md

[weierophinney]

@samsonasik Thanks for the heads-up; this is still a WIP, but I'd definitely missed that, and will include it in the patch.

[weierophinney]

@harikt —

Now I need to see how I can upgrade without much issues :) .

I wrote a migration guide as part of this PR. :)

[weierophinney]

@harikt That said, I need to update zend-expressive-helpers after this is merged; I didn't want to remove the route result observer functionality until after this was accepted.

[ZergMark]

@weierophinney I was thinking perhaps something along the below. It labels the areas correctly without the "first scan" problem of implying that each line is it's own type of middleware that comes before routing. Not a major issue of course.

[weierophinney]

@ZergMark Awesome! I'll make that update shortly!

[weierophinney]

@ZergMark Updated!

[ZergMark]

@weierophinney Thanks. Looks great.

[ezimuel]

@weierophinney it's ok for me, very nice feature!

[mtymek]

Why do we need a check if routing and dispatch middleware are registered? This adds extra logic for every call to pipe().
If this check is needed, I'd rather throw an exception to show the user that something is not properly configured.

[weierophinney]

It's to prevent double-registration; most likely you would never want to double-route/double-dispatch, as that's expensive to do in the same request.

We can investigate making these exceptions instead, but that's out of scope for this patch; the behavior added for the dispatch middleware was done to mimic the existing behavior of registering the routing middleware.

[mtymek]

Wow, I love this changes! 👍 👍 👍 👍

[danizord]

Maybe this recipe should be updated to suggest config merging instead?

[weierophinney]

For standalone, priority is a good rule-of-thumb. Integrated, you'd want it likely inside a middleware pipeline definition.

[danizord]

I'm talking about merging config for ViewHelperManager instead of using middlewares to update it.

[danizord]

Should be before routing middleware

[weierophinney]

Please send a PR with this change, with the one from your following comment.