Clear exception usage in Plugins

[sagikazarmark]

Q	A
Bug?	yes
New Feature?	no
Version	1.1-

Actual Behavior

Plugin implementations only accept Http\Client\Exception instances in their rejection handler (here and here) while there is no such limitation in the promise spec or the RejectedPromise.

Expected Behavior

Plugins should accept any kind of exceptions and handle only which are acceptable for them and rethrow the rest. This is not a breaking change since we just need to widen the accepted exception types in the plugins.

Steps to Reproduce

See #33

Possible Solutions

As said, widen the range of accepted exception types.

[GrahamCampbell]

Yeh, I'm not sure what we should do here. For now, in this other package, this has been fixed by implementing your interface.

[sagikazarmark]

Yes, but are those exceptions actually related to the HTTP flow or they are rather Domain exceptions? I suspect it's the latter and in that case just implementing that interface is not a real solution from our library point of view (nor from Github API point of view, but that's another question).

[GrahamCampbell]

All these exceptions are transformations of the http exceptions provided by github.

[sagikazarmark]

In our case I think that means domain exceptions (or both). Example:

HttpNotFound: HTTP exception
OrganizationNotFound: Domain Exception

[GrahamCampbell]

Ah, right. I see what you mean. Yes, I agree we probably need to change this, since we want to think more abstractly about the domain.

[sagikazarmark]

If you mean abstraction in terms of the transport layer (HTTP), then yes, exactly.

[GrahamCampbell]

Yeh. We need to allow non-http exceptions in here I think.

[joelwurtz]

Mmh not sure about that, we got a long discussion about that in : php-http/httplug#87

And then we change this : php-http/httplug@dfbf75b

So for me, yes our promise is not bound to any particular exception, but a promise in the context of httplug only pass exception of the base interface to the next promise (others may (must ?) be directly throw, and not call the next callable in the chain promise)

@GrahamCampbell what was the adapter used ? Guzzle6 ?

[joelwurtz]

Okay, the bug comes from we are directly using "RejectedPromise" but we should have a HttpRejectedPromise (and HttpFulfilledPromise) in httplug IMO which respect this behavior / signature, and use this promise instead of the global one

WDYT ?

[sagikazarmark]

Does that mean that a non-http exception should immediately stop the execution?

IMO we don't need such promises (for the moment) or at least we are not forced to do that, we can simply change the logic which creates the rejected promise to throw any non-http exceptions, however, for the long-term we probably should to ensure type safety.

[joelwurtz]

Does that mean that a non-http exception should immediately stop the execution?

Yes, for me that what we should do

we can simply change the logic which creates the rejected promise

This logic is inside the promise (a promise create another promise), so there is no way to do that a part in the RejectedPromise class, but since this one is generic and not bound to Httplug it does not make sense, that's why i want to add a HttpRejectedPromise in httplug library.

Also having a HttpFulfilledPromise in httplug would allow us to enforce that we got a PSR7 Response everywhere.

However i may be too strict about that, and we can simply allow everything ? In this case, i'm fine but we must be consistent : if we allow everything as an Exception, we should allow everything as a Resolution, having only a part in the contract is the worst solution.

[joelwurtz]

I make an exemple in php-http/httplug#116 for a better vision of what i think we should do.