New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create initial proposal for middleware interface #755

Merged
merged 3 commits into from Jun 6, 2016

Conversation

Projects
None yet
@shadowhand
Contributor

shadowhand commented May 10, 2016

@hannesvdvreken hannesvdvreken referenced this pull request May 10, 2016

Open

A year passed... #1

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark May 10, 2016

Contributor

I would rename it to HTTP Middleware as it is closely PSR-7 related, but middlewares themselves are not.

Contributor

sagikazarmark commented May 10, 2016

I would rename it to HTTP Middleware as it is closely PSR-7 related, but middlewares themselves are not.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 10, 2016

Contributor

I would think @mindplay-dk has right of first refusal as "editor" here, since he brought it up.

Contributor

pmjones commented May 10, 2016

I would think @mindplay-dk has right of first refusal as "editor" here, since he brought it up.

Show outdated Hide outdated proposed/middleware-meta.md
* [Zend Stratigility](https://github.com/zendframework/zend-stratigility/blob/1.0.0/src/MiddlewarePipe.php#L69-L79)
* [Relay](https://github.com/relayphp/Relay.Relay/blob/1.0.0/src/MiddlewareInterface.php#L24)
* [Slim](https://github.com/slimphp/Slim/blob/3.4.0/Slim/MiddlewareAwareTrait.php#L66-L75)
* [Middleman](https://github.com/mindplay-dk/middleman/blob/1.0.0/src/MiddlewareInterface.php#L24)

This comment has been minimized.

@localheinz

localheinz May 10, 2016

Contributor

@shadowhand

How about keeping these projects sorted alphabetically?

@localheinz

localheinz May 10, 2016

Contributor

@shadowhand

How about keeping these projects sorted alphabetically?

This comment has been minimized.

This comment has been minimized.

@shadowhand

shadowhand May 10, 2016

Contributor

Thanks for the additional list!

@shadowhand

shadowhand May 10, 2016

Contributor

Thanks for the additional list!

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 10, 2016

Contributor

@pmjones I'm perfectly happy to let @mindplay-dk take ownership. My goal was simply to get the ball rolling.

Contributor

shadowhand commented May 10, 2016

@pmjones I'm perfectly happy to let @mindplay-dk take ownership. My goal was simply to get the ball rolling.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 10, 2016

Contributor

@sagikazarmark I'm not opposed to a rename.

Contributor

shadowhand commented May 10, 2016

@sagikazarmark I'm not opposed to a rename.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 10, 2016

Contributor

@shadowhand Good form, sir. :-)

Contributor

pmjones commented May 10, 2016

@shadowhand Good form, sir. :-)

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 10, 2016

Contributor

Just dropping a link to the draft I wrote here:

https://gist.github.com/mindplay-dk/1d60ccfa083acfca32698d3c6d9a0945

I'm not going to do much more now - I don't have time. Will check in here and on the mailing list when I can.

@shadowhand feel free to use whatever you want from what I drafted. (That awesome list by @localheinz should be added to it, I think - I started, but only have a few items on mine.)

Contributor

mindplay-dk commented May 10, 2016

Just dropping a link to the draft I wrote here:

https://gist.github.com/mindplay-dk/1d60ccfa083acfca32698d3c6d9a0945

I'm not going to do much more now - I don't have time. Will check in here and on the mailing list when I can.

@shadowhand feel free to use whatever you want from what I drafted. (That awesome list by @localheinz should be added to it, I think - I started, but only have a few items on mine.)

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 10, 2016

Contributor

Very good. @mindplay-dk thanks for bringing up the idea; @shadowhand it looks like you're editor and lead if you want to be. I'll be happy to fill the role of coordinator if you want me to.

Contributor

pmjones commented May 10, 2016

Very good. @mindplay-dk thanks for bringing up the idea; @shadowhand it looks like you're editor and lead if you want to be. I'll be happy to fill the role of coordinator if you want me to.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 11, 2016

Contributor

I just pushed a bunch of changes to this, including a complete interface, as well as a definition for the exception and trait for server request checking. Please review and let me know if other changes need to be made before an entrance vote.

@pmjones should I list you as the sponsor of this proposal?

@mindplay-dk I have integrated your draft with my proposal. Some of it was directly copied in and some of it was edited. Please let me know if you don't agree with my changes.

@localheinz the list of middleware has been updated from your list. Thank you for sharing it!

Contributor

shadowhand commented May 11, 2016

I just pushed a bunch of changes to this, including a complete interface, as well as a definition for the exception and trait for server request checking. Please review and let me know if other changes need to be made before an entrance vote.

@pmjones should I list you as the sponsor of this proposal?

@mindplay-dk I have integrated your draft with my proposal. Some of it was directly copied in and some of it was edited. Please let me know if you don't agree with my changes.

@localheinz the list of middleware has been updated from your list. Thank you for sharing it!

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 11, 2016

Contributor

@shadowhand As a sponsor or coordinator, your call.

I think this is a very good start and ready for presentation to the group once a second sponsor is found.

Contributor

pmjones commented May 11, 2016

@shadowhand As a sponsor or coordinator, your call.

I think this is a very good start and ready for presentation to the group once a second sponsor is found.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 11, 2016

Contributor

@mwop would you be willing to be the second sponsor, since Zend Framework has a clear stake in this proposal?

Contributor

shadowhand commented May 11, 2016

@mwop would you be willing to be the second sponsor, since Zend Framework has a clear stake in this proposal?

@localheinz

This comment has been minimized.

Show comment
Hide comment
@localheinz

localheinz May 11, 2016

Contributor

@shadowhand

I think you intended to mention @weierophinney, right?!

Contributor

localheinz commented May 11, 2016

@shadowhand

I think you intended to mention @weierophinney, right?!

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 11, 2016

Contributor

@localheinz oops yes I did.

Contributor

shadowhand commented May 11, 2016

@localheinz oops yes I did.

@n1215

This comment has been minimized.

Show comment
Hide comment
@n1215

n1215 May 11, 2016

I think RequiresServerRequestTrait::assertServerRequest() should return ServerRequestInterface for code completion.

Maybe another method name like ensureServerRequest is better.

n1215 commented May 11, 2016

I think RequiresServerRequestTrait::assertServerRequest() should return ServerRequestInterface for code completion.

Maybe another method name like ensureServerRequest is better.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 11, 2016

Contributor

@n1215 I don't agree. Method chaining increases cognitive load for the sake of convenience and could be abused with immutable objects

EDIT: see https://ocramius.github.io/blog/fluent-interfaces-are-evil/

Contributor

shadowhand commented May 11, 2016

@n1215 I don't agree. Method chaining increases cognitive load for the sake of convenience and could be abused with immutable objects

EDIT: see https://ocramius.github.io/blog/fluent-interfaces-are-evil/

@n1215

This comment has been minimized.

Show comment
Hide comment
@n1215

n1215 May 11, 2016

@shadowhand
This change has no method chain. I cannot understand why fluent interfaces are related to this.
https://gist.github.com/n1215/a3ef3f644bb795dd204c70e730dc3fab

n1215 commented May 11, 2016

@shadowhand
This change has no method chain. I cannot understand why fluent interfaces are related to this.
https://gist.github.com/n1215/a3ef3f644bb795dd204c70e730dc3fab

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 11, 2016

Contributor

Returning an object is a fluent interface. There is no need for it.

Contributor

shadowhand commented May 11, 2016

Returning an object is a fluent interface. There is no need for it.

Show outdated Hide outdated proposed/middleware.md
*
* @throws ServerRequestRequiredException
*/
private function assertServerRequest(RequestInterface $request)

This comment has been minimized.

@mindplay-dk

mindplay-dk May 12, 2016

Contributor

this should probably be protected to allow class hierarchies

@mindplay-dk

mindplay-dk May 12, 2016

Contributor

this should probably be protected to allow class hierarchies

This comment has been minimized.

@shadowhand

shadowhand May 12, 2016

Contributor

Why would that be necessary? A trait can be used in any scenario and so using protected offers no advantage.

@shadowhand

shadowhand May 12, 2016

Contributor

Why would that be necessary? A trait can be used in any scenario and so using protected offers no advantage.

This comment has been minimized.

@sagikazarmark

sagikazarmark May 12, 2016

Contributor

Should this trait be here at all? I mean, for cache there is a util repo. Any non-interface stuff should be placed in such thing and should not be part of the standard IMO.

@sagikazarmark

sagikazarmark May 12, 2016

Contributor

Should this trait be here at all? I mean, for cache there is a util repo. Any non-interface stuff should be placed in such thing and should not be part of the standard IMO.

This comment has been minimized.

@shadowhand

shadowhand May 12, 2016

Contributor

What is the difference between shipping with an optional trait vs having a util repo? Personally I think having a trait is more efficient and easier to use.

@shadowhand

shadowhand May 12, 2016

Contributor

What is the difference between shipping with an optional trait vs having a util repo? Personally I think having a trait is more efficient and easier to use.

This comment has been minimized.

@sagikazarmark

sagikazarmark May 12, 2016

Contributor

IMO it makes it part of the standard, while it isn't. The less content the better it is. And it's an already existing pattern within the FIG.

@sagikazarmark

sagikazarmark May 12, 2016

Contributor

IMO it makes it part of the standard, while it isn't. The less content the better it is. And it's an already existing pattern within the FIG.

Show outdated Hide outdated proposed/middleware.md
*
* @param RequestInterface $request
* @param ResponseInterface $response
* @param callable $next

This comment has been minimized.

@mindplay-dk

mindplay-dk May 12, 2016

Contributor

needs inline documentation explaining what this is - a lot of people (myself included) thought this was the next middleware component on the stack, which it isn't, it's a delegate with a different signature.

I would suggest using adding an interface for the delegate type-hint this parameter as @param callable|MiddlewareDelegateInterface $next - or otherwise, add inline documentation displaying the method signature, e.g.:

@param callable|MiddlewareDelegateInterface $next delegate function to dispatch the next middleware component:
                                                  function (RequestInterface $request, ResponseInterface $response) : ResponseInterface
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

needs inline documentation explaining what this is - a lot of people (myself included) thought this was the next middleware component on the stack, which it isn't, it's a delegate with a different signature.

I would suggest using adding an interface for the delegate type-hint this parameter as @param callable|MiddlewareDelegateInterface $next - or otherwise, add inline documentation displaying the method signature, e.g.:

@param callable|MiddlewareDelegateInterface $next delegate function to dispatch the next middleware component:
                                                  function (RequestInterface $request, ResponseInterface $response) : ResponseInterface

This comment has been minimized.

@samdark

samdark May 12, 2016

Contributor

Agree. I've thought it's next middleware as well at first.

@samdark

samdark May 12, 2016

Contributor

Agree. I've thought it's next middleware as well at first.

This comment has been minimized.

@shadowhand

shadowhand May 12, 2016

Contributor

@mindplay-dk the meta clearly states that defining the dispatch mechanism is not a goal. I'm happy to document it more, but I don't think adding an interface is appropriate for the scope of this proposal.

@shadowhand

shadowhand May 12, 2016

Contributor

@mindplay-dk the meta clearly states that defining the dispatch mechanism is not a goal. I'm happy to document it more, but I don't think adding an interface is appropriate for the scope of this proposal.

This comment has been minimized.

@mindplay-dk

mindplay-dk May 14, 2016

Contributor

This isn't about the dispatch mechanism, it's just a matter of missing documentation for the $callable argument - it's not obvious to anyone what exactly that callable is, most people seem to think it's the next middleware component, which isn't true, it's a delegate to dispatch the next middleware function.

Since we're not defining the delegate interface as an actual type, we do need to document what it is:

@param callable $next delegate function to dispatch the next middleware component:
                      function (RequestInterface $request, ResponseInterface $response): ResponseInterface

Yes?

@mindplay-dk

mindplay-dk May 14, 2016

Contributor

This isn't about the dispatch mechanism, it's just a matter of missing documentation for the $callable argument - it's not obvious to anyone what exactly that callable is, most people seem to think it's the next middleware component, which isn't true, it's a delegate to dispatch the next middleware function.

Since we're not defining the delegate interface as an actual type, we do need to document what it is:

@param callable $next delegate function to dispatch the next middleware component:
                      function (RequestInterface $request, ResponseInterface $response): ResponseInterface

Yes?

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

IMO the exception and trait are too much - it raises more questions than answers. The exception itself is a runtime-exception anyhow; runtime-exceptions are unchecked, so it doesn't actually matter if these are consistent or not.

I still think that requiring everyone to do a run-time check to see if the incoming request is a server-request, is really problematic - the large majority of middleware will have to implement this check; it seems wrong to favor the most rare case and make every middleware check with instanceof in almost every case.

I would strongly prefer having two interfaces:

  1. ServerMiddlewareInterface accepting ServerRequestInterface as the first argument, and
  2. MiddlewareInterface accepting RequestInterface for the other rare cases

Since the second interface is by far the most rarely used, having to do instanceof check etc. in those few cases is okay - IMO, the primary use case of server-middleware should not have to be littered with run-time type-checks and doc-blocks for IDE support.

Contributor

mindplay-dk commented May 12, 2016

IMO the exception and trait are too much - it raises more questions than answers. The exception itself is a runtime-exception anyhow; runtime-exceptions are unchecked, so it doesn't actually matter if these are consistent or not.

I still think that requiring everyone to do a run-time check to see if the incoming request is a server-request, is really problematic - the large majority of middleware will have to implement this check; it seems wrong to favor the most rare case and make every middleware check with instanceof in almost every case.

I would strongly prefer having two interfaces:

  1. ServerMiddlewareInterface accepting ServerRequestInterface as the first argument, and
  2. MiddlewareInterface accepting RequestInterface for the other rare cases

Since the second interface is by far the most rarely used, having to do instanceof check etc. in those few cases is okay - IMO, the primary use case of server-middleware should not have to be littered with run-time type-checks and doc-blocks for IDE support.

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark May 12, 2016

Contributor

I agree with @mindplay-dk. Are there any cases when the same middleware can be used for both kinds of messages at all? Because that seems to be the reason behind having one common interface, but I don't think it's a common case, if possible at all.

Contributor

sagikazarmark commented May 12, 2016

I agree with @mindplay-dk. Are there any cases when the same middleware can be used for both kinds of messages at all? Because that seems to be the reason behind having one common interface, but I don't think it's a common case, if possible at all.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

The problem with having two different interfaces is in type hinting. At the very minimum, we would end up with three interfaces (one empty):

interface MiddlewareInterface {} // totally empty

ServerMiddlewareInterface extends MiddlewareInterface
{
    public function __invoke(...);
}

ClientMiddlewareInterface extends MiddlewareInterface
{
    public function __invoke(...);
}

The only difference between server and client middleware is the usage of methods that are associated with ServerRequestInterface, such as attributes and cookies.

Contributor

shadowhand commented May 12, 2016

The problem with having two different interfaces is in type hinting. At the very minimum, we would end up with three interfaces (one empty):

interface MiddlewareInterface {} // totally empty

ServerMiddlewareInterface extends MiddlewareInterface
{
    public function __invoke(...);
}

ClientMiddlewareInterface extends MiddlewareInterface
{
    public function __invoke(...);
}

The only difference between server and client middleware is the usage of methods that are associated with ServerRequestInterface, such as attributes and cookies.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 12, 2016

Contributor

Agree with @mindplay-dk as well. In addition, Middleware that can be used for both RequestInterface and ServerRequestInterface can be typehinted against just RequestInterface, since (if memory serves) ServerRequestInterface extends it.

Contributor

pmjones commented May 12, 2016

Agree with @mindplay-dk as well. In addition, Middleware that can be used for both RequestInterface and ServerRequestInterface can be typehinted against just RequestInterface, since (if memory serves) ServerRequestInterface extends it.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

@pmjones sort of, but it doesn't provide protection for ensuring that necessary methods exist, hence the addition of the trait to this proposal.

ServerMiddlewareInterface cannot extend from MiddlewareInterface, because the signatures are different. See here: https://3v4l.org/fVPAV

Contributor

shadowhand commented May 12, 2016

@pmjones sort of, but it doesn't provide protection for ensuring that necessary methods exist, hence the addition of the trait to this proposal.

ServerMiddlewareInterface cannot extend from MiddlewareInterface, because the signatures are different. See here: https://3v4l.org/fVPAV

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark May 12, 2016

Contributor

I guess he meant the case when the middleware does not implement the interface, but is a callable.

Contributor

sagikazarmark commented May 12, 2016

I guess he meant the case when the middleware does not implement the interface, but is a callable.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 12, 2016

Contributor

@shadowhand My point (perhaps mistaken) is that a dispatch mechanism might take any combination of ServerRequest middlewares and (Client)Request middlewares; a middleware used in that dispatch cycle typehinted to (Client)Request can still receive a ServerRequest object and fullfill the typehint. (Perhaps I am forgetting something.)

Contributor

pmjones commented May 12, 2016

@shadowhand My point (perhaps mistaken) is that a dispatch mechanism might take any combination of ServerRequest middlewares and (Client)Request middlewares; a middleware used in that dispatch cycle typehinted to (Client)Request can still receive a ServerRequest object and fullfill the typehint. (Perhaps I am forgetting something.)

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

@pmjones that's correct, but how the middleware check that the request is of the correct type? There needs to be some mechanism for that and I feel this proposal is incomplete without (at minimum) a standard exception class that can be caught.

Contributor

shadowhand commented May 12, 2016

@pmjones that's correct, but how the middleware check that the request is of the correct type? There needs to be some mechanism for that and I feel this proposal is incomplete without (at minimum) a standard exception class that can be caught.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 12, 2016

Contributor

how the middleware check that the request is of the correct type

(/me ponders)

I think for a ServerMiddleware typehinted to ServerRequestInterface, obviously no need to check for type, since it's the extended type.

And I think for a ClientMiddleware typehinted to RequestInterface, there's no expectation of the ServerRequest methods, so there's no need to check for type there either.

So for those who want to write a middleware that works for both Request and ServerRequest, depending on only the RequestInterface seems like the right thing to do.

I feel like I'm missing an important part of your question somehow.

Contributor

pmjones commented May 12, 2016

how the middleware check that the request is of the correct type

(/me ponders)

I think for a ServerMiddleware typehinted to ServerRequestInterface, obviously no need to check for type, since it's the extended type.

And I think for a ClientMiddleware typehinted to RequestInterface, there's no expectation of the ServerRequest methods, so there's no need to check for type there either.

So for those who want to write a middleware that works for both Request and ServerRequest, depending on only the RequestInterface seems like the right thing to do.

I feel like I'm missing an important part of your question somehow.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones May 12, 2016

Contributor

p.s. I'm coming at this from the direction of wanting two middleware interfaces, one for client requests and one for server requests. Perhaps that's the disconnect.

Contributor

pmjones commented May 12, 2016

p.s. I'm coming at this from the direction of wanting two middleware interfaces, one for client requests and one for server requests. Perhaps that's the disconnect.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

There's no problem with a middleware that only needs the methods in Request. The problem is when you do require ServerRequest methods you either have to:

  1. Typehint against RequestInterface, which could result in a fatal error the request is not ServerRequestInterface.
  2. Implement a totally different interface in order to type hint against ServerRequestInterface.

I was attempting to solve the former relatively cleanly by providing a trait that components could use.

The problem with the latter is that validating middleware type requires checking two entirely different interfaces, rather than one. Which means you cannot type hint against it in your dispatcher.

Contributor

shadowhand commented May 12, 2016

There's no problem with a middleware that only needs the methods in Request. The problem is when you do require ServerRequest methods you either have to:

  1. Typehint against RequestInterface, which could result in a fatal error the request is not ServerRequestInterface.
  2. Implement a totally different interface in order to type hint against ServerRequestInterface.

I was attempting to solve the former relatively cleanly by providing a trait that components could use.

The problem with the latter is that validating middleware type requires checking two entirely different interfaces, rather than one. Which means you cannot type hint against it in your dispatcher.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

As far as I can figure, we only need two interfaces.

Per PSR-7 there are two levels, not two kinds of requests - a general "request" and a specialized "server request".

Our middleware architecture should mirror that, e.g. a general "middleware" and a specialized "server middleware".

Though I can't quite figure out if or how that's possible with PHP... it seems to require either a type union or generics or some other feature PHP doesn't seem to have... hmm...

Contributor

mindplay-dk commented May 12, 2016

As far as I can figure, we only need two interfaces.

Per PSR-7 there are two levels, not two kinds of requests - a general "request" and a specialized "server request".

Our middleware architecture should mirror that, e.g. a general "middleware" and a specialized "server middleware".

Though I can't quite figure out if or how that's possible with PHP... it seems to require either a type union or generics or some other feature PHP doesn't seem to have... hmm...

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

I'm strongly opposed to the two interface solution but would be open to a three interface solution. Two interfaces means that no type hints can be used for dispatch/collection, because the interfaces have no common ancestor. Three interfaces (one empty) allows a small amount of assurance that you're getting what you want.

Contributor

shadowhand commented May 12, 2016

I'm strongly opposed to the two interface solution but would be open to a three interface solution. Two interfaces means that no type hints can be used for dispatch/collection, because the interfaces have no common ancestor. Three interfaces (one empty) allows a small amount of assurance that you're getting what you want.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

I suppose, if we can't have two interfaces (in a meaningful way) then middleware will need to be implemented something like:

class MyServerMiddleware implements MiddlewareInterface {
    public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        return $this->process($request, $response, $next);
    }

    private function process(
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        // ...
    }
}

That seems really shite. I think IDEs and static analysis tools will warn for the call in __invoke() right off the bat. hmmm....

Contributor

mindplay-dk commented May 12, 2016

I suppose, if we can't have two interfaces (in a meaningful way) then middleware will need to be implemented something like:

class MyServerMiddleware implements MiddlewareInterface {
    public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        return $this->process($request, $response, $next);
    }

    private function process(
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        // ...
    }
}

