Rename DelegateInterface::next() to process()

[oscarotero]

According with #15

[schnittstabil]

I'm so :-1: on this. I've tried to figure out why you want to have the same name process.

The only locations I've found:

@mindplay-dk wrote at https://github.com/http-interop/http-middleware/issues/15#issue-173641000

I don't think using the name process() in the middleware-delegate would be an issue? I can't imagine a case where a middleware delegate implementation is also a middleware component, can you?

I can.

@shadowhand wrote at https://github.com/http-interop/http-middleware/pull/29#issuecomment-260838130

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.

A "serious mistake" – really? Furthermore, do you think middleware providers must be protected from themselves?

The last location #26:

Both the middleware and delegate interface define a process method to prevent misuse of middleware as delegates.

If a middleware was used as a delegate the entire middleware stack would end up recursive, instead of piped. The implementation of delegate should be defined within middleware dispatching systems.

Recursion is not bad, only endless loops are, and you need to mess up your setup to achieve that – Nobody can protect stupid developers to do stupid things. Even more, naming both methods process can not prevent setups with endless loops.

@mindplay-dk wrote at https://github.com/http-interop/http-middleware/pull/29#issuecomment-260466570

The same object can have more than one role in different domains - that's basically what interfaces are for.

I fully agree, but the name unification makes it impossible create those objects.

But, lets talk about code:

class DefaultResponseHandler implements DelegateInterface, ServerMiddlewareInterface
{
    /**
     * Default/final handler
     */
    public function __invoke(RequestInterface $request) {
        $contentType = … $request-> …;
        $response = new Response();
        switch ($contentType) {
            case "text/html":
            case "application/json":
                return $response->withHeader('content-type', $contentType.'; charset=utf-8');
            default:
                return $response->withHeader('content-type', 'text/plain; charset=utf-8');
        }
    }

    /**
     * DelegateInterface::dispatch
     */
    public function dispatch(RequestInterface $request) {
        return $this->__invoke($request);
    }

    /**
     * ServerMiddlewareInterface::process
     */
    public function process(ServerRequestInterface $request, DelegateInterface $delegate);
        return $this->__invoke($request);
    }
}

The above pattern is called Class Adapter pattern and is well known.

Thus, DefaultResponseHandler can play different roles in different frameworks:

• It can play the role of the default handler or a middleware in Equip\Dispatch\Stack.
• It can play the role of the final handler or a middleware in Zend\Diactoros\Server.
• …
• But most important, it can play the role of a middleware in middleware stacks, which don't utilize final/default handlers at all:

$stack = new Stack([
   new ControllerMiddleware(),
   new DefaultResponseHandler()
]);

$stack->dispatch($request);

I fully agree with @alamin3000, naming both process is a huge design mistake. :worried:

[shadowhand]

Let's consider this diagram:

A stack is a composition of middleware. The stack creates a delegate that references the stack. For each middleware in the stack, the stack calls the middleware, passing the delegate, which increments the stack and continues until the entire stack is processed. Data flows one way: from the stack, to the middleware, back to the stack. (Note that dependency arrows are reversed from execution flow.)

Therefore, if would be a violation of SRP for the delegate to also act as a middleware, because a delegate should only be aware of the reference to the stack, and a middleware should have no awareness of the stack.

A stack/dispatcher should contain a delegate that makes sense for that stack/dispatcher. A middleware doesn't care about the delegate, because it is only a way to pass control back to the stack. Mixing the two concerns is weird and, as your example shows, creates convoluted code.

[mindplay-dk]

