Fix for issue 8 #10

Nyholm · 2015-10-30T08:33:21Z

This is an overview over the exceptions in Guzzle6. All exception implement GuzzleException.

SeekException (Network )
TransferException (Network)
- RequestException
  - TooManyRedirectsException (Network)
  - ConnectException (Network)
  - BadResponseException
  -  - ClientException
  -  - ServerException

sagikazarmark · 2015-10-30T08:36:06Z

It doesn't fix the problem, NetworkException is thrown anyway if the request does not have a response.

Nyholm · 2015-10-30T08:43:25Z

Sorry, I thought a RequestException always had a Response unless it was a TooManyRedirectsException or ConnectException.

Am I correct when highlighting the network errors?

sagikazarmark · 2015-10-30T08:47:46Z

Not sure, haven't checked yet. Ideally Network exception should be thrown when it is actually related to networking, HttpException should be thrown if we have a response, otherwise, if it is a Guzzle RequestException, our RequestException should be thrown. Not sure, if the order of conditions is right though.

Nyholm · 2015-10-30T09:23:01Z

Digging in Guzzle's source there are situations where a instance of GuzzleRequestException is created with no Response (GuzzleHttp\Handler\StreamHandler::__invoke).

So I've updated the PR. It first checks if it is a GuzzleRequestException but not network related (ie Guzzle's TooManyRedirectsException or ConnectException). If it is a GuzzleRequestException and has a response we throw a HttpException. If we for some reason does not have a response we throw a RequestException.

If the request was related to Network (ie Guzzle's SeekException, TransferException, TooManyRedirectsException or ConnectException) we will throw a NetworkException.

sagikazarmark · 2015-10-30T09:39:11Z

Exception thrown when a seek fails on a stream.

Seek is not network related:

https://github.com/guzzle/guzzle/blob/master/src/Exception/SeekException.php#L7

TransferException should be converted to our own TransferException, but make sure that it is the last as it is a base exception for all others.

I wonder if TooMany redirects is Network exception. It is a configurable thing how many redirects are supposed to be treated invalid, not sure if it is enabled by default. @php-http/owners WDYT?

Please also add some tests to make sure that every scenario is taken care of.

Nyholm · 2015-10-30T10:16:58Z

Sure, I've added some test.

I don't catch SeekException and TransferException because they do not have a Request object. I can not convert them to any of our exceptions.

SeekException is never created the Guzzle lib and TransferException is always instantiated by its children.

sagikazarmark · 2015-10-30T10:27:24Z

We have our own TransferException which does not have a Request object. We still can convert a SeekException to Request exception as we have the Request, even if the exception doesn't, can't we?

Nyholm · 2015-10-30T10:29:42Z

Our TransferException is an abstract class so I can't use it at the moment. Should I change it to a normal class?

We still can convert a SeekException to Request exception as we have the Request, even if the exception doesn't, can't we?

You are correct.. Will do.

sagikazarmark · 2015-10-30T10:31:50Z

Our TransferException is an abstract class so I can't use it at the moment. Should I change it to a normal class?

@dbu AFAIK you changed it to abstract. This is a case when we could use it. Should we change it back or add a separate exception for cases like this?

This would probably only make sense if we can expect TransferException thrown from Guzzle at all.

Nyholm · 2015-10-30T10:47:57Z

This would probably only make sense if we can expect TransferException thrown from Guzzle at all.

We do not expect TransferException. Guzzle only extend it with the RequestException.

class RequestException extends TransferException {}

dbu · 2015-10-30T11:24:08Z

if there is a use case that makes sense and does not warrant a more specific exception, sure lets make it concrete.

Nyholm · 2015-10-30T13:04:18Z

I do not believe we ever going to catch a TransferException if we already are catching RequestExceptions.

I've updated the PR.
A) Seek and Transfer exceptions are converted to RequestException
B) Connect and TooManyRedirects exceptions are converted to NetworkException
C) If any other exceptions (extending Guzzle's RequestException) as a response we convert them to HttpException
D) If the response is missing, we convert it to a RequestException

sagikazarmark · 2015-10-31T17:58:41Z

Seek and Transfer exceptions are converted to RequestException

Transfer exceptions does not necessarily have a Request context, while SeekException definitely have. Transfer exceptions should be converted into our TransferException (which is a concrete class again from now)

I think TooManyRedirects should also be converted to an HttpException (or not converted at all). I've already expressed why it can't simply be a Network issue.