That seems really shite. I think IDEs and static analysis tools will warn for the call in __invoke() right off the bat. hmmm....

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

Or maybe we'll have to ship it with a base class for server middleware?

abstract class ServerMiddleware implements MiddlewareInterface
{
    final public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        if (! $request instanceof ServerRequestInterface)) {
            throw new RuntimeException("middleware implementation error :-(");
        }

        return $this->run($request, $response, $next);
    }

    abstract public function run(
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface;
}
Contributor

mindplay-dk commented May 12, 2016

Or maybe we'll have to ship it with a base class for server middleware?

abstract class ServerMiddleware implements MiddlewareInterface
{
    final public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface {
        if (! $request instanceof ServerRequestInterface)) {
            throw new RuntimeException("middleware implementation error :-(");
        }

        return $this->run($request, $response, $next);
    }

    abstract public function run(
        ServerRequestInterface $request,
        ResponseInterface $response,
        callable $next
    ) : ResponseInterface;
}
@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

@mindplay-dk I really don't like that.

Contributor

shadowhand commented May 12, 2016

@mindplay-dk I really don't like that.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

IMO, an empty interface is an ugly work-around and it's no assurance of anything - it could lead to confusion as well, what if somebody implements the empty interface? It'll pass the type-check even if it has neither the server or client signature.

