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

Middleware #141

Closed
wants to merge 4 commits into from
Closed

Middleware #141

wants to merge 4 commits into from

Conversation

bartvanhoutte
Copy link

Based on the middleware implementation in kriswallsmith/Buzz. Only supports intercepting the Request so far.

This was referenced Nov 22, 2019
@bartvanhoutte
Copy link
Author

I've cloned master and the test that fails in this PR fails on master as well. I can't figure out on how to make it work at the moment. However, I was able to fix PHP 5.3 compatibility after some tinkering in Docker.

@clue
Copy link
Owner

clue commented Nov 29, 2019

@bartvanhoutte Thanks for looking into this, this looks like a good starting point 👍

Let me look into the build error first, this seems to be unrelated to your PR (and also affects #142).

@jcchavezs
Copy link

@clue @bartvanhoutte any idea on when/how to restart this? Maybe rebasing is first good step?

@clue
Copy link
Owner

clue commented Jun 20, 2020

Thanks for your patience!

The unrelated build error has been fixed via #145 in the meantime, so the build should now be able to pass. A rebase may be required to trigger a new build.

I still think this feature makes perfect sense and I think this PR is a good starting point. It doesn't currently contain any tests and very little documentation, so this is definitely something that needs to be worked out.

From an API perspective, I like the withMiddleware() approach (this is in line with the new options API planned in #154). I'm not sure I like the MiddlewareInterface because I'd rather KISS. Perhaps we can use a simple callable definition like we're doing in https://github.com/reactphp/http#middleware? Among others, this also allows working with both the outgoing request and incoming response objects.

Another thing that has yet to be worked out is how this interferes with existing options like redirects, timeouts and compression (#49). It's my understanding this should be handled "internally", i.e. the middleware will only see the outgoing request and the redirected response in return when redirects are used (can be disabled of course).

What do you think?

@clue
Copy link
Owner

clue commented Jul 12, 2020

Again thanks for bringing this up! I have to close this one as per #177 now that future development will focus on https://github.com/reactphp/http instead. If you feel this makes sense to port over, please file a new PR over in the new repo and I'm happy to help review this 👍

@clue clue closed this Jul 12, 2020
@jcchavezs
Copy link

jcchavezs commented Jul 12, 2020 via email

@clue
Copy link
Owner

clue commented Jul 12, 2020

New PRs are very much welcome! 👍

@jcchavezs
Copy link

I am sorry. I somehow thought there was an issue for this. I might create one.

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