RequestException - check if readable before access

[SpacePossum]

Hi all!

Thanks for the great package :)
First time PR so please bear with me ;)

I target master here because the issue is not on 5.x or lower it seems.
In my project I attempted to do download file by doing something like:

<?php

require_once __DIR__.'/vendor/autoload.php';

$client = new \GuzzleHttp\Client();
$resource = fopen(__DIR__.'/tmp.txt', 'wb');
$response = $client->request(
    'GET',
    'https://github.com/guzzle/guzzle/tree/5.3_DOES_NOT_EXISTS',
    ['sink' => $resource]
);

currently this results in

$ php tmp.php 
PHP Fatal error:  Uncaught RuntimeException: Cannot read from non-readable stream in /home/possum/work/guzzle/vendor/guzzlehttp/psr7/src/Stream.php:208
Stack trace:
#0 /home/possum/work/guzzle/src/Exception/RequestException.php(139): GuzzleHttp\Psr7\Stream->read(120)
#1 /home/possum/work/guzzle/src/Exception/RequestException.php(107): GuzzleHttp\Exception\RequestException::getResponseBodySummary(Object(GuzzleHttp\Psr7\Response))
#2 /home/possum/work/guzzle/src/Middleware.php(66): GuzzleHttp\Exception\RequestException::create(Object(GuzzleHttp\Psr7\Request), Object(GuzzleHttp\Psr7\Response))
#3 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/Promise.php(203): GuzzleHttp\Middleware::GuzzleHttp\{closure}(Object(GuzzleHttp\Psr7\Response))
#4 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/Promise.php(156): GuzzleHttp\Promise\Promise::callHandler(1, Object(GuzzleHttp\Psr7\Response), Array)
#5 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/TaskQueue.php(47): GuzzleHttp\Promise\Promise::GuzzleHttp\Promise\{cl in /home/possum/work/guzzle/vendor/guzzlehttp/psr7/src/Stream.php on line 208

with this patch:

$ php tmp.php 
PHP Fatal error:  Uncaught GuzzleHttp\Exception\ClientException: Client error: `GET https://github.com/guzzle/guzzle/tree/5.3_DOES_NOT_EXISTS` resulted in a `404 Not Found` response in /home/possum/work/guzzle/src/Exception/RequestException.php:113
Stack trace:
#0 /home/possum/work/guzzle/src/Middleware.php(66): GuzzleHttp\Exception\RequestException::create(Object(GuzzleHttp\Psr7\Request), Object(GuzzleHttp\Psr7\Response))
#1 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/Promise.php(203): GuzzleHttp\Middleware::GuzzleHttp\{closure}(Object(GuzzleHttp\Psr7\Response))
#2 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/Promise.php(156): GuzzleHttp\Promise\Promise::callHandler(1, Object(GuzzleHttp\Psr7\Response), Array)
#3 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/TaskQueue.php(47): GuzzleHttp\Promise\Promise::GuzzleHttp\Promise\{closure}()
#4 /home/possum/work/guzzle/vendor/guzzlehttp/promises/src/Promise.php(246): GuzzleHttp\Promise\TaskQueue->run(true)
#5 /home/possum/work/guzzle/vendor/guzzlehttp in /home/possum/work/guzzle/src/Exception/RequestException.php on line 113

I haven't made a unit test yet because I'm wondering if the proposed solution is the correct one. I've my doubts that is OK if a Stream isSeekable but yet not isReadable. So maybe this needs to be solved within the Stream class itself (isSeekable always returns false if not isReadable) or maybe the Stream is not created correctly?

Some meta data;

$ php -v
PHP 7.2.5-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: May  5 2018 04:59:13) ( NTS )

let me know what you guys think :)

[SpacePossum]

@sagikazarmark anything I can do here?

[sagikazarmark]

@SpacePossum sorry for the late response.

This seems to be a proper patch, although (as you also said) it lacks tests which would be nice before merging.

I believe a seekable, non-readable stream is perfectly valid (eg. you just want to write content, but never read from the same stream, maybe it's a file that will be read by some other process).

[SpacePossum]

thanks @sagikazarmark for the response, much appreciated :)
I've added some tests

[sagikazarmark]

Thanks @SpacePossum

Sorry for blocking the PR, but I would prefer if you could make some changes in the tests. Thanks!

[SpacePossum]

@sagikazarmark thanks for the review, I've updated the test, let me know what you think when you the time, no rush :)

[sagikazarmark]

Unfortunately anonymous classes are not supported on PHP 5.6 and we still provide support for that version sadly.

[SpacePossum]

I've updated the test to support PHP 5.6

[SpacePossum]

@sagikazarmark thanks for the pointers, all green now :D

[Nyholm]

Thank you

[SpacePossum]

thanks @Nyholm let me know if there is anything left for me to do here, ❤️ to see this in :)

[Nyholm]

Lets wait for Mark to give his final blessing.

[sagikazarmark]

LGTM, thanks a lot!

[Nyholm]

Great work. Thank you.

[SpacePossum]

thanks @Nyholm and @sagikazarmark :D

[wirwolf]

Hello @SpacePossum, i create a fork of this repo and release this marge request into the new version(6.4.0).

Namespace compatibility is 100%, and you can use my changes if you patch composer.json in your projects

• replace "guzzlehttp/guzzle": "^6.3" to "someblackmagic/guzzle": "^6.4"
• add "guzzlehttp/guzzle": "6.3.*" into replace block

Fork link: https://github.com/SomeBlackMagic/guzzle/

[SpacePossum]

Thanks @wirwolf !

I do hope this fix gets shipped with the regular guzzlehttp/guzzle package as well or is this project no longer maintained?

[Nyholm]

It is. We will release a new version later today.

[SpacePossum]

nice :) thanks so much!