Closed schnittstabil closed 7 years ago
I'm sorry, when is the delegate not delegating to middleware?
When there is no more middleware on the stack? That shouldn't happen if your stack is arranged properly - if that's not the case, that's a bug.
Even if it's some other component that, say, delegates to a controller, the component that delegates to the controller is still middleware in this context.
I thought this description was perfect as it was. The proposed description is unclear on the intended use or expected result - it's too abstract, even if there are edge-cases where the component you delegate to is ultimately something other than middleware, it's role in relation to middleware is still as middleware.
Even if it's some other component that, say, delegates to a controller, the component that delegates to the controller is still middleware in this context.
Yes, that's the reason. And I disagree, especially in this context: Middlewares are called Middlewares, because they are in the middle of something.
If I understood you correctly, according to your view, at least one of the following must hold:
ServerMiddlewareInterface extends DelegateInterface
DelegateInterface extends ServerMiddlewareInterface
DelegateInterface
at all and the ServerMiddlewareInterface
would look similar to this:interface ServerMiddlewareInterface
{
public function process(
ServerRequestInterface $request,
ServerMiddlewareInterface $next = null // <-- !!!
);
}
I disagree with 1. and 2., because of LSP. And, personally(!), I think 3. violates SRP, which is debatable.
Even if it's some other component that, say, delegates to a controller, the component that delegates to the controller is still middleware in this context.
Furthermore, a controller is a valid delegate – there is no need for an additional component.
Btw, the current FQIN is Interop\Http\Middleware\DelegateInterface
– and does not contain *Server*
.
Two suggestions:
Interop\Http\Middleware\ServerDelegateInterface
Interop\Http\ServerMiddleware\DelegateInterface
IMHO, 2. would be a good choice for the whole package…
Furthermore, a controller is a valid delegate – there is no need for an additional component.
@schnittstabil that completely depends on your controller definition/concept/interface - it may or may not be a valid delegate itself; it's very likely not, as developers/frameworks tend to dream up all sorts of creative controller concepts, e.g. with parameter resolution, value-binding, result abstraction, filters, there are any number of concepts and possible definitions.
More importantly, the concept of controllers is beyond the scope of this specification, so, as mentioned, even if the delegate is delegating to something other than middleware, such as a controller, the role of that component as it pertains to this specification is still as middleware.
We're not interested in what else the component being delegated to might also be, e.g. in a different domain. It's role in the context of the middleware domain is as middleware.
Personally, I think DelegateInterface::process
needs to be renamed to __invoke
. After implementing https://github.com/equip/dispatch I found this to be silly:
return $delegate->process($request);
It should really just be:
return $delegate($request);
But to be clear, I do not think we should change the middleware signature to callable
, it should remain as is.
But to be clear, I do not think we should change the middleware signature to callable, it should remain as is.
IMO changing the method-name to save like 8-9 keystrokes when dispatching the delegate is silly - you still won't be able to use callable
which the type-hint prevents, so you still need something like this in practically every implementation, a dumb wrapper around what is most likely internally a callable
anyhow.
Frankly, we've debated the pros and cons of this to death.
An interface with an ordinary method-name is a totally idiomatic thing - and doesn't cause anyone any problems, beyond typing a few extra characters. Stuffing __invoke()
in an interface is pretty unorthodox.
I'm strongly suggesting we close this can of worms again, right now, please.
controller […] may or may not be a valid delegate itself … It's role in the context of the middleware domain is as middleware.
@mindplay-dk Sorry, I think you contradict yourself. May you please take a stand on the following question:
Do you believe that all Delegates are Middlewares?
May you please take a stand on the following question:
Do you believe that all Delegates are Middlewares?
No, I don't.
The delegate is used to manage a stack, queue, collection, whatever, of middleware, but is not itself middleware. The idea when calling it from within middleware is to see if some other middleware can produce a response.
To reiterate: a delegate may compose middleware, and is guaranteed to return a response, but is not itself middleware; it delegates to the middleware it composes.
Do you believe that all Delegates are Middlewares?
No, they are delegates to middleware.
Even if the thing being ultimately dispatched is a controller or something else, the delegate is always a middleware delegate in the context of middleware.
Controllers are not a concept within this domain. The same object can have more than one role in different domains - that's basically what interfaces are for. In the context of the middleware domain, the delegate is to middleware.
I don't know how to make this any clearer.
Do you believe that all Delegates are Middlewares?
No, delegates are not middleware. They are the interface between the middleware container and the middleware itself:
Middleware is stored in a container, which is executed by a delegate the container creates.
https://github.com/equip/dispatch/blob/master/src/Stack.php#L58 https://github.com/equip/dispatch/blob/master/src/Delegate.php
I'm :-1: on this overall.
While I recognize that with the removal of MiddlewareInterface
we're now only dealing with server-side middleware, one of the original ideas was to create a delegate that could, potentially, mix and match client- and server-side middleware. Client-side middleware may be expected to return a promise, but that paradigm may make sense only in asynchronous workflows; I can definitely see a potential for synchronous workflows that would continue to produce a response. As such, changing the signature of DelegateInterface
at this time to only accept server-side requests potentially removes this possibility.
I definitely still see the argument against using a delegate that only typehints on RequestInterface
— indeed, I made those arguments myself. However, I'm also seeing that, in practice, it's a basically moot point: a server-side middleware stack will be slinging around a server-side request, and IDEs will not get confused in most cases, as the DelegateInterface
is something that will be implemented in libraries, but only consumed in applications; as such, the typehint is the only salient point, and ServerRequestInterface
fulfills that already. If a type mismatch does occur (e.g., client-side middleware passing a brand new non-server-side request to the delegate, which in turn dispatches server-side middleware), PHP will raise an error, and the user will know they need to write a wrapper around that specific client-side middleware, as it's mis-behaving.
tl;dr: DelegateInterface
may have use cases beyond server-side requests, and the current signature both supports that, and is not contradictory with server-side middleware runners in real-world practice.
@weierophinney what about renaming process()
to __invoke()
?
Thank god, we all agree that they are different.
The delegate is used to manage a stack, queue, collection, whatever, of middleware … the delegate is always a middleware delegate in the context of middleware. … They [delegates] are the interface between the middleware container and the middleware itself
@weierophinney, @mindplay-dk, @shadowhand: I believe you all have had only your very own concrete implementation in mind. Take the following example using Equip Dispatch:
use Equip\Dispatch\Stack;
$default = function (RequestInterface $request) {
return new Response();
};
$stack = new Stack([
new FooMiddleware(),
// ...
], $default);
$request = ServerRequest::fromGlobals();
$response = $stack->dispatch($request);
To me, $default
is the simplest delegate I can think of. Moreover, the Middlewares are in the middle of the process – and $default
is at the very end and not in the middle.
tl;tr Therefore, DelegateInterface::process
should not claim to call the next middleware, that would be implementation specific.
Btw, @mindplay-dk You started to talk about controllers, not me – I don't know what kind of controllers you have in mind, but to me they were only one example of many.
@weierophinney what about renaming process() to __invoke()?
Sorry I forgot to answer that one earlier, @shadowhand !
I'd personally prefer the process()
(or other reasonable name) method. My rationale is based on implementing http-middleware in Stratigility and Expressive, and setting them up to work interchangeably with double-pass (now considered legacy).
Our Next
implementation has always been callable, but requires both a $request
and $response
argument. If http-middleware were to also use __invoke()
, we'd need to make $response
optional, and then we have a potential problem: if we do not receive a response, but the next middleware in the stack is double-pass, we will error.
By having separate methods, we were able to adapt our Next
implementation to work with either style, giving us forwards compatibility with http-middleware, but backwards compatibility with existing double-pass middleware. Of course, the goal is to eventually only support http-middleware signatures, but this gives us a migration path.
Using __invoke()
makes it very tempting to typehint on callable
as well, instead of the interface, which, for me, is an argument for keeping a discrete method: encouraging using the typehints. This will be particularly relevant if projects follow in the footsteps we're taking, by adapting callable middleware in a decorator. Having the typehint makes determining the signature far easier, particularly when adapting multiple middleware styles.
In terms of actual usage, I do not find that using invoke is necessarily faster/easier to type than having a dedicated method. Additionally, I find mocking easier with a dedicated method.
@schnittstabil I really don't understand your argument. I think you're arguing that a DelegateInterface
instance could be middleware that does not accept a delegate, and thus that the DelegateInterface
argument should be nullable.
I personally do not agree. I'd prefer a single signature. If the $delegate
argument is nullable, middleware that needs to use it MUST validate the value before usage. If we make it required, the final, innermost middleware can simply ignore the argument and return a response. The value to having it non-null is type-safety for all other layers of the application.
If this is not your argument, my apologies. That said, if it's not, can you please clarify?
@schnittstabil I really don't understand your argument. I think you're arguing that a DelegateInterface instance could be middleware that does not accept a delegate, and thus that the DelegateInterface argument should be nullable.
Oh, no! My https://github.com/http-interop/http-middleware/pull/29#issuecomment-260214510, was a reply to @mindplay-dk's response – it turns out that it was a misunderstanding. I fully agree with you and came to exactly the same conclusion.
The discussion started, because @mindplay-dk wanted to know:
I'm sorry, when is the delegate not delegating to middleware?
Which refers to the following change, if I'm not wrong again:
interface DelegateInterface
{
/**
- * Dispatch the next available middleware and return the response.
+ * Process an incoming server request and return a response.
*/
public function process(…);
}
@weierophinney okay, I can agree with that. The problem I have with process()
is two-fold:
return $delegate($request);
I am curious about your comment about the Next
implementation... are you really pursuing a plan that uses the same delegate implementation for both double-pass and single-pass?
@schnittstabil I think the correct docblock description would be:
Delegate the request to the next available middleware.
If no middleware is available an empty response MUST be returned.
At least this is my operating assumption with equip/dispatch
.
Delegate the request to the next available middleware, or return an empty response if none is available.
I don't think that's generally true. Sometimes, for very, very simple applications, we don't need a stack at all, and the delegate does not return an empty response:
// contact.php
$middleware = new Middlewares\ContentType();
$request = ServerRequest::fromGlobals();
$response = $middleware->process($request, new class implements DelegateInterface {
public function process(ServerRequestInterface $request)
{
// send an email…
// create the response…
$type = $request->getHeaderLine('Accept');
$response = new Response();
if ($type === 'text/html') {
$response->getBody()->write('<p>Thanks for your message</p>');
} elseif ($type === 'application/json') {
$response->getBody()->write(json_encode(['message' => 'Thanks for your message']));
}
return $response;
}
});
Of course, at some later point we may want to use more than one middleware, or routing, or whatever. But, there are many use cases out there, which are simple and will ever be such simple, e.g. some IoT applications.
@schnittstabil I think we disagree here. What you are expressing is a middleware, not a delegate. Ideally the delegate default always returns an empty response that is processed by middleware, even if there is only one. (In your case, why have middleware at all?) Mostly because the delegate default should use a factory to create the response, so that swapping PSR-7 implementation is possible:
$responseFactory = $container->get('ResponseFactory'); // or whatever
$default = function (RequestInterface $request) use ($responseFactory) {
return $responseFactory->createResponse();
};
// ... etc
What you are expressing is a middleware, not a delegate. Ideally the delegate always returns an empty response that is processed by middleware, even if there is only one.
An interesting point of view – likely the reason for all those misunderstandings.
Mostly because the delegate should use a factory to create the response, so that swapping PSR-7 implementation is possible
IMO, there is no difference between injecting a factory into Middleware or a Delegate, but maybe I don't get the point.
If I understand you correctly, then you suggest:
$default = function (RequestInterface $request) {
return new Response();
};
$stack = new Equip\Dispatch\Stack([
// middleware, which implements content negotiation:
new Middlewares\ContentType(),
new class implements ServerMiddlewareInterface {
public function process(ServerRequestInterface $request, DelegateInterface $delegate)
{
// alternative 1: create an empty response using $delegate
$response = $delegate->process($request);
// alternative 2: create an empty response using a response factory
$response = $responseFactory->createResponse();
//…
// generate content:
$response->getBody()->write('<p>Thanks for your message</p>')
//…
return $response;
}
}
], $default);
$response = $stack->dispatch(ServerRequest::fromGlobals());
To be honest:
ServerMiddlewareInterface
? $delegate
is never used by the last one.$default
? If the last middleware calls $default
, then it is a bug of the application.Furthermore, I've never seen that before, e.g.
def index(request):
return HttpResponse("Hello, world. You're at the polls index.")
class UserController extends Controller
{
/**
* Store a new user.
*
* @param Request $request
* @return Response
*/
public function store(Request $request)
{
$name = $request->name;
//
}
}
$app->get('/hello/{name}', function ($request, $response, $args) {
// ! this is not a middleware ! - there is no $next
$response->write("Hello, " . $args['name']);
return $response;
});
The main reason I've never seen that before might be, because most frameworks don't distinguish Middlewares and Dispatcher. However, comparing the examples above: Django views and Laravel controllers implement the DelegateInterface
to some extent – in my opinion, that cannot be a coincidence!
@shadowhand —
I am curious about your comment about the Next implementation... are you really pursuing a plan that uses the same delegate implementation for both double-pass and single-pass?
Yes!
We implemented and released it already in zend-stratigility 1.3:
In there, we have the legacy __invoke()
, but also the http-interop process()
. There's a small amount of logic duplication between them, but they dispatch the middleware based on the middleware type. We allow the two types to intermingle by storing a "response prototype" in the Next
instance, so that if a non-interop middleware is delegated to from interop middleware, we have a response to pass to it. (In 2.0, that prototype gets pushed into a decorator for the legacy middleware instead, and Next becomes far simpler.)
While we could potentially do this all in __invoke()
, it makes for quite a few more paths through that method, making maintenance harder, and requires we change our signature to make the second, $response
argument, optional. This is not a BC break, per se, but definitely a bit more difficult to support. Having the separate method meant we could segregate the functionality, and made the transition to the proposed 2.0 version essentially just a matter of removing the __invoke()
implementation and inlining the Dispatch::process
logic.
@schnittstabil oh boy... I wrote "delegate" twice when I meant "default". The correct thing to do here is:
If your middleware does not terminate early, allow the delegate to return the response. If the delegate has reached the end of the stack, it will execute the $default
to get an empty response. Use the stream factory to create a body and attach it to the response. (Do not use stream writing! This is buggy!)
// omitted type hints for clarity
public function process($request, $delegate)
{
$response = $delegate->process($request);
$body = $this->streamFactory->createStream('<h1>It works!</h1>');
return $response->withBody($body);
}
OR If your middleware does terminate early, generate a response in the middleware and return it without delegating:
public function process($request, $delegate)
{
if (should_terminate($request)) {
return $this->responseFactory->createResponse(400);
}
return $delegate->process($request);
}
I get that, in the case with $default
, you're not strictly delegating to a middleware component - but I maintain that the role of even a simple function consisting of return new Response()
is still acting as middleware in it's relation to the middleware component that calls it.
What else that component may be, in addition to acting as middleware in this context, is completely outside the scope of this specification - it's implementation details and personal choices in the middleware stack.
Let's stop debating and suggest alternatives to these two lines of documentation instead?
For example:
Delegates control from the current middleware to a different component.
Or:
Pass control to a different component and return the Response generated by it.
Or suggest something?
But don't try to make it so neutral-sounding that it has no meaning to anyone trying to learn about the role of the interface and method in the context of the middleware domain.
@shadowhand
Delegates don't process anything. Their entire purpose is to blindly pass the request to the next available middleware, or return a response if the end of the stack is reached.
That describes how it's implemented - the method should describe the effect of calling that method, what it does internally leads to the processing of the request, and that's all the client code should care about.
As per the first point, there is only one viable public method. Hence it seems preferable to write:
return $delegate($request);
- If you tried to use
__invoke()
for every interface containing only one public method, you'd never be able to implement more than one interface in the same class- It's not a given that the delegate implementation has only one public method, only that it's the only public method offered via this interface
- Your preference ultimately is based on a desire to save 9 keystrokes.
We've had debated this subject to death already. Why are we wasting time on this again? It's a very minor detail of no substantial consequence whatsoever, so can we please use a safe, plain, ordinary one-method interface and not run the risk of blocking or conflicting with something else by reserving a magic method? 9 keystrokes really is not enough of a reason to do that.
__invoke()
has no practical advantage over a regular, named method, in this context, where you're going to type-hint against the interface itself anyhow - __invoke()
does not become interchangeable with callable
in this context, so there really is not real practical benefit to resorting to magic here. None.
@mindplay-dk I forgot that part of the reason we named the delegate method process
was to prevent a class from implementing both Middleware and Delegate interfaces, as that would be a serious mistake in interpretation. Thanks for reminding me.
Based on the feedback I am closing this PR. The current Delegate
implementation is fine and modifying the accepted request type has no distinct value nor will it interfere with async middleware.
What else that component may be, in addition to acting as middleware in this context, is completely outside the scope of this specification - it's implementation details and personal choices in the middleware stack.
@mindplay-dk I fully agree on that, and never said that the specification should mention controllers or similar. But, as you said, the same object can have more than one role in different domains.
To me DelegateInterface
is not an internal-only interface of the middleware PSR. Instead, in my example above, I claim $default
fulfills the DelegateInterface
to some extend. But, to make it explicit, let me introduce a more concrete version:
$default = new class implements DelegateInterface {
public function process(ServerRequestInterface $request)
{
return new Response();
}
};
In this example, $default
should act as the default handler for end of stack (Equip Dispatch jargon), i.e. a final handler (zend-diactoros jargon).
I claim that $default
fulfills the DelegateInterface
contract:
$default->process($request)
it dispatches/processes an incoming server request and return a response – that should be obvious too.Furthermore, I claim the same for Django views and Laravel controller methods as well. In these frameworks they play the role of views or controller methods respectively. But in the context of the middleware stack, they also play the role of delegates.
@shadowhand IMO, things get really messy with your suggestion, e.g. with Stratigility. To be honest, I don't like how some things are solved with Stratigility – but ok, Stratigility is not the only one allowing the following (notably expressjs allows similar).
It is a common pattern in those applications to register multiple middlewares/handlers per route:
$app = new MiddlewarePipe();
$server = Server::createServer($app, …);
// non admin
$app->pipe('/', function ($request, DelegateInterface $delegate) {
if ($request->getQueryParams()['name'] == 'admin') {
return $delegate->process($request); // [1]
}
$response = $delegate->process($request); // [2]
$body = $this->streamFactory->createStream('<h1>Hi non admin!</h1>');
return $response->withBody($body);
});
// admin
$app->pipe('/', function ($request, DelegateInterface $delegate) {
$response = $delegate->process($request);
$response = $response->withHeader('secret', 'unicorns are awesome');
$body = $this->streamFactory->createStream('<h1>Hi admin!</h1>');
return $response->withBody($body);
});
$server->listen(new NoopFinalHandler());
[1]
and [2]
delegate to the next middleware, which is the admin one, not the NoopFinalHandler
– ugh! Hence, $delegate->process($request)
is not suited to to get an empty response – only $responseFactory->createResponse()
leads to maintainable code.
Furthermore, I see no problem with $responseFactory
, it is framework specific and does not have to implement Psr\Http\Message\ResponseFactoryInterface
, e.g.:
class MyResponseFactory
{
function createResponse()
{
return new TextResponse();
}
}
@schnittstabil none of your last post made any sense to me... maybe because it is specific to Stratigility, which has never been comprehensible to me.
In summary:
$app->pipe('/', function ($request, DelegateInterface $delegate) {
return $delegate->process($request); // [1]
});
$app->pipe('/', function ($request, DelegateInterface $delegate) {
$response = $delegate->process($request);
$body = $this->streamFactory->createStream('Hello World');
return $response->withBody($body);
});
If you visit /
, then Stratigility will output Hello World
(because of [1]
).
Thus, $delegate->process($request)
is unsuited to get an empty response:
If I need an empty response, I have to create one explicitly (new …
) or call a factory – even in the controllers are middlewares world.
ServerRequestInterface
is the right thing@weierophinney wrote:
a server-side middleware stack will be slinging around a server-side request, and IDEs will not get confused in most cases, as the
DelegateInterface
is something that will be implemented in libraries, but only consumed in applications;
Despite I don't agree about the only implemented in libraries argument, the real issue is much bigger.
As a middleware-stack implementer, I need to create a DelegateInterface
instance which calls the next ServerMiddlewareInterface
instance, e.g. from Equip\Dispatch\Delegate
:
class Delegate implements DelegateInterface
{
public function process(RequestInterface $request)
{
//…
return $this->middleware[$this->index]->process($request, $this->nextDelegate());
}
}
This means, we must call ServerMiddlewareInterface::process
with a RequestInterface
instance in one way or another, however a ServerRequestInterface
instance is expected.
Currently, that works nicely: The type system of PHP is too dump and does not complain about it, but relying on that is not future proof, e.g. see PHP RFC: Property type-hints. In my opinion, it would be a shame if a standard would force implementers to trick the PHP type system someday.
Moreover, of course, just to be clear, we can not do:
class Delegate implements DelegateInterface
{
public function process($request)
{
}
}
That throws a PHP Fatal error, already today:
Declaration of Delegate::process($request) must be compatible with
Interop\Http\Middleware\DelegateInterface::process(Psr\Http\Message\RequestInterface $request)
You'll also get the fatal error if DelegateInterface
invokes a ServerMiddlewareInterface
instance with only a RequestInterface
. Further, your stack trace will help you know what called the delegate, and thus which middleware is improperly creating a non-server request prior to calling the delegate.
Using the looser RequestInterface
on the delegate, which should typically only pass on the given request to the middleware to which it delegates, allows us to have middleware with different signatures at a later point. Restricting to ServerRequestInterface
denies that possibility.
The differences between theses approaches are about where and when errors are thrown:
Using the looser
RequestInterface
on the delegate, …, allows us to have middleware with different signatures at a later point. Restricting toServerRequestInterface
denies that possibility.
That can be fixed:
// Client
interface DelegateInterface
{
public function process(RequestInterface $request);
}
interface MiddlewareInterface
{
public function process(RequestInterface $request, DelegateInterface $delegate);
}
// Server
interface ServerDelegateInterface
{
public function process(ServerRequestInterface $request);
}
interface ServerMiddlewareInterface
{
public function process(ServerRequestInterface $request, ServerDelegateInterface $delegate);
}
Hooray, type-safe client and server middlewares :tada:
abstract class Middleware
{
public function foo(ResponseInterface $response)
{
// client and server code
}
}
class ClientMiddleware extends Middleware implements MiddlewareInterface
{
public function process(RequestInterface $request, DelegateInterface $delegate)
{
return $this->foo($delegate->process($request));
}
}
class ServerMiddleware extends Middleware implements ServerMiddlewareInterface
{
public function process(ServerRequestInterface $request, ServerDelegateInterface $delegate)
{
return $this->foo($delegate->process($request));
}
}
Only the modification of $request
s seems to be a small drawback. But, as you say:
You'll also get the fatal error if
DelegateInterface
invokes aServerMiddlewareInterface
instance with only aRequestInterface
.
With the interfaces above however, this can now be checked statically.
I forgot that part of the reason we named the delegate method process was to prevent a class from implementing both Middleware and Delegate interfaces
@shadowhand perhaps add this to the meta section?
Fixes: