Added new client: Samson\Protocol\Protocol\HTTP #64

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants

Burgov commented Apr 11, 2012

This PR adds an optional dependency: samson/protocol.

The problem with Curl is that it isn't available on all installations and file_get_contents refuses to actually fetch contents if the return status is any other dan 2xx, meaning you can't get the actual error message for, for example, a 404 or 500 response.

This adds a third client, named Protocol. This one tries to do an HTTP request using bare sockets. The socket classes appear to be more common in PHP installations than CURL, so it should almost always work.

I ran the tests using the server.php script and they work as expected.

composer.json
@@ -16,7 +16,8 @@
"php": ">=5.3.0"
},
"suggest": {
- "ext-curl": "*"
+ "ext-curl": "*",
+ "samson/protocol": "dev-master"
@stof

stof Apr 11, 2012

Contributor

I don't think you should suggest dev-master explicitly as this version will change. You should either use a constraint targeting releases (if there is any) or * IMO

test/Buzz/Test/Client/FunctionalTest.php
);
+
+ if (class_exists('Samson\Protocol\Protocol\HTTP', true)) {
@stof

stof Apr 11, 2012

Contributor

you should remove the second argument as it is its default value

lib/Buzz/Client/Protocol.php
+
+use Buzz\Message;
+use Samson\Protocol\Protocol\HTTP;
+class Protocol extends AbstractClient implements ClientInterface
@stof

stof Apr 11, 2012

Contributor

missing empty line between the use statement and the class definition

lib/Buzz/Client/Protocol.php
+ }
+
+ public function send(Message\Request $request, Message\Response $response)
+ {
@stof

stof Apr 11, 2012

Contributor

you should set the timeout according to the value configured in the client (inherited from AbstractClient)

lib/Buzz/Client/Protocol.php
+ public function __construct()
+ {
+ $this->http = new HTTP();
+ $this->http->setTimeout($this->getTimeout());
@stof

stof Apr 11, 2012

Contributor

this is wrong. The timeout is set by a setter, so after the constructor

Contributor

stof commented Apr 11, 2012

I don't like the way your library handles errors: it does not handle them and break the whole app in such case. die when the response is invalid is a no-go IMO

Burgov commented Apr 11, 2012

@stof: you're right. I'll create an issue right away and fix it soon. Actually I wrote the script over a year ago and ported it into the library you see now, but I haven't had time to really polish it up yet. Hopefully I will be able to do it this evening

Burgov commented Apr 11, 2012

the die is now replaced by a proper exception

Contributor

stof commented Apr 11, 2012

@Burgov you should implement the ignore_error in the adapter then.

And btw, does your library follow redirections ? If no, the adapter need to handle it

Burgov added some commits Apr 12, 2012

Protocol now handles redirection and throws the same error curl does …
…when it reaches its limit.

Also tests were added for redirection control
Owner

kriswallsmith commented Jun 24, 2012

Please break this into smaller PRs.

Contributor

stof commented Jun 24, 2012

@kriswallsmith smaller PRs ? This only adds one new client implementation, and its tests

Owner

kriswallsmith commented Jun 24, 2012

The change to how PHPUnit sets up autoloading should be a separate PR.

Owner

kriswallsmith commented Jun 24, 2012

I should also say I'm hesitant to merge the new client. I would rather introduce a stream client than integrate with a vendor for that.

Baachi commented Jun 24, 2012

I agree with @kriswallsmith.
I would also prefer a stream or socket client without a vendor.

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