Closed kocsismate closed 8 years ago
Another downside I've just become aware is that Condition
objects can't be reused: they have to instantiated as many times as we need them. However, in my opinion it isn't really difficult to fix this problem.
Hm, why not just dispatch the middleware if the condition is met? Why need to insert the middleware into the chain? I think it cause quite a few number of confusion.
One example: you add a middleware which formats the output as JSON if the path starts with /api, but the action serving the content does a subrequest, which is not under API, but harmony already contains the output formatting middleware. What would happen then?
Also, insert where? If you just dispatch the middleware when the condition is met you can be sure of the order.
Not to mention, that adhering the middleware chain AFTER building it is probably not a good idea, could cause some confusion as well.
Or am I missing something?
Thank you very much for your feedback! It was really helpful, but I only had time today to rework my implementation and answer your message. So now my first example looks like the following:
$harmony = new Harmony($request, $response);
$harmony
->addCondition(
new HttpMethodCondition(["POST"]),
function (Harmony $harmony) {
$harmony
->addMiddleware(new BodyParserMiddleware())
->addMiddleware(new CsrfMiddleware());
}
)
->addMiddleware(new FastRouteMiddleware($router))
->addMiddleware(new DispatcherMiddleware())
->addFinalMiddleware(new DiactorosResponderMiddleware(new SapiEmitter()));
$harmony();
Notable changes:
Condition
only evaluates a condition (it is not responsible anymore to invoke middleware)insert
method was removed, as it was quite uglytrue
, they are invoked "in a single batch". This means that middleware following the condition (FastRouteMiddleware
etc.) are only invoked when the middleware group has been executed. This solution is quite similar to Zend Stratigility's MiddlewarePipe
(https://github.com/zendframework/zend-stratigility/blob/master/doc/book/executing-middleware.md), but you are not limited to define conditions based on only paths. My current implementation can be seen here: https://github.com/woohoolabs/harmony/blob/b215dae63aad1ee159cd1f4eff1d51fcd715ebf1/src/Harmony.php#L264I am closing this issue because in my opinion, I fixed the issues mentioned. If you have any concerns or objections, feel free to reopen it.
After a discussion in https://github.com/php-http/http-kernel/pull/1, I'd want to add native support for dispatching middleware conditionally in Harmony 3.0.
After experimenting a little bit with the idea, I found that a really elegant way to enable this functionality is to use a
Condition
object - a special kind of middleware - which is responsible for invoking a callable if a certain condition is met. When invoked, this callable receives the current instance of the framework (Harmony
class) as its single parameter. This looks like the following:What exactly happens here? First, all the middleware are added to Harmony via the
add()
andaddFinal()
methods. Then, they are executed each after each: as the first middleware is aCondition
, it evaluates first whether the current HTTP method has side-effects. If so, theCsrfMiddleware
gets inserted via theinsert()
method right after the currently evaluated middleware (which is now theCsrfMiddleware
).Then the execution is continued either with
CsrfMiddleware
orFastRouteMiddleware
, and then withDispatcherMiddleware
. Finally, the HTTP response gets emitted byDiactorosResponderMiddleware
.With this implementation, you can even remove middleware at your own will or do other nasty tricks (however I am not sure it is a really good idea to do such things):
The advantages of this idea is that it is very easy to implement, it is trivial to define custom and complex conditions, and none of the unneeded middleware will be instantiated. The only downside I can see is that the API of the framework becomes a little bit more complex: we have both an
add()
and aninsert()
methods (or methods with more intention-revealing names).So now, I'd like to gather some feedback about my idea, so I would be happy if you could provide your possible use-cases or collect some pros and cons :)