Well, heck...

interface MiddlewareInterface
{
    public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    );
}

/**
 * @method __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next)
 */
interface ServerMiddlewareInterface extends MiddlewareInterface
{
}

Hahaha ;-)

Contributor

mindplay-dk commented May 12, 2016

IMO, an empty interface is an ugly work-around and it's no assurance of anything - it could lead to confusion as well, what if somebody implements the empty interface? It'll pass the type-check even if it has neither the server or client signature.

Well, heck...

interface MiddlewareInterface
{
    public function __invoke(
        RequestInterface $request,
        ResponseInterface $response,
        callable $next
    );
}

/**
 * @method __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next)
 */
interface ServerMiddlewareInterface extends MiddlewareInterface
{
}

Hahaha ;-)

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 12, 2016

Contributor

If someone implements an empty interface it will blow up in their face. I'm fine with that.

Contributor

shadowhand commented May 12, 2016

If someone implements an empty interface it will blow up in their face. I'm fine with that.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk May 12, 2016

Contributor

Well, since there's no happy middle ground with PHP as it is today, I guess we'll have to settle for basically what the community is already doing - have a middleware-interface, and doc-block your implementation of __invoke() with @param ServerRequestInterface $request and deal with the missing type-check.

Contributor

mindplay-dk commented May 12, 2016

