Add BatchResult

[sagikazarmark]

Fixes #47

[dbu]

otherwise, we should look in the exceptions to see if there is an exception carrying a response and return that. if that is not found, i would throw the ecxeption we find at $request. or an invalid argument exception if the request does not exist.

[sagikazarmark]

Currently responses are handled the following way: every response and exception is collected in the BatchResult. If there are any exceptions, The BatchException gets the result and it is thrown.

There is a getExceptionFor method to get the exception. If that exception is HttpException, you can get a response. I would not mix the two methods.

I also wouldn't throw the exception, because that way you cannot process the rest.

The reason why I put exceptions here as well is that an exception cannot be immutable. But it would possibly be better, to collect exceptions somewhere else (eg. right in the batch exception?)

[sagikazarmark]

I have an ugly idea for that. 😜

In batch exception accept two objects: batch result, exception chain

The exception chain would be an object which accepts a request, an exception and the previous/next exception. THis way you have one object in the end to be injected into the batch exception. You can always get the next exception by calling getNextException, which returns the next in the chain.

[dbu]

hm. when i think of responses, i might want to process all responses,
even unsuccessful ones. but it indeed could get very confusing. so lets
adjust phpdoc to always say "successful responses" and throw an
exception if i try to request a response that does not exist (maybe with
a hint that the request failed, telling the user he should check with
the isSuccessful / isFailed method.

[hannesvdvreken]

throw the exception we find at $request

I dunno about throwing it...

[sagikazarmark]

This is what I tried first. The problem is that an Exception cannot be cloned, so making it immutable that way doesn't work.

[dbu]

hm. i would prefer the clean and logic code structure over insisting on the immutability. if people want to do weird stuff, they always find ways to do it. we can do an `@internal` annotation on the add method and note you should not use this outside a client/adapter.

[sagikazarmark]

Should we do this in case of the BatchResult as well? I think it would be weird if the Result is immutable, the exception is not.

[dbu]

i would keep it immutable. its just a technical detail that the exception is not forced immutable. it is to be considered immutable by the user

[sagikazarmark]

Done

[hannesvdvreken]

Returns a BatchResult object which contains succesful responses and exceptions for failed requests.

[dbu]

s/and exceptions //

[joelwurtz]

Maybe providing a Trait for the method sendRequests on the HttpPsrClient (using the sendRequest method) would be useful for client/adapter not handling this use case ?

trait BatchRequest {
    public function sendRequests($requests)
    {
         foreach ($requests as $request) {
             ...
         }
    }
}

[sagikazarmark]

@joelwurtz Might be useful in cases where the underlying client does not support parallel requests. Internally it could rely on sendRequest method of the interface. Would you like to open a PR against the terminology branch?

[dbu]

the idea makes sense. i just wonder how much implementation this repository should provide. maybe we could have an adapter-base (or something like that) repository that provides a trait and other utility things we come up with. the trait not on the same level as the spec itself.

[sagikazarmark]

Yeah, that's a good point as well.

I would say, until we don't have enough utilities, collect them at one place. Before release, we can separate components which are not part of the spec. The utility namespace could even be the same or we can add the Util word to the namespace. WDYT?

[joelwurtz]

No problem for making a PR @sagikazarmark will wait to have this merged before doing it (so no changes need to be made with the new BatchResult object)

[sagikazarmark]

Actually, I think this is ready to merge. @dbu ?

[dbu]

agreed. and lets have a Util namespace or something right away

[sagikazarmark]

@joelwurtz PR merged. If you provide a PR, please do that under the Util namespace.