Non PSR-18 exceptions

[gmponos]

Q	A
Bug?	yes
New Feature?	no
Version	2.0

Hello,

According to PSR-18

A Client MUST throw an instance of Psr\Http\Client\ClientExceptionInterface if and only if it is unable to send the HTTP request at all or if the HTTP response could not be parsed into a PSR-7 response object.

There was a tag made today that Guzzle-6 adapter is PSR-18 compliant https://github.com/php-http/guzzle6-adapter/releases/tag/v2.0.0

I am trying to understand how this works since there are cases that guzzle throws non PSR exception.

https://github.com/guzzle/guzzle/blob/master/src/Handler/StreamHandler.php#L55
https://github.com/guzzle/guzzle/blob/master/src/Handler/StreamHandler.php#L252
https://github.com/guzzle/guzzle/blob/master/src/Handler/CurlFactory.php#L458
https://github.com/guzzle/guzzle/blob/master/src/Handler/CurlFactory.php#L531

Maybe I am wrong or I miss something..

[sagikazarmark]

Guzzle exceptions are handled here:

https://github.com/php-http/guzzle6-adapter/blob/master/src/Promise.php#L114

On the other hand, some exceptions are being thrown here: https://github.com/php-http/guzzle6-adapter/blob/master/src/Promise.php#L65

[gmponos]

OK I found out what I missed

        try {
            return Promise\promise_for($handler($request, $options));
        } catch (\Exception $e) {
            return Promise\rejection_for($e);
        }

The exception is always wrapped with a promise inside guzzle.

Anyway what you pointed about this is wrong right? So at least this issue was not created for nothing 😄

[mekras]

Currently (version 2.0.1) Http\Client\Exception\HttpException exception is thrown at least on 400 response. This is not compatible with PSR-18.

[dbu]

in version 2 of httplug, the exceptions are compatible with PSR-18: HttpException extends RequestException which implements the psr exception.

[mekras]

It's not about class compatibility, it's about behavior compatibility. The exception should not be thrown if the request is successfully sent and the response is successfully received. Even if it is a response with an error message.

[dbu]

oh, sorry i did not read that right. yeah that should not happen. we should be configuring guzzle to not throw exceptions on http level errors. are you sure we have that problem? if you pass a guzzle instance to the adapter, it will be misconfigured for psr-18 as by default it does throw exceptions...

we instanciate the client like this:

guzzle6-adapter/src/Client.php

Line 68 in c4e1a4f

private static function buildClient(array $config = []): GuzzleClient

is that not the correct way to have it not throw exceptions?

[sagikazarmark]

I think it is. There is nothing we can do about misconfigured clients, they will break the interface.

@mekras can you show us some code to reproduce the issue?

Using this in the example is fine: https://httpbin.org/#/Status_codes/get_status__codes_

[mekras]

@sagikazarmark, https://gist.github.com/mekras/70d591ae38a14d6cd036bef3498eb514

[dbu]

the problematic line is https://gist.github.com/mekras/70d591ae38a14d6cd036bef3498eb514#file-test-php-L23 - you instantiate the guzzle client with default guzzle behaviour, which is to throw exceptions. you can omit the guzzle client, then the adapter will instantiate guzzle in a spec compatible way:

guzzle6-adapter/src/Client.php

Lines 68 to 75 in c4e1a4f

	private static function buildClient(array $config = []): GuzzleClient
	{
	$handlerStack = new HandlerStack(\GuzzleHttp\choose_handler());
	$handlerStack->push(Middleware::prepareBody(), 'prepare_body');
	$config = array_merge(['handler' => $handlerStack], $config);
	return new GuzzleClient($config);
	}

[mekras]

Huh. Yes, this works. That code was written for guzzle6-adapter 1.x where Guzzle client was required. Thanks.

[dbu]

the client argument was not required in any released version of the adapter, but it was and is allowed. i wonder if we should add some phpdoc to the adapter class to be more explicit about this? we don't say anything in the phpdoc, i think the documentation website somewhere mentions but its very non-obvious for the user.

[dbu]

i created #69 to better document this

[gmponos]

If I remember correctly I had made a test with default behavior of guzzle and the adapter was working without throwing exceptions..

If my memory is not good and I remember wrong and what @mekras is saying is true then IMHO I find it problematic that I have to configure a Guzzle client properly and I have to know it from before...

This kinda loses the meaning of an adapter...