Well, since there's no happy middle ground with PHP as it is today, I guess we'll have to settle for basically what the community is already doing - have a middleware-interface, and doc-block your implementation of __invoke() with @param ServerRequestInterface $request and deal with the missing type-check.

Show outdated Hide outdated proposed/middleware-meta.md
## 3.1 Goals
* Provide a middleware interface that is compatible with PSR-7 as a Composer package.

This comment has been minimized.

@philsturgeon

philsturgeon May 30, 2016

Contributor

Maybe mention "PSR-7 Http Messages" to keep it clear exactly whats happening. Lots of numbers can confuse.

@philsturgeon

philsturgeon May 30, 2016

Contributor

Maybe mention "PSR-7 Http Messages" to keep it clear exactly whats happening. Lots of numbers can confuse.

Show outdated Hide outdated proposed/middleware-meta.md
Agreeing on a formal middleware interface eliminates several problems and
provides a number of benefits:
* Provides a formal standard for middleware developers to commit to.

This comment has been minimized.

@philsturgeon

philsturgeon May 30, 2016

Contributor

These goals make sense to you and I know what you're getting at, but the trouble is they're likely not clear to the masses.

It almost looks like you're defining a definition with the definition.

"Since PSR-7 has standardized an interface for HTTP requests and responses, we have started to see a huge number of varying implementations of middlewares utilizing these interfaces. Whilst the message interfaces are a great step towards middleware, the actual middleware handlers need to be standardized so that these handlers can be shared between multiple implementations."

  • Provide a formal standard for the structure of middleware "handlers".
  • Eliminates duplication of multiple incredibly similar interfaces defined by various frameworks.
  • Allow sharing of middlewares without minor discrepancies in method signatures breaking things.
  • Enables any middleware component to run in any compatible framework.

Or something.

It's super important to keep language accessible and clear, so we don't have people - who would otherwise be onboard - arguing against it. This happens a lot :D

@philsturgeon

philsturgeon May 30, 2016

Contributor

These goals make sense to you and I know what you're getting at, but the trouble is they're likely not clear to the masses.

It almost looks like you're defining a definition with the definition.

"Since PSR-7 has standardized an interface for HTTP requests and responses, we have started to see a huge number of varying implementations of middlewares utilizing these interfaces. Whilst the message interfaces are a great step towards middleware, the actual middleware handlers need to be standardized so that these handlers can be shared between multiple implementations."

  • Provide a formal standard for the structure of middleware "handlers".
  • Eliminates duplication of multiple incredibly similar interfaces defined by various frameworks.
  • Allow sharing of middlewares without minor discrepancies in method signatures breaking things.
  • Enables any middleware component to run in any compatible framework.

Or something.

It's super important to keep language accessible and clear, so we don't have people - who would otherwise be onboard - arguing against it. This happens a lot :D

Show outdated Hide outdated proposed/middleware-meta.md
6. Votes
--------
* **Entrance Vote:** _(not yet taken)_

This comment has been minimized.

shadowhand added some commits May 10, 2016

Add proposal for middleware interface
A PSR-7 compatible middleware interface has been defined as an informal
standard already. This takes these existing implementations and creates
a formal standard recommendation.
@geggleto

This comment has been minimized.

Show comment
Hide comment
@geggleto

geggleto May 31, 2016

As a follow up to @tuupola's comment, The already existing frameworks have chosen the double pass approach for a good reason. Perhaps if we could get some of them to comment on why they choose the double pass approach in the first place.

geggleto commented May 31, 2016

As a follow up to @tuupola's comment, The already existing frameworks have chosen the double pass approach for a good reason. Perhaps if we could get some of them to comment on why they choose the double pass approach in the first place.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand May 31, 2016

Contributor

I think @pmjones knows the history.

Contributor

shadowhand commented May 31, 2016

I think @pmjones knows the history.

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand Jun 6, 2016

Contributor

@pmjones @opengeek what do I need to do to get this merged? The acceptance vote is passed.

Contributor

shadowhand commented Jun 6, 2016

@pmjones @opengeek what do I need to do to get this merged? The acceptance vote is passed.

@pmjones

This comment has been minimized.

Show comment
Hide comment
@pmjones

pmjones Jun 6, 2016

Contributor

ping @michaelcullum @svpernova09 @squinones

Contributor

pmjones commented Jun 6, 2016

ping @michaelcullum @svpernova09 @squinones

@svpernova09 svpernova09 merged commit 67c8c42 into php-fig:master Jun 6, 2016

@svpernova09

This comment has been minimized.

Show comment
Hide comment
@svpernova09

