reactphp / http

Event-driven, streaming HTTP client and server implementation for ReactPHP.
https://reactphp.org/http/
MIT License
747 stars 143 forks source link

Add Browser middleware #492

Closed R4c00n closed 8 months ago

R4c00n commented 1 year ago

Inspired by Guzzle middlewares I added a middleware system to the Browser component. I took the already existing \React\Http\Io\MiddlewareRunner as an example for this.

Example usage:

class MyCustomMiddleware implements \React\Http\Browser\MiddlewareInterface
{
  public function __invoke(RequestInterface $request, callable $next)
  {
    return $next($request->withHeader('X-Foo', 'Bar')); // I know this is also possible another way, but just as a demo
  }
}

$client = (new Browser())->withMiddleware(new MyCustomMiddleware());
$client->get('https://example.com/api/v1/demo');

My concrete use case right now is that I want to keep track of the last request for advanced logging. I do this by storing request information in a custom middleware.

Please let me know what you think and if I should apply some changes to my code in order for this to get merged :)

R4c00n commented 1 year ago

There seems to be a segmentation fault in the PHP8.1 build.

I also don't know what the error in the PHP5.3 builds wants to tell me.

Fatal error: Declaration of React\Tests\Http\BrowserMiddlewareStub::__invoke() must be compatible with that of React\Http\Browser\MiddlewareInterface::__invoke() in /home/runner/work/http/http/tests/BrowserMiddlewareStub.php on line 11

The signature is the same as in the interface. 3v4l also says yes https://3v4l.org/L7P2V#v5.3.29

WyriHaximus commented 1 year ago

Done this some years ago over at https://github.com/orgs/php-api-clients/repositories?q=middleware-&type=all&language=&sort= and would love to see this land directly in here instead of building on top of it. Things like gzip and other compressions could also benefit from this. Other things that come to mind are metrics about requests per route/method/whatever, tracing, or authentication. Maybe even #445 could benefit from this before it lands directly in this package.

This, IMHO would be a great feature candidate for our next major: v3.

R4c00n commented 1 year ago

Hi @WyriHaximus, happy to hear that this feature can be useful for others as well. But the next new major version would be v2, not v3, wouldn't it?

WyriHaximus commented 1 year ago

Hi @WyriHaximus, happy to hear that this feature can be useful for others as well.

Well, I'm not the only one to convince, talked about this with @clue and @SimonFrings in the past and we had other features to include back then.

But the next new major version would be v2, not v3, wouldn't it?

We went with v3 instead of v2 for aesthetic reasons and to keep it in line with other packages already having a v2. With v3 we're pulling everything on the same minimum version again.

SimonFrings commented 1 year ago

@R4c00n Thanks for opening this PR, this is definitely a cool feature and something I want to see being part of this component! 👍

I also don't know what the error in the PHP5.3 builds wants to tell me.

It seems like it has some problem with the invoke() of your interface, but I think we don't even need a new interface for this and ultimately will resolve the problem with PHP 5.3.

But the next new major version would be v2, not v3, wouldn't it?

As @WyriHaximus said, we decided to go for v3 instead of v2 to keep our versioning consistent across all our projects (especially because of promise v3), you can read all about our decision here: https://github.com/orgs/reactphp/discussions/472#discussioncomment-3680968

Also a little heads up, once all tests are green and this is ready to merge, make sure to combine your commits into one. Nothing to worry about now, I think it makes most sense to do this, once everything is in and functional ;)

SimonFrings commented 8 months ago

It seems like this ticket is open for quite a while now and haven't received any updates since. To avoid issues and pull request laying around for too long, I'll close this for now.

We're currently working towards a v1.10.0 of our HTTP component and once this is released, we can start with the feature implementations for HTTP v3 as discussed in #517. As @WyriHaximus said above, this could be a good v3 candidate so we can reopen/revisit this once we're sure this will be part of the v3 release. If this doesn't involve any BC breaks, it's also possible to add this in a following v3.1, v3.2, etc. 👍