Add sendRequests method as a trait

[joelwurtz]

See discussion in #48

[joelwurtz]

Method should be good, but i'm having a hard time to write specification for the Trait (never done phpspec before) and it's seems quite hards since i use the sendRequest method internally, do you have an idea for the spec @sagikazarmark ?

[hannesvdvreken]

I dunno about this...

[sagikazarmark]

I usually do this as well. This makes sure your code works, even if you don't implement the relevant interface.

[hannesvdvreken]

Did some reading. I'm cool with this, was just worried about keeping this in sync with the interface definition.

[sagikazarmark]

This problem can be handled on a test level: if the stub implements HttpPsrClient then the tests will fail if the signature is incorrect.

[sagikazarmark]

Nice work @joelwurtz! 👍 👍

Unfortunately it is hard to test internal methods with phpspec. I usually use a hack with the stub: inject the response/exception in the stub's constructor. You can then return it in the sendRequest method.

Alternatively you can inject a HttpPsrClient mock and call its sendRequest in the stub. This way you have greater control over what's happening. Then in your tests you can add calls to the HttpPsrClient mock.

[sagikazarmark]

Since this is the first case we use the batch result/exception solution, we don't have any reference. But I wonder if we should create both the result and the exception at the beginning. Shouldn't we create the excetpion on demand?

[joelwurtz]

We can create the exception on demand but i think it will make code harder to read, as we must add some conditions / loop system as there is no way to pass an array of request -> exceptions to the BatchException in its current implementation

[sagikazarmark]

Yeah, it is on purpose that you have to add them one by one, because in most cases (and in this as well) processing of results is linear. This way you don't have to mess with array indicies. Alternatively we could have a method in the BatchException which allows an array input with array elements of a Request and an Exception. But that would be more complicated IMO.

[joelwurtz]

So, should i keep the current implementation or do you prefer something like this :

        $batchResult    = new BatchResult();
        $batchException = null;

        foreach ($requests as $request) {
            try {
                $batchResult->addResponse($request, $this->sendRequest($request, $options));
            } catch (Exception $e) {
                if (null === $batchException) {
                    $batchException = new BatchException();
                    $batchException->setResult($batchResult);
                }

                $batchException->addException($request, $e);
            }
        }

        if (null !== $batchException) {
            throw $batchException;
        }

        return $batchResult;

or something like this :

        $batchResult     = new BatchResult();
        $batchExceptions = new \SplObjectStorage();

        foreach ($requests as $request) {
            try {
                $batchResult->addResponse($request, $this->sendRequest($request, $options));
            } catch (Exception $e) {
                $batchExceptions[$request] = $e;
            }
        }

        if (count($batchExceptions) > 0) {
            $batchException = new BatchException();
            $batchException->setResult($batchResult);

            foreach ($batchExceptions as $request => $exception) {
                $batchException->addException($request, $exception);
            }

            throw $batchException;
        }

        return $batchResult;

[sagikazarmark]

Well, both versions are harder to read. Probably the first one is cleaner. Let's leave it as is for now, from a public API point of view it doesn't matter, we can change this later.

[joelwurtz]

Yeah but the problem is that i need a implementation for the ResponseInterface (i have try to use prophecy but it only mock method and the interface is not used so it always fail in the sendRequests method)

[sagikazarmark]

i have try to use prophecy but it only mock method and the interface is not used so it always fail in the sendRequests method

I am not sure I understand this.

You can do something like this:

class BatchRequestSpec extends ObjectBehavior
{
    function let()
    {
        $this->beAnInstanceOf('spec\Http\Client\Util\BatchRequestStub');
    }

    function let(HttpPsrClient $client)
    {
        $this->beAnInstanceOf('spec\Http\Client\Util\BatchRequestStub', [$client]);
    }

    function it_does_something(HttpPsrClient $client, RequestInterface $request1, RequestInterface $request2, ResponseInterface $response, Exception $e)
    {
        $client->sendRequest($request1)->willReturn($response);
        $client->sendRequest($request2)->willThrow($e);

        $this->sendRequests([$request1, $request2]);

    }
}


class BatchRequestStub
{
    use BatchRequest;

    protected $client;

    public function __construct(HttpPsrClient $client)
    {
        $this->client = $client;
    }

    /**
     * {@inheritdoc}
     */
    public function sendRequest(RequestInterface $request, array $options = [])
    {
        return $this->client->sendRequest($request, $options);
    }
}

At least I think it should work.

[joelwurtz]

Yes it works, will update the PR

[joelwurtz]

Should be good with last comments

[sagikazarmark]

Can you squash the commits?

[joelwurtz]

done

[sagikazarmark]

Thank you @joelwurtz