svpernova09 Jun 6, 2016

Contributor

I don't have the ability to deploy at the moment. @reinink could you do a site update at your convenience?

Contributor

svpernova09 commented Jun 6, 2016

I don't have the ability to deploy at the moment. @reinink could you do a site update at your convenience?

@reinink

This comment has been minimized.

Show comment
Hide comment
@reinink

reinink Jun 6, 2016

Contributor

@svpernova09 Will do!

Contributor

reinink commented Jun 6, 2016

@svpernova09 Will do!

@shadowhand shadowhand deleted the shadowhand:middleware branch Jun 15, 2016

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 14, 2016

Contributor

@shadowhand two issues.

Regarding FrameInterface, what's the story behind the term "frame"? A "frame" object, as far as I understand, is just a delegate function for dispatching the next middleware on the stack. I don't understand what the term "frame" is supposed to communicate. I personally refer to the $next function as a "middleware delegate" in daily speech, and would have likely named it something like MiddlewareDelegateInterface, which is a bit wordy, but should raise fewer questions than "frame".

Also, I wonder, with StackInterface, do we have another case of SRP violation? IMO, from the consumer's perspecive, what makes something a "middleware stack", is the ability to process a request.

How you add or remove middleware doesn't affect my ability to dispatch a request - that's really about manipulating an internal array/stack, which is not how I implement immutability in my dispatcher, which is fully-immutable once instantiated; if one wants to manipulate the array/stack of middleware components, one would need to do that prior to instantiation, which is simpler.

Also, my dispatcher creates/initializes middleware on the fly, so that interface wouldn't even make sense or work for me, and I'm planning some other changes that would clash with this even more.

This makes me think that this part of the interface is first of all not necessary, but secondly, doesn't help interoperability - instead, it feels opinionated, and I feel it's actually standing in the way of interop. I realize it's listed as optional, but an optional interface doesn't guarantee interop, so it doesn't even really work for that.

In addition, the ability to process a request probably shouldn't be optional? It seems like this an interface any dispatcher could implement, and it probably should be a MUST, not a MAY, to maximize interop. Which again points at an SRP issue, because, as explain, those other two stack manipulation features are not a good fit for my dispatcher...

Contributor

mindplay-dk commented Aug 14, 2016

@shadowhand two issues.

Regarding FrameInterface, what's the story behind the term "frame"? A "frame" object, as far as I understand, is just a delegate function for dispatching the next middleware on the stack. I don't understand what the term "frame" is supposed to communicate. I personally refer to the $next function as a "middleware delegate" in daily speech, and would have likely named it something like MiddlewareDelegateInterface, which is a bit wordy, but should raise fewer questions than "frame".

Also, I wonder, with StackInterface, do we have another case of SRP violation? IMO, from the consumer's perspecive, what makes something a "middleware stack", is the ability to process a request.

How you add or remove middleware doesn't affect my ability to dispatch a request - that's really about manipulating an internal array/stack, which is not how I implement immutability in my dispatcher, which is fully-immutable once instantiated; if one wants to manipulate the array/stack of middleware components, one would need to do that prior to instantiation, which is simpler.

Also, my dispatcher creates/initializes middleware on the fly, so that interface wouldn't even make sense or work for me, and I'm planning some other changes that would clash with this even more.

This makes me think that this part of the interface is first of all not necessary, but secondly, doesn't help interoperability - instead, it feels opinionated, and I feel it's actually standing in the way of interop. I realize it's listed as optional, but an optional interface doesn't guarantee interop, so it doesn't even really work for that.

In addition, the ability to process a request probably shouldn't be optional? It seems like this an interface any dispatcher could implement, and it probably should be a MUST, not a MAY, to maximize interop. Which again points at an SRP issue, because, as explain, those other two stack manipulation features are not a good fit for my dispatcher...

@shadowhand

This comment has been minimized.

Show comment
Hide comment
@shadowhand

shadowhand Aug 15, 2016

Contributor

@mindplay-dk I would refer to https://groups.google.com/forum/#!topic/php-fig/GTm9ho6rFts which is covering the exact same things.

Contributor

shadowhand commented Aug 15, 2016

@mindplay-dk I would refer to https://groups.google.com/forum/#!topic/php-fig/GTm9ho6rFts which is covering the exact same things.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli
Member

mnapoli commented Aug 15, 2016

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 16, 2016

Contributor

@shadowhand @mnapoli I read both of threads, and I don't feel like either of them addresses either of the issues I'm highlighting.

  1. Separation of dispatch from configuration methods: SRP. My dispatcher won't allow modification - if I modify the stack, I do so prior to constructing the dispatcher. The current interface dictates a specific design, which I don't agree with; it's opinionated.
  2. The ability to process a request should be a MUST, not a MAY - my impression is that it is currently a MAY because of the responsibility overlap with configuration, which needs to be optional; I believe this points at two distinct responsibilities?
  3. No one suggested "delegate" as the proper term in those discussions? - I've posted the suggestion to that thread for further discussion.
Contributor

mindplay-dk commented Aug 16, 2016

@shadowhand @mnapoli I read both of threads, and I don't feel like either of them addresses either of the issues I'm highlighting.

  1. Separation of dispatch from configuration methods: SRP. My dispatcher won't allow modification - if I modify the stack, I do so prior to constructing the dispatcher. The current interface dictates a specific design, which I don't agree with; it's opinionated.
  2. The ability to process a request should be a MUST, not a MAY - my impression is that it is currently a MAY because of the responsibility overlap with configuration, which needs to be optional; I believe this points at two distinct responsibilities?
  3. No one suggested "delegate" as the proper term in those discussions? - I've posted the suggestion to that thread for further discussion.
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 16, 2016

Member

Separation of dispatch from configuration methods: SRP. My dispatcher won't allow modification - if I modify the stack, I do so prior to constructing the dispatcher. The current interface dictates a specific design, which I don't agree with; it's opinionated.

Agreed.

The ability to process a request should be a MUST, not a MAY - my impression is that it is currently a MAY because of the responsibility overlap with configuration, which needs to be optional; I believe this points at two distinct responsibilities?

Agreed.

No one suggested "delegate" as the proper term in those discussions? - I've posted the suggestion to that thread for further discussion.

That's already an improvement, even though still vague but that's just my opinion.

While I agree with your remarks about StackInterface my point is that interface should not be in the standard at all, hence those problems disappear.

Member

mnapoli commented Aug 16, 2016

Separation of dispatch from configuration methods: SRP. My dispatcher won't allow modification - if I modify the stack, I do so prior to constructing the dispatcher. The current interface dictates a specific design, which I don't agree with; it's opinionated.

Agreed.

The ability to process a request should be a MUST, not a MAY - my impression is that it is currently a MAY because of the responsibility overlap with configuration, which needs to be optional; I believe this points at two distinct responsibilities?

Agreed.

No one suggested "delegate" as the proper term in those discussions? - I've posted the suggestion to that thread for further discussion.

That's already an improvement, even though still vague but that's just my opinion.

While I agree with your remarks about StackInterface my point is that interface should not be in the standard at all, hence those problems disappear.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 16, 2016

Contributor

my point is that interface should not be in the standard at all, hence those problems disappear

Note that I'm not making that assertion - I'm not opposing the interface, as long as it's optional, and separate from the interface that makes it dispatchable.