@shadowhand I don't disagree with any of that - but, regardless, I don't see a problem with naming the delegate and middleware methods differently? (It might make the code slightly easier to read, since you won't visually confuse calls to middleware with calls to a delegate.)

However...

@schnittstabil If the methods should be named differently, what's the rationale for the choice of method-names? process and dispatch seem like arbitrary choices. If they have different names, the difference needs to convey relevant meaning in each case.

[schnittstabil]

@shadowhand AFAIK, according to UML and your description, all your arrows must be reversed.

Therefore, if would be a violation of SRP for the delegate to also act as a middleware, because a delegate should only be aware of the reference to the stack, and a middleware should have no awareness of the stack.

I must admit that my example above is not very well designed. But your SRP argumentation is flawed. SRP states:

A class should have only one reason to change.

However, I don't want to go any further into that. I agree that standards should not encourage bad design, but paternalism of implementers is going too far – especially if the readability suffers from it.

process and dispatch seem like arbitrary choices.

:+1: And I think the same holds for DelegateInterface, as @shadowhand wrote:

a middleware should have no awareness of the stack.

I agree with that, but current wording contradicts it and leads to a hard to grasp oddity:

1. the $delegate parameter is a delegate of the stack
2. and the middleware delegates the remaining computation to it

Thus, what a middleware essentially does is $delegate->delegateComputation(), and the first delegate means something quiet different than the second delegate :confused:

Even more, $delegate only provides a single method (process at the moment) which takes a request and returns a response, hence the middleware doesn't need to know that it is dealing with a delegate of a stack.

Therefore, I believe it is entirely sufficient for middlewares to deal with request executors or request handlers.

[alamin3000]

@weierophinney First of all, you are referring to a good looking diagram that is inaccurate. And your description is also wrong. I think you are confusing with middleware pipe (which is usually queue, not stack... but whatever) and middleware itself. And there are various ways to handle this middleware cycle so using one diagram to justify everything is not cool.

[schnittstabil]

Maybe:

interface ServerMiddlewareInterface
{
    /**
     * Process an incoming server request and return a response, optionally delegating
     * to the next request handler.
     *
     * @param ServerRequestInterface $request
     * @param RequestHandlerInterface $next
     *
     * @return ResponseInterface
     */
    public function process(ServerRequestInterface $request, RequestHandlerInterface $next);
}

interface RequestHandlerInterface
{
    /**
     * Process a request and return the response.
     *
     * @param RequestInterface $request
     *
     * @return ResponseInterface
     */
    public function process(RequestInterface $request);
}

Yeah, I know, 2x process :wink:

[alamin3000]

As for name, guys this will not settle with everyone... because we can come up with so many different names- process, execute, dispatch, delegate...

But none of them really makes sense for the middleware. Because once they are in the pipe, they only have one function to do and therefore only one function will be invoked by the cycle. And if the middleware has other functionalities, they will be called internally, not externally. That's why __invoke makes so much sense. It's a piece of code that needs to be run. Not processed or delegates or what not.

To say a middleware has method process(...) you are saying it can have other external functionalities such as render(), execute() or what not. But who is going to call those from outside? From that same pipe? That's why other platform (C#/.NET) uses Invoke and Javascript uses simple closure and our previous interface with __invoke/callable made little bit more sense.

This also eliminate this war with name.

[schnittstabil]

I would prefer __invoke too. But I don't know the cons which were already discussed at the ML – so I haven't decided yet…

[alamin3000]

Okay reading from the Readme I find this argument against __invoke:

In addition, classes that define invoke can be type hinted as callable, which results in less strict typing. This is generally undesirable, especially when the invoke method uses strict typing.

whether a class can be type hinted as callable or not isn't the issue. Because application (or Middleware Pipe) will NOT accept callable. It will accept ServerMiddlewareInterface, which will have invoke defined strictly `invoke(ServerRequestInterface... ) : ResponseInterface. So middleware pipe still can have method strongly typed-pipe(ServerMiddlewareInterface)`. It doesn't matter if the method in the interface is good old __invoke or not, it still refers by the interface. Therefore type hint issues are gone. At this point it doesn't matter if someone tries to use the middleware else where as a callable.

By using __invoke so, You still make the middleware as one single unit of work. Does ONE thing. So you still call the middleware as invokable: $middleware($request, ...)

And second argument:

In addition, it allows compatibility with existing middleware that already defines an __invoke method.

Well, an eventual rewrite/refactor would need to happen to fully support new name process any way....

[schnittstabil]

In addition, it allows compatibility with existing middleware that already defines an __invoke method.

That is the worst argument in discussions about standards. There will always be an existing implementation which already defines __invoke, process, execute, dispatch, delegate or whatever. Furthermore, the adapter pattern is well known for decades.

Even more, it is absolutely reasonable for a standard to use the most widely accepted name/approach.

One of the last standards I've tracked was Promises/A+ and the official ECMAScript 2015 Promises. Both use then as method name, but differ in some details, hence the new official ECMAScript standard was a BC for all Promises/A+ libraries – Nobody cried about the wording and backward compatibility issues. And today all projects move to the new official standard.

[weierophinney]

Even more, it is absolutely reasonable for a standard to use the most widely accepted name/approach.

As somebody who has actually attempted to incorporate http-middleware into an existing middleware library, I very much disagree.

Let me demonstrate why, first with the delegate.

If we define DelegateInterface such that it defines __invoke() instead of process(), we run into issues with existing double-pass middleware. If that middleware delegates, it will look like this:

function (ServerRequestInterface $request, ResponseInterface $response, callable $next) {
    // do some work
    // delegate:
    $response = $next($request, $response);
    // do more work
    return $response;
}

The problem is subtle: the $response argument to $next is silently dropped by the delegate if it implements DelegateInterface. This may seem like a non-issue, but if there is any middleware later in the stack that implements double-pass, the response argument has disappeared. That said, the delegate should likely be memoizing a response at some point for this very situation, so we can skip over that case.

But what about cases where ServerMiddlewareInterface implementations occur in such a stack? Those now call the delegate with only the request:

$response = $next($request);

And this is where things get dicey: the delegate signature differs from double-pass signatures, which require the response argument. These delegates may be implementing an interface already; we now have a situation where they must choose to either change their existing interface (BC break!) or continue without adopting http-middleware.

When I looked at this in Stratigility, the issue wasn't quite as complex: our Next class did not implement an interface, and we could potentially make the $response argument optional. But we're left with a very complex problem: just because we're called with only a single argument does not mean the next middleware in the stack is not going to need the $response argument.

The next issue is with middleware implements. While you feel that BC is not an issue, as somebody who has implemented BC breaks in major versions in the past, I can vouch for the fact that they hinder adoption, particularly if the break is in something that is widely used. As you have noted, the double-pass signature is in wide use. If all of a sudden new versions of middleware dispatchers only support single-pass, all your middleware is now incompatible. The end result is one of two things:

• Folks upgrade.
• Folks stay on the legacy version.

I've seen it time and time again, and people tend to be very conservative in adopting new systems that require them to rewrite code without offering any compelling new benefits, particularly if their existing code cannot work in the new system without change.

As such, I've been very much behind having the http-middleware interfaces implement different methods than existing middleware stacks, as it allows us to provide forwards compatibility features in existing stacks.

Returning to the work we've done with Stratigility, we were able to provide such features, so that developers can mix and match existing double-pass middleware with http-middleware, and everything just works. This allows them to then gradually rewrite their middleware to be compatible with http-middleware, and their application continues to just work. They don't have to rewrite everything at once, which often leads to more errors and decreased chance of success — which can often lead to a decision not to migrate.

I evaluated what we'd need to do in Stratigility to make it work if http-middleware used __invoke() in its interface. While we could still type-hint on it in the dispatcher, the harder work comes when we pipe middleware into the system, particularly if we want to support callable middleware by decorating it. This is solvable, and we solved it already. But more difficult is adapting middleware written by end-users or implemented in third-party libraries: if they are using our existing interface (Zend\Stratigility\MiddlewareInterface), they cannot then go and adapt their existing middleware to also work with http-middleware, as the signatures conflict.

As a concrete example of why I feel so strongly about this: in zend-servicemanager v3, we had a huge effort to make the various factory signatures consistent. This, however, meant BC breaks in the signatures. We knew from the v1-v2 migration that if we did a clean break, users would not migrate. In fact, we knew if we did this, _any component that defined factories would require a major version bump_. So we decided to do the following:

• We defined the new signatures in new interfaces in a subnamespace.
• We updated the existing interfaces such that they extended the new interfaces.

This allowed us to update existing implementations to implement the methods defined in the interfaces in both major version branches, which then allowed code to opt-in to the new major version, while simultaneously retaining compatibility with the existing version.

An approach like this also allowed end users to adapt their code for version 3 before upgrading, giving them confidence that the upgrade would go seamlessly.

I really want this standard to succeed. But my experience tells me it will not succeed if it requires developers change their existing code en masse in order to adopt it. Defining a standard that can be dropped in in parallel with the existing code will allow a gradual migration, which will help the standard gain adoption.

[alamin3000]

@weierophinney This is SO irony. You guys don't care about changing years of used method signature in the industry, and start lecturing/dictating which is better/worse, and changing all the names and patterns on the way... but worry so much about temporary migration pain for developers. LOL.

BTW @weierophinney Have you seen how messy code you guys wend up having in the Zend MiddlewarePipe, Next and else where gotten just to support these? Developers will eventually have to rewrite lots of code anyway to adopt to the proposed interfaces.

To me, a middleware is a simple piece of code that comes between a request and response. And there is just one interaction to that code/class from the outside, is to invoke that code. Within the lifecycle of that middleware chain, there is no other interaction to that class/code from external. That's why this class SHOULD be invokable. Assigning any arbitrary name like process, execute, delegate is non-sense.

And if you are going to complain about your pain, then here is my story. Most of our Action classes uses method signature process(ServerRequestInterface) : ModelInterface. So our developers don't care about middleware concept, simply implement that method. Well you can guess what we will have to go through with this PSR having same name with different signature. But that doesn't bother me as much as everything else.

[schnittstabil]

While you feel that BC is not an issue, as somebody who has implemented BC breaks in major versions in the past, I can vouch for the fact that they hinder adoption, particularly if the break is in something that is widely used.

There's no need to get personal. I well know my audience, especially in projects where I've released major versions in a relatively small amount of time. I can assure you, the BCs have been thoroughly considered.

@weierophinney I don't really understand your argumentation. You already know and describe in detail, that mixing single-pass and double-pass in a single stack is a bad idea and can lead to very hard to debug issues – you mentioned some problems above and I'm sure you know Anthony Ferrara’s post which mentions that many double-pass implementations are poorly written, Single/Double-Pass-Adapters don't work well and thus double-pass middlewares don't work with single-pass middlewares as expected.

But then you conclude:

with Stratigility, we were able to provide such features, so that developers can mix and match existing double-pass middleware with http-middleware, and everything just works.

Everything just works – WUT? :flushed: