Abstracting HTTP requests to keep it simple, flexible

Question

The project is an API that will itself make HTTP requests to external webservices.

Since this is PHP, we actually chose to use guzzlehttp. Now the library itself doesn't matter much, but I'm scared we'll be stuck with it if it ever has a major change (it already happened in the past).

So I thought I'd create an interface that would be common to be able to swap the implementation if needed. Here it is (I removed the Use statements for clarity) :

<?php

namespace App\Core\Request;

interface HttpRequest
{
    public function get();
    public function post();
    public function setUri($uri);
    public function setDatas(array $datas);
    public function setHeaders(array $headers);
}

I thought this would be usable by cURL if we need it.

Then I made the Guzzle implementation, here it is :

<?php

namespace App\Core\Request;

class GuzzleHttpRequest implements HttpRequest
{
    protected $client;
    protected $logger;
    protected $datas   = [];
    protected $headers = [];
    protected $uri;
    protected $requestLog;
    protected $currentRequestLog;
    protected $serviceId;

    const DEFAULT_ERROR_CODE = 500;

    public function __construct(Client $client, GuzzleLogger $logger, RequestsLog $requestLog)
    {
        $this->client     = $client;
        $this->logger     = $logger;
        $this->requestLog = $requestLog;
        $this->serviceId  = Config::get('app.locale');
    }

    public function setDatas(array $datas)
    {
        $this->datas = $datas;
    }

    public function setHeaders(array $headers)
    {
        $this->headers = $headers;
    }

    public function get()
    {
        return $this->request('GET', 'query');
    }

    public function post()
    {
        return $this->request('POST', 'json');
    }

    protected function request($method, $dataType)
    {
        try {
            $response = $this->sendRequest($method, $dataType);

            return $this->handleResponse($response);
        }
        catch (RequestException $e) {
            return $this->handleRequestException($e);
        }
        catch (Exception $e) {
            return $this->handleException($e);
        }
    }

    protected function sendRequest($method, $dataType)
    {
        $this->checkUri();

        $this->currentRequestLog = $this->requestLog->fromConnection($this->serviceId, $this->getUri());

        return $this->client->request($method, $this->getUri(), $this->getOptions($dataType));
    }

    protected function checkUri()
    {
        if (is_null($this->getUri())) {
            throw new UriNotSetException('Uri should be set.', self::DEFAULT_ERROR_CODE);
        }
    }

    protected function getUri()
    {
        return $this->uri;
    }

    public function setUri($uri)
    {
        $this->uri = $uri;
    }

    protected function getOptions($dataType)
    {
        return [$dataType => $this->datas, 'headers' => $this->headers, 'handler' => $this->getHandler('generic')];
    }

    protected function getHandler($logName)
    {
        $uniqId = uniqid('REQUEST');

        return $this->logger->createLoggingHandlerStack([
            "REQUEST_LOG_ID_{$this->currentRequestLog->id} REQUEST : \r\n {request}",
            "REQUEST_LOG_ID_{$this->currentRequestLog->id} RESPONSE : {response}",
        ], $logName);
    }

    protected function handleResponse(GuzzleResponse $response)
    {
        $this->currentRequestLog->succeed();

        return ResponseJsonSuccess::getResponse($this->getFormattedResponse($response));
    }

    protected function getFormattedResponse(GuzzleResponse $response)
    {
        return (string)$response->getBody();
    }

    protected function handleRequestException(RequestException $e)
    {
        $this->currentRequestLog->fail();

        $code    = $e->getCode() ?: self::DEFAULT_ERROR_CODE;
        $message = (string)$e->getMessage();

        return $this->handleException(new Exception($message, $code));
    }

    protected function handleException(Exception $e)
    {
        return ResponseJsonError::getResponse($e);
    }
}

This class could be problematic to me as it's doing lots of things. It's able to handle requests and responses (and exceptions bound to these), and it also logs all of its requests.

I did this class to be able to implement subclasses to change the behavior when needed, but I still wanted to be able to use this main class for most requests.

Here's an example of a subclass I made :

<?php

namespace App\Services\Pivotal\Request;

class PivotalGuzzleHttpRequest extends GuzzleHttpRequest
{
    private $tokenFactory;

    public function __construct(Client $client, GuzzleLogger $logger, RequestsLog $requestLog, TokenFactory $tokenFactory)
    {
        parent::__construct($client, $logger, $requestLog);
        $this->tokenFactory = $tokenFactory;
    }

    protected function getFormattedResponse(Response $response)
    {
        $this->currentRequestLog->succeed();

        return json_decode((string)$response->getBody(), true);
    }

    protected function getOptions($dataType)
    {
        $options            = parent::getOptions($dataType);
        $options['headers'] = [
            "Content-Type"  => "application/json",
            "Authorization" => "Bearer {$this->tokenFactory->getTokenId()}"
        ];
        $options['handler'] = $this->getHandler('pivotal');

        return $options;
    }
}

How can I improve this code ? All I'm looking for is the code to be easy to use, easy to test, and easy to understand.

Answer 1

Generally, I think this code is well thought out and well structured.

• Good use of dependency injection, especially with preference for passing type-hintable objects as parameters instead of primitives.
• Methods seem to have good scope, with specific purposes.

Some general thoughts

• Consider adding Doc Blocks. This code is well-written, but you could take it to the next level in terms of professionalism by adding these, IMO.
• A number of lines appear to be flirting with ideal 80 character per line limit. I think there are a number of cases where the code might read better if you broke some of your longer lines up.
• Though I think you generally do a good job of writing code that is self-explanatory, there are perhaps some points where minimal comments could add value.
• I agree with sentiment on this class doing too much, a lot of it not specific to usage of the Guzzle client. I actually wonder whether you might consider an abstract class in place of (or perhaps on conjunction with) the use of the interface. This might allow you to move common exception handling, logging, property getting/setting, etc. out of the Guzzle-specific implementation, while still forcing implementation of any abstract methods within inheriting classes.
• I am not sure about some of your reasoning for then extending Guzzle class class to the Pivotal class. Is this composable enough? Do you think there would be a PivotSomeOtherClientHttpRequest at some point? In other words, is some of the functionality to Pivotal something that can be encapsulated into it's own class which is passed to (or gets passed) an HttpRequest object to perform these operations? Without other examples of how the Guzzle class would potentially be extended, it is hard to make more substantive suggestions.
• Can data that is going to be set to $this->datas only be an array? This seems restrictive.
• It is not clear to me how you set a query string for GET request. Do you have to rely on passing this in URI?
• Consider grouping your getters/setters together rather than splitting them in class with other methods in between.
• setUri() should have some validation attached to it (i.e. string is passed at minimum, perhaps non-zero-length string if you don't expect the value to be empty string).
• Consider validation on other setters if, for example, empty arrays are not expected (you are already type-hinting for array type).

Code-specific notes

$this->serviceId  = Config::get('app.locale');

Is Config::get() guaranteed to have a value at this key? Do you need to do any validation that this configuration value is set properly before completing the constructor (vs. throwing exception)? Also variable name and config name do not seem to match. Is the variable being set here a service ID or a locale setting?

---

public function post()
{
    return $this->request('POST', 'json');
}

Should this method perhaps be called postJson?

---

protected function getOptions($dataType)
{
    return [$dataType => $this->datas, 'headers' => $this->headers, 'handler' => $this->getHandler('generic')];
}

Not sure I like method name here. getRequestOptions() perhaps? You seem to end up with weird key/value pair here: $dataType => $this->datas. Maybe this is based on underlying Guzzle client but seems like it would make more sense to have two key-value pairs here like 'dataType' => $dataType, 'data' => $this->datas.

This is also a good example of a line that could be broken up like:

    return [
        $dataType => $this->datas,
        'headers' => $this->headers,
        'handler' => $this->getHandler('generic')
    ];