The existence of a middleware delegate interface does seem to have some merit in terms of offline inspections and IDE support, do you not agree?

I'm currently implementing a different type of middleware stack, not for HTTP requests, and I'm using the approach with type-safe delegate - I will say that, in any other regard besides inspections/IDE support, those delegates are themselves merely dumb delegates to internally-generated closures, e.g. something like:

class MiddlewareDelegate implements MiddlewareDelegateInterface
{
    /**
     * @var callable
     */
    private $delegate;

    /**
     * @param callable $delegate
     */
    public function __construct(callable $delegate)
    {
        $this->delegate = $delegate;
    }

    /**
     * @param Request $request
     *
     * @return Response
     */
    public function resolve(Request $request)
    {
        return call_user_func($this->delegate, $request);
    }
}

In other words, this doesn't appear to have any value beyond external type-safety - internally, it's still call_user_func() with no safety.

I am somewhat concerned about the performance implications - in micro benchmarks, these dispatchers are going to measurably slower than simpler dispatchers using callables with no object-wrapping. In practice, probably only measurable in micro benchmarks though; in practice, the cost is negligible for sure.

Contributor

mindplay-dk commented Aug 16, 2016

my point is that interface should not be in the standard at all, hence those problems disappear

Note that I'm not making that assertion - I'm not opposing the interface, as long as it's optional, and separate from the interface that makes it dispatchable.

The existence of a middleware delegate interface does seem to have some merit in terms of offline inspections and IDE support, do you not agree?

I'm currently implementing a different type of middleware stack, not for HTTP requests, and I'm using the approach with type-safe delegate - I will say that, in any other regard besides inspections/IDE support, those delegates are themselves merely dumb delegates to internally-generated closures, e.g. something like:

class MiddlewareDelegate implements MiddlewareDelegateInterface
{
    /**
     * @var callable
     */
    private $delegate;

    /**
     * @param callable $delegate
     */
    public function __construct(callable $delegate)
    {
        $this->delegate = $delegate;
    }

    /**
     * @param Request $request
     *
     * @return Response
     */
    public function resolve(Request $request)
    {
        return call_user_func($this->delegate, $request);
    }
}

In other words, this doesn't appear to have any value beyond external type-safety - internally, it's still call_user_func() with no safety.

I am somewhat concerned about the performance implications - in micro benchmarks, these dispatchers are going to measurably slower than simpler dispatchers using callables with no object-wrapping. In practice, probably only measurable in micro benchmarks though; in practice, the cost is negligible for sure.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 16, 2016

Member

The existence of a middleware delegate interface does seem to have some merit in terms of offline inspections and IDE support, do you not agree?

Do you have an example? I really don't see it. Here is something concrete: my implementation of StackInterface: https://github.com/stratifyphp/http/blob/master/src/Middleware/Pipe.php How would another interface on top of that class improve anything? (note it's implementing Middleware)

Member

mnapoli commented Aug 16, 2016

The existence of a middleware delegate interface does seem to have some merit in terms of offline inspections and IDE support, do you not agree?

Do you have an example? I really don't see it. Here is something concrete: my implementation of StackInterface: https://github.com/stratifyphp/http/blob/master/src/Middleware/Pipe.php How would another interface on top of that class improve anything? (note it's implementing Middleware)

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 16, 2016

Contributor

@mnapoli callable $next is where this would help - you don't have static analysis or IDE support for calls to $next now, e.g. Pipe.php#49: return $next($request, $response)

By changing this into e.g. MiddlewareDelegateInterface $next, you'd get static analysis and IDE auto-complete for a more concise statement like return $next->process($request, $response).

Even for those who don't use static analysis tools or an IDE, you'd get the benefit of a bad call failing immediately at the call site, as opposed to failing later, or (worse) executing without error and producing a bad result - that sort of thing is much harder to debug.

Contributor

mindplay-dk commented Aug 16, 2016

@mnapoli callable $next is where this would help - you don't have static analysis or IDE support for calls to $next now, e.g. Pipe.php#49: return $next($request, $response)

By changing this into e.g. MiddlewareDelegateInterface $next, you'd get static analysis and IDE auto-complete for a more concise statement like return $next->process($request, $response).

Even for those who don't use static analysis tools or an IDE, you'd get the benefit of a bad call failing immediately at the call site, as opposed to failing later, or (worse) executing without error and producing a bad result - that sort of thing is much harder to debug.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 16, 2016

Member

Oh OK you are talking about FrameInterface, I mentioned StackInterface so here's the confusion :)

In the thread I linked there was this proposal: https://groups.google.com/d/msg/php-fig/V12AAcT_SxE/ZrNIz46zCAAJ What do you think about it? It's a middleground between simplicity ($next($request) is so simple it would be too bad to complicate it just even a little) and type safety:

interface Delegate
{
    public function __invoke(ServerRequestInterface $request) : ResponseInterface;
}

Can be then used like this:

function (ServerRequestInterface $request, Delegate $next) {
    return $next($request);
}

WDYT?

Member

mnapoli commented Aug 16, 2016

Oh OK you are talking about FrameInterface, I mentioned StackInterface so here's the confusion :)

In the thread I linked there was this proposal: https://groups.google.com/d/msg/php-fig/V12AAcT_SxE/ZrNIz46zCAAJ What do you think about it? It's a middleground between simplicity ($next($request) is so simple it would be too bad to complicate it just even a little) and type safety:

interface Delegate
{
    public function __invoke(ServerRequestInterface $request) : ResponseInterface;
}

Can be then used like this:

function (ServerRequestInterface $request, Delegate $next) {
    return $next($request);
}

WDYT?

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 16, 2016

Contributor

@mnapoli the problem with __invoke() is you can't really type-hint against it, or at least only with php-doc, if that means you expect a callable to also pass as an argument... whereas, if you do expect to statically type-hint, and don't expect a callable to work, using __invoke() serves no purpose - and semantically, $next refers to the next middleware, so $next->process() to me reads as telling the next middleware to process, whereas $next(), hmm, doesn't mean much...

Also, some people have voiced concern that reserving __invoke() in this manner prevents you from using __invoke() for your own purposes. I don't so much see the use-case, but either way, I think a "real" interface with a real method is more idiomatic and conventional than __invoke() magic.

Contributor

mindplay-dk commented Aug 16, 2016

@mnapoli the problem with __invoke() is you can't really type-hint against it, or at least only with php-doc, if that means you expect a callable to also pass as an argument... whereas, if you do expect to statically type-hint, and don't expect a callable to work, using __invoke() serves no purpose - and semantically, $next refers to the next middleware, so $next->process() to me reads as telling the next middleware to process, whereas $next(), hmm, doesn't mean much...

Also, some people have voiced concern that reserving __invoke() in this manner prevents you from using __invoke() for your own purposes. I don't so much see the use-case, but either way, I think a "real" interface with a real method is more idiomatic and conventional than __invoke() magic.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 16, 2016

Contributor

On a side note, an interface like this serves more than one purpose:

interface DispatchableInterface
{
    public function dispatch(RequestInterface $request): ResponseInterface;
}

That is, a middleware stack could implement it - or a simple controller could implement it; which means you could use, in some contexts, a simple controller or an entire middleware stack interchangibly, which could create interesting opportunities, as this could work for a bunch of different types of components.

I would not name it StackInterface, since that implies a singular purpose, rather than describing a general facet implemented by dispatchers, controllers, decorators of either, and possibly many other components.

