Skip to content

[WIP] Client / HttpClient decoupling for Guzzle integration #30

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

Closed

Conversation

romainneutron
Copy link
Contributor

Hello,

this PR decouples the relation between Client and HttpClientInterface. This will allow to use Guzzle as HttpClient instead of Buzz.

The improvement is pretty good : when running multiple request in the same PHP script, Guzzle use CurlMulti as base request handler, using HTTP 1.1 at its best (all requests are done inside the same connection) whereas Buzz does not support it (it opens a new connection on every request).

Here are the results of my mini benchmark for 50 requests :

Buzz ~ 1min
Guzzle ~ 15seconds

I open this PR now as it will introduce some BC breaks and lots of changes in the code, comments and feedbacks welcomed !

todo :

  • Tests
  • BC break documentation
  • Update documentation

$this->httpClient->authenticate($authMethod, $tokenOrLogin, $password);
}

public function executeCommand($method, $command, $parameters, $headers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably better to rename to executeRequest

@dovy
Copy link

dovy commented Feb 5, 2013

I'd love to see this finished off. ;)

@TitanKing
Copy link

Hey guys, we are planning to use this pull request (in a very big project, performance essential) rather than Buzz (which seemed to be dead), does anyone have an idea if this will be finished soon? It would be a great step forward.

Will this eventually replace Buzz or does this serve as an alternative?

@romainneutron
Copy link
Contributor Author

Hello @TitanKing !

I just have to finish to write some tests.... Unfortunately, I got a lot of work at the moment.

I'll ping you as soon as it's ready

@TitanKing
Copy link

Bleh... who cares about tests :D, do you think it is safe for us to just continue using it? Will this eventually replace Buzz as the default?

@romainneutron
Copy link
Contributor Author

You should use the Buzz client for the moment and wait for a stable release.

If you really want to test this implementation, feel free to use my repo in your project

@TitanKing
Copy link

Thanks man will do, your a star.

@pilot
Copy link
Contributor

pilot commented Aug 30, 2013

@romainneutron how is going? have a chance to see that you finish it?

@romainneutron
Copy link
Contributor Author

Well, I got lot of work on other projects. I've spent some time here, but this library is quite coupled with Buzz and I've not finished to port everything.
IIRC, unit tests and filters need to be updated. As this PR introduces a major BC break, it should be also documented.
I've not planned to push some work on the upcoming weeks. If somebody wants to get my commits and push them, I'd be glad.

@cursedcoder
Copy link
Contributor

closing because of #80

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

Successfully merging this pull request may close these issues.

5 participants