Also, I find the processing logic a bit weird, I would follow something like this:

We can make the assumption that Guzzle always throws a GuzzleException (can we @mtdowling)
If it is ConnectException then convert to NetworkException.
If it is a RequestException AND has a response then convert to HttpException, otherwise convert to Request exception.
If none of the above, convert to TransferException.

Seek exception is the only hard thing I think, but as I can see you already solved it without a convert method usage.

👍 for the separate catch statements and separate conversion methods, I like that solution.

dbu · 2015-11-01T21:27:01Z

will TooManyRedirect show the first or the last response? oh, and is there any place where we control limit of redirections? or is that client specific and for adapters in the injected client?

sagikazarmark · 2015-11-01T21:28:53Z

I think TooManyRedirections should be handled in a plugin. Not sure if TooManyRedirections is thrown by default without specific configuration, I hope not.

dbu · 2015-11-01T21:46:48Z

well, if the client does follow redirects, a plugin will never see them. if not, a plugin is the right way. either way, following redirects without limit is a reciepie for desaster, i would always want some limit.

Nyholm · 2015-11-03T20:42:28Z

will TooManyRedirect show the first or the last response?

There is an issue that we have not discussed here. TooManyRedirectsException does not have a response at all. I do not convert TooManyRedirectsException at all ATM. I can not convert it to a HttpException since it requires a response.

dbu · 2015-11-04T06:58:53Z

not sure how much we care about the response in that case. if guzzle has non, use a general network exception. we need to have an idea who follows redirects: the guzzle client or a plugin by php-http? guess it depends how you config guzzle. perf wise guzzle is probably less overhead, but a php-http plugin will be more consistent across implementations. we cant do more than recommend one or the other way

sagikazarmark · 2015-11-04T08:15:36Z

src/Guzzle6HttpAdapter.php