Contributor

mindplay-dk commented Aug 16, 2016

On a side note, an interface like this serves more than one purpose:

interface DispatchableInterface
{
    public function dispatch(RequestInterface $request): ResponseInterface;
}

That is, a middleware stack could implement it - or a simple controller could implement it; which means you could use, in some contexts, a simple controller or an entire middleware stack interchangibly, which could create interesting opportunities, as this could work for a bunch of different types of components.

I would not name it StackInterface, since that implies a singular purpose, rather than describing a general facet implemented by dispatchers, controllers, decorators of either, and possibly many other components.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 16, 2016

Member

if you do expect to statically type-hint, and don't expect a callable to work, using __invoke() serves no purpose

It's simpler and it's very similar to current middlewares: return $next($request). Developers will be familiar with that. With FrameInterface not so much.

whereas $next(), hmm, doesn't mean much...

But… that's what PSR-7 middlewares are doing today: when has it become bad/complex? Everybody uses it and it works so well exactly because it's simple.

We don't need to have names for everything. We struggle finding a name for that thing because it's nothing really, it's just an action: "call the next middleware", there's nothing more to it. We have a solution today, it works, it's simple, let's try to keep its simplicity.

Trying to force it into a concept of a frame or a delegate or something is just adding complexity for no gain. If we reaaaally want type safety (I see no real added value because any good middleware framework will type-hint correctly the $next callable anyway) __invoke() solves the problem.

reserving __invoke() in this manner prevents you from using __invoke() for your own purposes

That's fine. Will there really be an implementation of the "delegate/frame" that needs to be invokable for other reasons? Do we have actual examples of that? (not just theoretical scenarios) And if so, can the project write an adapter?
The delegate object will usually be a very simple one, it will not have a lot of responsibilities (judging from existing implementation).

I would not name it StackInterface, since that implies a singular purpose

I think you are confusing Frame and Stack which are 2 different things. Or are you switching topic?

Member

mnapoli commented Aug 16, 2016

if you do expect to statically type-hint, and don't expect a callable to work, using __invoke() serves no purpose

It's simpler and it's very similar to current middlewares: return $next($request). Developers will be familiar with that. With FrameInterface not so much.

whereas $next(), hmm, doesn't mean much...

But… that's what PSR-7 middlewares are doing today: when has it become bad/complex? Everybody uses it and it works so well exactly because it's simple.

We don't need to have names for everything. We struggle finding a name for that thing because it's nothing really, it's just an action: "call the next middleware", there's nothing more to it. We have a solution today, it works, it's simple, let's try to keep its simplicity.

Trying to force it into a concept of a frame or a delegate or something is just adding complexity for no gain. If we reaaaally want type safety (I see no real added value because any good middleware framework will type-hint correctly the $next callable anyway) __invoke() solves the problem.

reserving __invoke() in this manner prevents you from using __invoke() for your own purposes

That's fine. Will there really be an implementation of the "delegate/frame" that needs to be invokable for other reasons? Do we have actual examples of that? (not just theoretical scenarios) And if so, can the project write an adapter?
The delegate object will usually be a very simple one, it will not have a lot of responsibilities (judging from existing implementation).

I would not name it StackInterface, since that implies a singular purpose

I think you are confusing Frame and Stack which are 2 different things. Or are you switching topic?

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark Aug 16, 2016

Contributor

But… that's what PSR-7 middlewares are doing today

I've heard this argument quite a few times (or in a more general form: everyone is doing that, how can it be wrong?). I don't think it's a valid argument. Someone started it, some other people continued the "standard" and that's why it became so popular. It doesn't mean it's good (or bad), but again: it doesn't make it a valid argument.

Trying to force it into a concept of a frame or a delegate or something is just adding complexity for no gain.

The concept of delegates exist in other languages and I am very happy when I can use a language where it exists. The fact that it doesn't exist in PHP shouldn't keep us from "emulating" the idea. If PHP receives delegates in a future version, it will be easier to adapt.

Will there really be an implementation of the "delegate/frame" that needs to be invokable for other reasons?

No, it's probably bad design, but I still agree with @mindplay-dk. For me using __invoke in an interface is weird. I had the same feeling about __toString in PSR-7.

Contributor

sagikazarmark commented Aug 16, 2016

But… that's what PSR-7 middlewares are doing today

I've heard this argument quite a few times (or in a more general form: everyone is doing that, how can it be wrong?). I don't think it's a valid argument. Someone started it, some other people continued the "standard" and that's why it became so popular. It doesn't mean it's good (or bad), but again: it doesn't make it a valid argument.

Trying to force it into a concept of a frame or a delegate or something is just adding complexity for no gain.

The concept of delegates exist in other languages and I am very happy when I can use a language where it exists. The fact that it doesn't exist in PHP shouldn't keep us from "emulating" the idea. If PHP receives delegates in a future version, it will be easier to adapt.

Will there really be an implementation of the "delegate/frame" that needs to be invokable for other reasons?

No, it's probably bad design, but I still agree with @mindplay-dk. For me using __invoke in an interface is weird. I had the same feeling about __toString in PSR-7.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 17, 2016

Member

👍 I don't have much to add so I'll avoid going circles. Just about this:

I've heard this argument quite a few times (or in a more general form: everyone is doing that, how can it be wrong?). I don't think it's a valid argument.

A standard is not a place for innovation (I personally learnt this with ContainerInterface which was massively rejected years ago). What if we standardize something that sucks (whatever the reason)?

