Skip to content
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

[RFC] Middleware, where do they belong #221

Closed
jsor opened this issue Sep 14, 2017 · 7 comments
Closed

[RFC] Middleware, where do they belong #221

jsor opened this issue Sep 14, 2017 · 7 comments

Comments

@jsor
Copy link
Member

jsor commented Sep 14, 2017

We now have a middleware system merged into this package (#215 🎉) and also our first contributed middleware implementation (#219 by @christoph-kluge 🎉), which is awesome! But i think we should discuss and decide first where and how we continue with the middleware system.

1. Decide where the middleware system lives

The middleware system is completely decoupled from the Server and it might be an option to move it to a separate package.

  1. Move middleware to a separate package, eg react/http-middleware

    We have 2 (or 3, if http-client gets merged into this package, Merge HttpClient component into this component #148) layers which have to be maintained in a single repository. This could make versioning difficult because introducing a BC break in one layer requires a new major version for the other layers too. A separate package decouples the version of the middleware layer from react/http.

    Having the middleware layer in its own package can also encourage other middleware systems or application frameworks (Express.php anyone?) to arise. This is also possible having default middleware implementations shipped with this package, but it reduces confusion if a separate package can depend on react/http without a built-in middleware system which might be incompatible with the third-party system.

  2. Keep middlware in react/http

    Provides a good DX because one can start with everything available out-of-the-box.

2. Define requirements for built-in middleware implementations

We should define what middleware implementations are built-in (in react/http) or provided by ReactPHP (in react/http-middleware) itself. We should document that so it's clear if a middlerware is suited for the ReactPHP repo and a PR is desired or if it's better suited for a third-party package.

  1. Limit middleware implementations and provide clear guidelines

    Such a guideline could be: We provide built-in middleware implementations to replicate PHP's default SAPI behaviour. That means, everything required to setup a PSR-7 ServerRequestInterface as it would be initialized from PHP superglobals.

    Example from zend-diactoros

    https://github.com/zendframework/zend-diactoros/blob/fb7f06e1b78c2aa17d08f30633bb2fa337428182/src/ServerRequestFactory.php#L50-L77

  2. No Limits! Keep 'em comig

In both cases, we should add a Wiki page for third-party middleware similar to https://github.com/reactphp/react/wiki/Users

@WyriHaximus
Copy link
Member

In both cases, we should add a Wiki page for third-party middleware similar to https://github.com/reactphp/react/wiki/Users

Huge plus on this 👍 been playing with middlewares over the last few days and already worked out a few simple ones (routing is among them).

But IMHO all the middleware we need to create a HTTP server fully compatible with PHP should be in the package. With regards to the http-client merging into this package we could do something like this where we have middlewares for both the client and server but also specific middlewares for only the client OR the server:

  • Middleware:
    • RequestBodyBuffer
    • Server:
    • Client:
      • SomeClientOnlyMiddleware

@clue
Copy link
Member

clue commented Sep 17, 2017

@jsor I think you're raising a very good discussion here!

1.2. Keep middlware in react/http

Given the current scope of the middleware handlers I would vote for this option 👍 The way they are currently designed (being just a callable essentially) would technically allow them to also live outside of this project. But given that they're an essential feature for full PSR-7 compatibility, it appears sounds to keep them close to the "core" of this library.

2.1. Limit middleware implementations and provide clear guidelines
Such a guideline could be: We provide built-in middleware implementations to replicate PHP's default SAPI behaviour. That means, everything required to setup a PSR-7 ServerRequestInterface as it would be initialized from PHP superglobals.

Yes, please! I think we've already set a successful precedence for this in our other packages, so it only makes sense to follow this here 👍 I would like to actively encourage people to build their own middleware handlers and promote them under their own brand. This way they can be built in a reusable way for each and every possible (and maybe highly specific) use-case 👍

I agree that getting all kind of middleware handlers in here would make this project a moving target given the vast amount of different use cases. Also, I firmly believe that this would actually hinder the adoption as one would have to cater for a lot of edge cases that may not be related to the specific use case.

Should we eventually find a set of middleware handlers that are common to a lot of use cases, we can still safely prepare some kind of meta package specifically for this and/or even get this in here. This is not possible the other way around: Should we decide to get everything in here and eventually find out we would like to remove some of these, this would result in a BC break.

I would like to hear some more thoughts from other people on this, ping @christoph-kluge, @mbonneau, what do you think?

@WyriHaximus I think we agree on this matter, but I'm not so sure about "client middleware". Does it make sense to keep this discussion separated for now? I feel it's out of scope currently and would much rather discuss this in the context of the client first.

@christoph-kluge
Copy link
Contributor

@clue thanks for mentioning here and asking for my opinion. I'm still on my way to figure out the full scope of reactphp in order get an overview how things are interconnected. Hope that my feedback will give value to the discussion. :)

  1. Decide where the middleware system lives

We could support both react/http and react/http-middleware but each for it's own matters. In such a scenario I would expect that react/http should contain all requirements to achieve HTTP 1.0/1.1/2.0 support for PSR-7 and PHP SAPI in order to have an out-of-the-box running http-server.

  • new Server(new SAPIMiddlewareRunner('index.php'))
  • new Server(new PSR7MiddlewareRunner(function(Request $request) { return new Response(200)); }))

Whereas react/http-middleware might then act as a collection for different use-cases to support a broader coverage. Something similar like php-pm is doing. They provide the basics in php-pm/php-pm and add extensions in different repositories within the same namespace (php-pm/php-pm-httpkernel) to make the entry-points for new developers as easy as possible w/o searching for it..

Such a scenario would allow us to have the core in react/http, cover some basics in custom repositories and keep the ability for people to bake their custom solutions.

@WyriHaximus
Copy link
Member

@clue Yeah we are on the same page, the client middleware is working out great for @php-api-clients but I'm unsure about doing that in a low level package like react/http(-client). Was just braindumping an idea and thinking ahead a bit 😄 . But for now keeping it simple and just assume server middlewares.

What about setting up a section for middleware on https://github.com/reactphp/react/wiki/Users or create https://github.com/reactphp/react/wiki/HttpMiddlewareUsers when it grows out of https://github.com/reactphp/react/wiki/Users

This package should, IMHO, only contain middleware to create a working HTTP 1.0, 1.1, and 2.0 server for PHP.

@jsor
Copy link
Member Author

jsor commented Sep 27, 2017

To summarize:

  1. Middleware stay in react/http.
  2. We only provide built-in middleware implementations in react/http required to setup a PSR-7 ServerRequestInterface as it would be initialized from PHP superglobals.
  3. We create a wiki page https://github.com/reactphp/http/wiki/Middleware where everyone can list third-party middleware implementations.

Everyone ok with that?

@WyriHaximus
Copy link
Member

Sounds good to me 👍

@jsor
Copy link
Member Author

jsor commented Sep 27, 2017

:shipit:

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

No branches or pull requests

4 participants