-            throw $this->createException($e);
+        } catch (Guzzle\SeekException $e) {
+            throw new RequestException($e->getMessage(), $request, $e);
+        } catch (Guzzle\TransferException $e) {


We should catch all other GuzzleExceptions here.

Guzzle got 2 types of Exceptions. SeekException and TransferException. Both are implementing GuzzleException. Logically the same but sure, I'll make the change.

In case they introduce new ones. Also, I am not sure that plugins don't introduce do that.

sagikazarmark · 2015-11-04T08:21:05Z

@dbu I asked @Nyholm not to convert TooManyRedirects to a Network exception. I believe it is not really a network problem. If something, then a RequestException (if we cannot assume that we have a response, which is weird, because a Response is necessary in order to detect the fact of redirection. But guzzle might not inject the response into the request.

I think we should have this functionality in a plugin. Probably we should mention in the plugin docs, that it is recommended NOT to use any further redirection detection in the underlying client.

dbu · 2015-11-04T08:36:09Z

@sagikazarmark agreed: recommend to use plugin. if not plugin is used, catch the guzzle exception to wrap it into one of ours. (maybe in the generic catch-all thing rather than specifically - there are probably more cases of guzzle throwing some exceptions we do not explicitly want to handle)

sagikazarmark · 2015-11-04T08:38:38Z

there are probably more cases of guzzle throwing some exceptions we do not explicitly want to handle

Why? I think we want to catch all exceptions.

Maybe in case of adapters we could introduce an UnexpectedAdapterException to indicate that this couldn't be handled. But rather not, as we would have to introduce an adapter-utils package and it seems like an overkill for just one exception.

dbu · 2015-11-04T08:51:36Z

oh yes, we want to catch all exceptions and wrap them. with "explicitly" i mean that we won't handle every single case of guzzle exception specifically, but just convert it into our basic exception.

regarding UnexpectedAdapterException i would not do that, using the basic RequestException and having a message that explains the adapter threw some exception we did not specifically handle should be enough.

sagikazarmark · 2015-11-04T08:55:58Z

Agree. By removing the check for TooManyRedirect should end up in a RequestException.

joelwurtz · 2015-11-04T10:17:44Z

Previous version of this adapter also set the guzzle client to use response instead of exception (like for 500 / 404 / ....) shouldn't we reintroduce that (so we can keep same behavior across adapters) ?

It was delete by the removal of the options parameter in the adapter.

sagikazarmark · 2015-11-04T10:19:44Z

Good question. What is the default behavior? Throwing exceptions or not? If not, then I am not sure we should do that. We should just remind the user that it is our task. If throwing exceptions is the default behavior then we should consider it.

Nyholm · 2015-11-04T10:53:58Z

By removing the check for TooManyRedirect should end up in a RequestException.

That is correct

What is the default behavior? Throwing exceptions or not?

The default behavior is to throw exceptions. Ref. about exceptions and parameters.

sagikazarmark · 2015-11-04T11:04:16Z

src/Guzzle6HttpAdapter.php

        }
    }

    /**
     * Converts a Guzzle exception into an Httplug exception.
     *
-     * @param RequestException $exception
+     * @param GuzzleExceptions\TransferException $exception


This is incorrect

Sorry, updating.

sagikazarmark · 2015-11-04T11:08:15Z

I added a few more comments. After that, it should be fine. Can you also please squash commits?

sagikazarmark · 2015-11-04T11:09:36Z

Also, do you know why tests are failing?

sagikazarmark · 2015-11-04T11:10:12Z

@dbu Have you updated the MessageFactory usage in integration tests?

Nyholm · 2015-11-04T11:18:12Z

The tests are failing because I try to instantiate an abstract TransferException.
Composer does (apparently) install the alpha version of Httplug instead of 1.0.x-dev.

sagikazarmark · 2015-11-04T11:21:08Z

As I can see, the tests are also failing because of php-http/adapter-integration-tests#12

Nyholm · 2015-11-04T11:32:24Z

Is that a bug in composer? The min-stability is dev. Shouldn't it always prefer 1.0.x-dev before 1.0.0-alpha

sagikazarmark · 2015-11-04T11:36:25Z

Change the constraint to dev-master for now.

Nyholm · 2015-11-04T11:48:29Z

There you go.

sagikazarmark · 2015-11-04T12:06:17Z

Awesome, thanks @Nyholm

Improve exception conversion, closes #8

dbu · 2015-11-04T12:08:36Z

Is that a bug in composer? The min-stability is dev. Shouldn't it always prefer 1.0.x-dev before 1.0.0-alpha

by default, it does prefer-stable: true, which means if there is a valid version, it takes that over 1.0.x-dev. you could limit to > alpha1 or set prefer-stable: false.

Nyholm · 2015-11-04T12:10:42Z

Thank you for the feedback and for merging.

Is that a bug in composer? The min-stability is dev. Shouldn't it always prefer 1.0.x-dev before 1.0.0-alpha

by default, it does prefer-stable: true, which means if there is a valid version, it takes that over 1.0.x-dev. you could limit to > alpha1 or set prefer-stable: false.

Okey, thank you.

Nyholm force-pushed the issue8 branch from 448eb43 to acbfce1 Compare October 30, 2015 12:59

sagikazarmark reviewed Nov 4, 2015
View reviewed changes

Nyholm force-pushed the issue8 branch from 4c53915 to ca329b2 Compare November 4, 2015 11:08

Nyholm force-pushed the issue8 branch from f5b8bc4 to ba12173 Compare November 4, 2015 11:18

Fix for issue php-http#8

eea819f

Nyholm force-pushed the issue8 branch from ac48056 to eea819f Compare November 4, 2015 11:47

sagikazarmark added a commit that referenced this pull request Nov 4, 2015

Merge pull request #10 from Nyholm/issue8

c887235

Improve exception conversion, closes #8

sagikazarmark merged commit c887235 into php-http:master Nov 4, 2015

Fix for issue 8 #10

Fix for issue 8 #10

Uh oh!

Conversation

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

dbu commented Oct 30, 2015

Uh oh!

Nyholm commented Oct 30, 2015

Uh oh!

sagikazarmark commented Oct 31, 2015

Uh oh!

dbu commented Nov 1, 2015 via email

Uh oh!

sagikazarmark commented Nov 1, 2015

Uh oh!

dbu commented Nov 1, 2015 via email

Uh oh!

Nyholm commented Nov 3, 2015

Uh oh!

dbu commented Nov 4, 2015 via email

Uh oh!

sagikazarmark Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

Nyholm Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

sagikazarmark Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

dbu commented Nov 4, 2015

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

dbu commented Nov 4, 2015

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

joelwurtz commented Nov 4, 2015

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

Nyholm commented Nov 4, 2015

Uh oh!

sagikazarmark Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

Nyholm Nov 4, 2015

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

sagikazarmark commented Nov 4, 2015

Uh oh!

Nyholm commented Nov 4, 2015