On the other hand past experience is very useful:

  • did it work?
  • was there any problems with it? (I'm happy that $response was removed from the signature for example)

I've never seen $next as a problem that needed solving. The arguments I see here are only theoretical and IMO reality is much more important than theory.

Member

mnapoli commented Aug 17, 2016

👍 I don't have much to add so I'll avoid going circles. Just about this:

I've heard this argument quite a few times (or in a more general form: everyone is doing that, how can it be wrong?). I don't think it's a valid argument.

A standard is not a place for innovation (I personally learnt this with ContainerInterface which was massively rejected years ago). What if we standardize something that sucks (whatever the reason)?

On the other hand past experience is very useful:

  • did it work?
  • was there any problems with it? (I'm happy that $response was removed from the signature for example)

I've never seen $next as a problem that needed solving. The arguments I see here are only theoretical and IMO reality is much more important than theory.

@sagikazarmark

This comment has been minimized.

Show comment
Hide comment
@sagikazarmark

sagikazarmark Aug 17, 2016

Contributor

What if we standardize something that sucks (whatever the reason)?

Although there is a chance for that (see PSR-6), I don't think that could happen because there is no better solution, rather because the development process sucks.

The fact that there are no problems with $next is also not a valid argument against doing something else IMO. There were arguments however why other solutions would be better.

Contributor

sagikazarmark commented Aug 17, 2016

What if we standardize something that sucks (whatever the reason)?

Although there is a chance for that (see PSR-6), I don't think that could happen because there is no better solution, rather because the development process sucks.

The fact that there are no problems with $next is also not a valid argument against doing something else IMO. There were arguments however why other solutions would be better.

@mindplay-dk

This comment has been minimized.

Show comment
Hide comment
@mindplay-dk

mindplay-dk Aug 17, 2016

Contributor

@mnapoli

The arguments I see here are only theoretical and IMO reality is much more important than theory

in my opinion, using an idiomatic interface/method does have an advantage: you can type-hint it for static analysis and IDE support. And it will fail early, which makes for easier debugging. Both of those things are practical, not theoretical.

The only advantage of __invoke() magic over a real interface, is it's familiar to middleware authors - but so are regular interfaces, and not just in the context of middleware, but in PHP in general, which is why I say it's more idiomatic.

Sure, a callable is less of a surprise to those already familiar with de-facto PSR-7 middleware, but this standard is already something else entirely - and to everyone else, the callable requires an explanation, whereas an interface is the explanation.

A standard is not a place for innovation

What could be less innovative that a plain old interface?

A standard should be more than a formalization of the familiar - if we can do something better/cleaner and fix mistakes of the past, now is the only opportunity to do that. We've already (most of us) crossed the bridge from double-pass to lambda-style, which was an important step, and a massive break from the de-facto standard, which will benefit the community in the long run.

Contributor

mindplay-dk commented Aug 17, 2016

@mnapoli

The arguments I see here are only theoretical and IMO reality is much more important than theory

in my opinion, using an idiomatic interface/method does have an advantage: you can type-hint it for static analysis and IDE support. And it will fail early, which makes for easier debugging. Both of those things are practical, not theoretical.

The only advantage of __invoke() magic over a real interface, is it's familiar to middleware authors - but so are regular interfaces, and not just in the context of middleware, but in PHP in general, which is why I say it's more idiomatic.

Sure, a callable is less of a surprise to those already familiar with de-facto PSR-7 middleware, but this standard is already something else entirely - and to everyone else, the callable requires an explanation, whereas an interface is the explanation.

A standard is not a place for innovation

What could be less innovative that a plain old interface?

A standard should be more than a formalization of the familiar - if we can do something better/cleaner and fix mistakes of the past, now is the only opportunity to do that. We've already (most of us) crossed the bridge from double-pass to lambda-style, which was an important step, and a massive break from the de-facto standard, which will benefit the community in the long run.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 17, 2016

Member

@sagikazarmark I'm not against improvements, but the only way to be sure something is stable/good enough is to try it out in real scenarios. If we innovate, we must then take some time (months/years?) to try that out, else (yes) we risk PSR-6 again. Even PSR-7 has issues that we only discovered when actually using it widely.

@mindplay-dk

you can type-hint it for static analysis and IDE support

I agree with that.

And it will fail early, which makes for easier debugging

I don't agree with this: all stack/pipe implementations I've seen type-hint properly the arguments so it will fail early (https://github.com/zendframework/zend-stratigility/blob/master/src/Next.php#L78, https://github.com/slimphp/Slim/blob/3.x/Slim/MiddlewareAwareTrait.php#L66) if you pass something wrong.

Sure, a callable is less of a surprise to those already familiar with de-facto PSR-7 middleware, but this standard is already something else entirely - and to everyone else, the callable requires an explanation, whereas an interface is the explanation.

I could agree with that if FrameInterface wasn't the root of the problem :) We cannot succeed (yet) to make that interface the explanation because it's confusing.

What could be less innovative that a plain old interface?

By that logic it's impossible to do something new unless we add new operators/concepts to the language.

Member

mnapoli commented Aug 17, 2016

@sagikazarmark I'm not against improvements, but the only way to be sure something is stable/good enough is to try it out in real scenarios. If we innovate, we must then take some time (months/years?) to try that out, else (yes) we risk PSR-6 again. Even PSR-7 has issues that we only discovered when actually using it widely.

@mindplay-dk

you can type-hint it for static analysis and IDE support

I agree with that.

And it will fail early, which makes for easier debugging

I don't agree with this: all stack/pipe implementations I've seen type-hint properly the arguments so it will fail early (https://github.com/zendframework/zend-stratigility/blob/master/src/Next.php#L78, https://github.com/slimphp/Slim/blob/3.x/Slim/MiddlewareAwareTrait.php#L66) if you pass something wrong.

Sure, a callable is less of a surprise to those already familiar with de-facto PSR-7 middleware, but this standard is already something else entirely - and to everyone else, the callable requires an explanation, whereas an interface is the explanation.

I could agree with that if FrameInterface wasn't the root of the problem :) We cannot succeed (yet) to make that interface the explanation because it's confusing.

What could be less innovative that a plain old interface?

By that logic it's impossible to do something new unless we add new operators/concepts to the language.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 17, 2016

Contributor

@mnapoli I've commented on this extensively on the list, but I'll summarize here:

  • Making it a pure callable makes it potentially incompatible with existing stacks (particularly if they are currently double pass), which would hamper adoption by existing libraries.
  • Defining __invoke() as the interface method similarly hampers adoption when libraries already define this, as many would have to change in backwards incompatible ways in order to accommodate existing middleware as well as middleware targeting this specification. Having a discrete, unique method ensures these can interoperate immediately, and allows tools to be built to help users migrate their middleware to work with the specification.
Contributor

weierophinney commented Aug 17, 2016

@mnapoli I've commented on this extensively on the list, but I'll summarize here:

  • Making it a pure callable makes it potentially incompatible with existing stacks (particularly if they are currently double pass), which would hamper adoption by existing libraries.
  • Defining __invoke() as the interface method similarly hampers adoption when libraries already define this, as many would have to change in backwards incompatible ways in order to accommodate existing middleware as well as middleware targeting this specification. Having a discrete, unique method ensures these can interoperate immediately, and allows tools to be built to help users migrate their middleware to work with the specification.
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 17, 2016

Member

@weierophinney Just to be sure: are you talking about FrameInterface?

Member

mnapoli commented Aug 17, 2016

@weierophinney Just to be sure: are you talking about FrameInterface?

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 17, 2016

Contributor

@mnapoli Yes, the current FrameInterface.

Contributor

weierophinney commented Aug 17, 2016

@mnapoli Yes, the current FrameInterface.

mnapoli added a commit that referenced this pull request Aug 17, 2016

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 17, 2016

Member

@weierophinney your response in the mailing list thread was great, I wasn't expecting existing middlewares to work with PSR-15 stacks (and the other way around), so I had no idea you wanted to make the 2 systems work together at that level.

I've opened a PR to document all that: #807 Reviews are welcome.

Member

mnapoli commented Aug 17, 2016

@weierophinney your response in the mailing list thread was great, I wasn't expecting existing middlewares to work with PSR-15 stacks (and the other way around), so I had no idea you wanted to make the 2 systems work together at that level.

I've opened a PR to document all that: #807 Reviews are welcome.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 17, 2016

Contributor

I wasn't expecting existing middlewares to work with PSR-15 stacks (and the other way around)

I'd been wanting invokables as well, but when I started considering what it would take to make the transition within Expressive or Slim, I realized that doing so made migration substantially harder. Having just come off of Zend Framework v2 -> v3 transitions, I recognized the same challenges we had there, as well as the same opportunities to make the transition easier for both library authors and consumers.

Contributor

weierophinney commented Aug 17, 2016

I wasn't expecting existing middlewares to work with PSR-15 stacks (and the other way around)

I'd been wanting invokables as well, but when I started considering what it would take to make the transition within Expressive or Slim, I realized that doing so made migration substantially harder. Having just come off of Zend Framework v2 -> v3 transitions, I recognized the same challenges we had there, as well as the same opportunities to make the transition easier for both library authors and consumers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment