-
Notifications
You must be signed in to change notification settings - Fork 270
[WIP] Guzzle 5.x support #138
Conversation
|
Hi @krzaczek and thanks for your amazing work! That is a huge PR and reviewing it will take some time, but we'll get there! Don't worry if you don't get an answer in the next few days, we'll make sure to get back to you in due time :) |
|
Hi @ubermuda ... cool ... I'm waiting for comments :) |
|
👍 |
|
@ubermuda I have added better exceptions handling that is compatible with current version. All exceptions are thrown as Docker\Exception\APIException |
|
Hi @krzaczek, first of all thanks for this PR. However our goal for the http client is to use this library : https://github.com/php-http/httplug so there is no conflict / duplication if someone use guzzle 5, or guzzle 6 or zend/http or .... (you see my point) Is it too much to ask to rework this PR by using this library ? This is my mind for a long time (abstraction of http client) as it's real pain actually, so it will be a huge win to have this. |
|
Well I need this .. as Guzzle 4.x conflicts with aws-sdk -- so sure ... but I don't know httpplug .. so this my take some time ... You ma close this PR ... i'll start a new one when I'm ready |
|
I see it's "early" development - alpha - hmmmm |
|
I'm one the maintainer and our goal is to have something stable in the beginning of 2016, in fact i start to work on this for docker-php (and also other libraries) Let's keep this open for the moment as a reminder |
|
I think it will be best in the future use this https://github.com/php-http/httplug But now this upgrade required, cause guzzle 4 now is very rare version. |
|
@krzaczek I add time to finally handle the refactoring of the library, there is now a 1.x branch which should be the stable release and maintained with docker remote api update. It does not require guzzle anymore, so if you want to give it a try (be aware that it's completly BC from the actual documentation / library) |
|
Yes I know .. i'll give it a try tommorow pawel (at) mobile
|
|
@joelwurtz Works perfectly :) We can close this PR now. |
|
Wondering also, will you be interested if it doesn't define a specific client implementation (so you can use guzzle instead of the socket client ?). Counterpart, some calls and features will not work with all clients implementation, like connecting to a unix socket domain, ssl or real time feature. |
|
Well like You said .. it won't 100% work on any other client .. so I don't think You should allow it. Since socket support is compiled in PHP by default what's the point ? :) |
|
Ps. I do have a bug but in https://github.com/php-http/message/blob/master/src/Encoding/FilteredStream.php#L118 $buf = $this->read(1048576);So a code $body = $logs->getBody()->getContents();Fails with an exception while(!$body->eof()) {
$log .= $body->read(4096);
}I Can read this stream manually ... and then it works |
|
Yeah it's not really a bug as it's trying to read the full message, but since you can have chunked message, it's possible to have more than 256 calls to the read function, i will lower the buf size to 8192, as afterall it does not make any sense to have less or more in php if nothing is setted (php already have a internal buffer of 8192 bytes) |
|
So anyway really thanks for the PR even if it didn't make to the master, it may have push me to move thing faster :). |
Guzzle library upgrade to 5.3
This is a PR to upgrade guzzle library form version 4.x to 5.3. I did this because ver 4 of guzzle conflicts with lots of new libraries like aws/aws-sdk-php
Decided to pick Guzzle 5.3 since it supports PHP 5.4 and I didn't want to make any BC
Please review the code and let me know what You think of it :)