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

Add Browser middleware #492

Closed
wants to merge 4 commits into from
Closed

Conversation

R4c00n
Copy link

@R4c00n R4c00n commented Mar 13, 2023

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
Copy link
Author

R4c00n commented Mar 13, 2023

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
Copy link
Member

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
Copy link
Author

R4c00n commented Mar 14, 2023

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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. 👍

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

Successfully merging this pull request may close these issues.

3 participants