3
\$\begingroup\$

I have recently refactored a REST client library and was hoping to get review for both the class itself and the unit tests that cover it.

This code is developed against PHP 7.1, but tested against PHP 5.6, 7.0 7.1, HipHop VM, and PHP nightly builds.

The full library can be seen on GitHub

RestClient.php

<?php

namespace MikeBrant\RestClientLib;

/**
 * @desc Class for executing RESTful service calls using a fluent interface.
 */
class RestClient
{
    /**
      * Flag to determine if basic authentication is to be used.
     * 
     * @var boolean
     */
    protected $useBasicAuth = false;

    /**
     * User Name for HTTP Basic Auth
     * 
     * @var string
     */
    protected $basicAuthUsername = null;

    /**
     * Password for HTTP Basic Auth
     *
     * @var string
     */
    protected $basicAuthPassword = null;

    /**
     * Flag to determine if SSL is used
     * 
     * @var boolean
     */
    protected $useSsl = false;

    /**
     * Flag to determine is we are to run in test mode where host's SSL cert is not verified
     * 
     * @var boolean
     */
    protected $useSslTestMode = false;

    /**
     * Integer value representing number of seconds to set for cURL timeout option. Defaults to 30 seconds.
     * 
     * @var integer
     */
    protected $timeout = 30;

    /**
     * Variable to store remote host name
     * 
     * @var string
     */
    protected $remoteHost = null;

    /**
     * Variable to hold setting to determine if redirects are followed
     * 
     * @var boolean
     */
    protected $followRedirects = false;

    /**
     * Variable to hold value for maximum number of redirects to follow for cases when redirect are being followed.
     * Default value of 0 will allow for following of unlimited redirects.
     * 
     * @var integer
     */
    protected $maxRedirects = 0;

    /**
     * Variable which can hold a URI base for all actions
     * 
     * @var string
     */
    protected $uriBase = '/';

    /**
     * Stores curl handle
     *
     * @var mixed
     */
    private $curl = null;

    /**
     * Variable to store request URL that is formed before a request is made
     * 
     * @var string
     */
    private $requestUrl = null;

    /**
     * Array containing headers to be used for request
     * 
     * @var array
     */
    private $headers = array();

    /**
     * Variable to store the request header as sent
     * 
     * @var string
     */
    private $requestHeader = null;

    /**
     * Variable to store the request data sent for POST/PUT requests. THis could be array, string, etc.
     * 
     * @var mixed
     */
    private $requestData = null;

    /**
     * Variable to store response code
     * 
     * @var integer
     */
    private $responseCode = null;

    /**
     * Variable to store cURL response info array
     * 
     * @var array
     */
    private $responseInfo = null;

    /**
     * Variable to store cURL reponse body
     * 
     * @var string
     */
    private $responseBody = null;

    /**
     * Constructor method. Currently no implementation.
     *
     * @return void
     */
    public function __construct() {}

    /**
     * Method to execute GET on server
     * 
     * @param string $action
     * @return RestClient
     * @throws \InvalidArgumentException
     * @throws \Exception
     */
    public function get($action = null) {
        $this->validateAction($action);
        $this->curlSetup();
        $this->setRequestUrl($action);
        curl_setopt($this->curl, CURLOPT_HTTPGET, true);
        // execute call. Can throw \Exception.
        $this->curlExec();

        return $this;
    }

    /**
     * Method to exexute POST on server
     * 
     * @param mixed $action
     * @param mixed $data
     * @return RestClient
     * @throws \InvalidArgumentException
     * @throws \Exception
     */
    public function post($action = null, $data = null) {
        $this->validateAction($action);
        $this->validateData($data);
        $this->curlSetup();
        $this->setRequestUrl($action);
        $this->setRequestData($data);
        curl_setopt($this->curl, CURLOPT_POST, true);
        // execute call. Can throw \Exception.
        $this->curlExec();

        return $this;
    }

    /**
     * Method to execute PUT on server
     * 
     * @param string $action
     * @param mixed $data
     * @return RestClient
     * @throws \InvalidArgumentException
     * @throws \Exception
     */
    public function put($action = null, $data = null) {
        $this->validateAction($action);
        $this->validateData($data);
        $this->curlSetup();
        $this->setRequestUrl($action);
        $this->setRequestData($data);
        curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, 'PUT');
        // execute call. Can throw \Exception.
        $this->curlExec();

        return $this;
    }

    /**
     * Method to execute DELETE on server
     * 
     * @param string $action
     * @return RestClient
     * @throws \InvalidArgumentException
     * @throws \Exception
     */
    public function delete($action = null) {
        $this->validateAction($action);
        $this->curlSetup();
        $this->setRequestUrl($action);
        curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, 'DELETE');
        // execute call. Can throw \Exception.
        $this->curlExec();

        return $this;
    }

    /**
     * Method to execute HEAD on server
     * 
     * @param string $action
     * @return RestClient
     * @throws \InvalidArgumentException
     * @throws \Exception
     */
    public function head($action = null) {
        $this->validateAction($action);
        $this->curlSetup();
        $this->setRequestUrl($action);
        curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, 'HEAD');
        curl_setopt($this->curl, CURLOPT_NOBODY, true);
        // execute call. Can throw \Exception.
        $this->curlExec();

        return $this;
    }

    /**
     * Sets host name of remote server
     * 
     * @param string $host
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setRemoteHost($host = null) {
        if(empty($host)) {
            throw new \InvalidArgumentException('Host name not provided.');
        } else if(!is_string($host)) {
            throw new \InvalidArgumentException('Non-string host name provided.');
        }

        // remove any http(s):// at beginning of host name
        $httpsPattern = '#https://#i';
        $httpPattern = '#http://#i';
        if (1 === preg_match($httpsPattern, $host)) {
            // this needs to be SSL request
            $this->setUseSsl(true);
            $host = str_ireplace('https://', '', $host);
        } else if (1 === preg_match($httpPattern, $host)) {
            $host = str_ireplace('http://', '', $host);
        }

        // remove trailing slash in host name
        $host = rtrim($host, '/');

        // look for common SSL port values in host name to see if SSL is needed
        $portPatterns = array(
            '/:443$/',
            '/:8443$/',
        );
        foreach ($portPatterns as $pattern) {
            if (1 === preg_match($pattern, $host)) {
                $this->setUseSsl(true);
            }
        }

        $this->remoteHost = $host;

        return $this;
    }

    /**
     * Sets URI base for the instance
     * 
     * @param string $uriBase
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setUriBase($uriBase = null) {
        if(empty($uriBase)) {
            throw new \InvalidArgumentException('URI base not provided.');
        } else if(!is_string($uriBase)) {
            throw new \InvalidArgumentException('Non-string URI base provided.');
        }

        // make sure we always have forward slash at beginning and end of uriBase
        $uriBase = '/' . ltrim($uriBase, '/');
        $uriBase = rtrim($uriBase, '/') . '/';
        $this->uriBase = $uriBase;

        return $this;
    }

    /**
     * Sets whether SSL is to be used
     * 
     * @param boolean $value
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setUseSsl($value = null) {
        if (!is_bool($value)) {
            throw new \InvalidArgumentException('Non-boolean value passed as parameter.');
        }
        $this->useSsl = $value;

        return $this;
    }

    /**
     * Sets whether SSL Test Mode is to be used
     * 
     * @param boolean $value
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setUseSslTestMode($value = null) {
        if (!is_bool($value)) {
            throw new \InvalidArgumentException('Non-boolean value passed as parameter.');
        }
        $this->useSslTestMode = $value;

        return $this;
    }
    /**
     * Sets basic authentication credentials
     * 
     * @param string $user
     * @param string $password
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setBasicAuthCredentials($user = null, $password = null) {
        if (empty($user)) {
            throw new \InvalidArgumentException('User name not provided when trying to set basic authentication credentials.');
        }
        if (empty($password)) {
            throw new \InvalidArgumentException('Password not provided when trying to set basic authentication credentials.');
        }

        $this->useBasicAuth = true;
        $this->basicAuthUsername = $user;
        $this->basicAuthPassword = $password;

        return $this;
    }
    /**
     * Sets HTTP headers from an associative array where key is header name and value is the header value
     * 
     * @param array $headers
     * @return RestClient
     */
    public function setHeaders(array $headers) {
        if(empty($headers)) {
            throw new \InvalidArgumentException('Empty array passed when triyng to set headers');
        }
        $this->headers = $headers;

        return $this;  
    }

    /**
     * Sets maximum timeout for cURL requests
     * 
     * @param integer $seconds
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setTimeout($seconds = null) {
        if(!is_integer($seconds) || $seconds < 0) {
            throw new \InvalidArgumentException('A non-negative integer value must be passed when trying to set timeout');
        }
        $this->timeout = $seconds;

        return $this;
    }

    /**
     * Sets flag on whether to follow 3XX redirects.
     * 
     * @param boolean $follow
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setFollowRedirects($follow = null) {
        if(!is_bool($follow)) {
            throw new \InvalidArgumentException('Non-boolean value passed as parameter.');
        }
        $this->followRedirects = $follow;

        return $this;
    }

    /**
     * Sets maximum number of redirects to follow. A value of 0 represents no redirect limit. Also sets followRedirects property to true .
     * 
     * @param integer $redirects
     * @return RestClient
     * @throws \InvalidArgumentException
     */
    public function setMaxRedirects($redirects = null) {
        if(!is_integer($redirects) || $redirects < 0) {
            throw new \InvalidArgumentException('A non-negative integer value must be passed when trying to set max redirects.');
        }
        $this->maxRedirects = $redirects;
        $this->setFollowRedirects(true);

        return $this;
    }

    /**
     * Get remote host setting
     * 
     * @return string
     */
    public function getRemoteHost() {
        return $this->remoteHost;
    }

    /**
     * Get URI Base setting
     * 
     * @return string
     */
    public function getUriBase() {
        return $this->uriBase;
    }

    /**
     * Get boolean setting indicating whether SSL is to be used
     * 
     * @return boolean
     */
    public function getUseSsl() {
        return $this->useSsl;
    }

    /**
     * Get boolean setting indicating whether SSL test mode is enabled
     * 
     * @return boolean
     */
    public function getUseSslTestMode() {
        return $this->useSslTestMode;
    }

    /**
     * Get timeout setting
     * 
     * @return integer
     */
    public function getTimeout() {
        return $this->timeout;
    }

    /**
     * Get follow redirects setting
     * 
     * @return boolean
     */
    public function getFollowRedirects() {
        return $this->followRedirects;
    }

    /**
     * Get max redirects setting
     * 
     * @return integer
     */
    public function getMaxRedirects() {
        return $this->maxRedirects;
    }

    /**
     * Returns URL used for last request
     * 
     * @return string
     */
    public function getRequestUrl() {
        return $this->requestUrl;
    }

    /**
     * Returns data sent with last request (i.e. POST/PUT data)
     * 
     * @return mixed
     */
    public function getRequestData() {
        return $this->requestData;
    }

    /**
     * Returns request header for last request
     * 
     * @return string
     */
    public function getRequestHeader() {
        return $this->requestHeader;
    }

    /**
     * Returns reespsone code for last request
     * 
     * @return integer
     */
    public function getResponseCode() {
        return $this->responseCode;
    }

    /**
     * Returns cURL response information array from last request
     * 
     * @return array
     */
    public function getResponseInfo() {
        return $this->responseInfo;
    }

    /**
     * Returns response body from last request
     * 
     * @return string
     */
    public function getResponseBody() {
        return $this->responseBody;
    }

    /**
     * Method to initialize cURL handle in object
     * 
     * @return void
     * @throws \Exception
     */
    protected function curlSetup() {        
        // reset all request/response properties
        $this->resetRequestResponseProperties();

        // initialize cURL. Throws \Exception on failure.
        $this->curl = $this->curlInit();
    }

    /**
     * Method to initilize a cURL handle
     * 
     * @return resource
     * @throws \Exception
     */
    protected function curlInit() {
        // initialize cURL
        $curl = curl_init();
        if($curl === false) {
            throw new \Exception('cURL failed to initialize.');
        }
        // set timeout
        curl_setopt($curl, CURLOPT_TIMEOUT, $this->timeout);

        // set basic HTTP authentication settings
        if (true === $this->useBasicAuth) {
            curl_setopt($curl, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
            curl_setopt($curl, CURLOPT_USERPWD, $this->basicAuthUsername . ':' . $this->basicAuthPassword);
        }

        // set headers
        if (!empty($this->headers)) {
            $headers = array();
            foreach ($this->headers as $key=>$val) {
                $headers[] = $key . ': ' . $val;
            }
            curl_setopt($curl, CURLOPT_HTTPHEADER, $headers);
        }

        // if not in production environment, we want to ignore SSL validation
        if (true === $this->useSsl && true === $this->useSslTestMode) {
            curl_setopt($curl, CURLOPT_SSL_VERIFYPEER, false);
            curl_setopt($curl, CURLOPT_SSL_VERIFYHOST, false);
        }

        // set option to add request header information to curl_getinfo output
        curl_setopt($curl, CURLINFO_HEADER_OUT, true);

        // set option to return content body
        curl_setopt($curl, CURLOPT_RETURNTRANSFER, true);

        // set redirect options
        if (true === $this->followRedirects) {
            curl_setopt($curl, CURLOPT_FOLLOWLOCATION, true);
            if ($this->maxRedirects > 0) {
                curl_setopt($curl, CURLOPT_MAXREDIRS, $this->maxRedirects);
            }
        }

        return $curl;
    }

    /**
     * Method to reset curl handle
     * 
     * @return void
     */
    protected function curlTeardown() {
        $this->curlClose($this->curl);
        $this->curl = null;
    }

    /**
     * Method to close cURL handle
     * 
     * @return void
     */
    protected function curlClose($curl = null) {
        curl_close($curl);
    }

    /**
     * Method to execute cURL call
     * 
     * @return void
     * @throws \Exception
     */
    protected function curlExec() {
        $curlResult = curl_exec($this->curl);
        if($curlResult === false) {
            // our cURL call failed for some reason
            $curlError = curl_error($this->curl);
            $this->curlTeardown();
            throw new \Exception('cURL call failed with message: "' . $curlError. '"');
        }

        // set object properties for request/response
        $curlInfo = curl_getinfo($this->curl);
        $this->responseInfo = $curlInfo;
        $this->requestHeader = $this->responseInfo['request_header'];
        $this->responseCode = $this->responseInfo['http_code'];
        $this->responseBody = $curlResult;
        $this->curlTeardown();
    }

    /**
     * Method to reset all properties specific to a particular request/response sequence.
     * 
     * @return void
     */
    protected function resetRequestResponseProperties() {
        $this->requestUrl = null;
        $this->requestHeader = null;
        $this->requestData = null;
        $this->responseCode = null;
        $this->responseInfo = null;
        $this->responseBody = null;
    }

    /**
     * Method to set the url for request
     * 
     * @param string $action
     * @return void
     */
    protected function setRequestUrl($action) {
        $url = $this->buildUrl($action);
        $this->requestUrl = $url;
        curl_setopt($this->curl, CURLOPT_URL, $url);
    }

    /**
     * Method to set a URL on a cURL handle.
     * 
     * @param string $action
     * @return string
     */
    protected function buildUrl($action) {
        if (true === $this->useSsl) {
            $url = 'https://';
        } else {
            $url = 'http://';
        }

        $url = $url . $this->remoteHost . $this->uriBase . $action;
        return $url;
    }

    /**
     * Method to set data to be sent along with POST/PUT requests
     * 
     * @param mixed $data
     * @return void
     */
    protected function setRequestData($data) {
        $this->requestData = $data;
        curl_setopt($this->curl, CURLOPT_POSTFIELDS, $data);
    }

    /**
     * Method to provide common validation for action parameters
     * 
     * @param string $action
     * @return void
     * @throws \InvalidArgumentException
     */
    protected function validateAction($action) {
        if(!is_string($action)) {
            throw new \InvalidArgumentException('A non-string value was passed for action parameter');
        }
    }

    /**
     * Method to provide common validation for data parameters
     * 
     * @param mixed $data
     * @return void
     * @throws \InvalidArgumentException
     */
    protected function validateData($data) {
        if(empty($data)) {
            throw new \InvalidArgumentException('An empty value was passed for data parameter');
        }
    }
}

RestClientTest.php

<?php

namespace MikeBrant\RestClientLib;

use MikeBrant\RestClientLib\RestClient;
use PHPUnit\Framework\TestCase;

/**
 * Mock for curl_init global function
 * 
 * @return mixed
 */
function curl_init() {
    if (!is_null(RestClientTest::$curlInitResponse)) {
        return RestClientTest::$curlInitResponse;
    }
    return \curl_init();
}

/**
 * Mock for curl_exec global function
 * 
 * @return mixed
 */
function curl_exec($curl) {
    if (!is_null(RestClientTest::$curlExecResponse)) {
        return RestClientTest::$curlExecResponse;
    }
    return \curl_exec($curl);
}

/**
 * Mock for curl_error global function
 * 
 * @return mixed
 */
function curl_error($curl) {
    if (!is_null(RestClientTest::$curlErrorResponse)) {
        return RestClientTest::$curlErrorResponse;
    }
    return \curl_error($curl);
}

/**
 * Mock for curl_getinfo function
 * 
 * @return mixed
 */
function curl_getinfo($curl) {
    if (!is_null(RestClientTest::$curlGetInfoResponse)) {
        return RestClientTest::$curlGetInfoResponse;
    }
    return \curl_getinfo($curl);
}

/**
 * @covers RestClient
 */
class RestClientTest extends TestCase{
    protected $client = null;

    protected $curlExecMockResponse = 'Test Response';

    protected $curlGetInfoMockResponse = array(
        'url' => 'http://google.com/',
        'content_type' => 'text/html; charset=UTF-8',
        'http_code' => 200,
        'header_size' => 321,
        'request_size' => 49,
        'filetime' => -1,
        'ssl_verify_result' => 0,
        'redirect_count' => 0,
        'total_time' => 1.123264,
        'namelookup_time' => 1.045272,
        'connect_time' => 1.070183,
        'pretransfer_time' => 1.071139,
        'size_upload' => 0,
        'size_download' => 219,
        'speed_download' => 194,
        'speed_upload' => 0,
        'download_content_length' => 219,
        'upload_content_length' => -1,
        'starttransfer_time' => 1.122377,
        'redirect_time' => 0,
        'redirect_url' => 'http://www.google.com/',
        'primary_ip' => '216.58.194.142',
        'certinfo' => array(),
        'primary_port' => 80,
        'local_ip' => '192.168.1.74',
        'local_port' => 59733,
        'request_header' => "GET / HTTP/1.1\nHost: google.com\nAccept: */*",
    );

    public static $curlInitResponse = null;

    public static $curlExecResponse = null;

    public static $curlErrorResponse = null;

    public static $curlGetInfoResponse = null;

    protected function setUp() {
        self::$curlInitResponse = null;
        self::$curlExecResponse = null;
        self::$curlErrorResponse = null;
        self::$curlGetInfoResponse = null;
        $this->client = new RestClient();
    }

    protected function tearDown() {
        $this->client = null;
    }

    public function notStringProvider() {
        return array(
            array(null),
            array(new \stdClass()),
            array(1),
            array(0),
            array(true),
            array(false),
            array(array())
        );
    }

    public function emptyProvider() {
        return array(
            array(null),
            array(''),
            array(0),
            array(0.0),
            array(false),
            array('0'),
            array(array())
        );
    }

    public function notStringAndEmptyProvider() {
        return array(
            array(null),
            array(''),
            array(new \stdClass()),
            array(1),
            array(0),
            array(0.0),
            array('0'),
            array(true),
            array(false),
            array(array())
        );
    }

    public function hostProvider() {
        return array(
            array('somedomain.com', 'somedomain.com', false),
            array('somedomain.com/', 'somedomain.com', false),
            array('https://somedomain.com', 'somedomain.com', true),
            array('http://somedomain.com', 'somedomain.com', false),
            array('somedomain.com:80', 'somedomain.com:80', false),
            array('somedomain.com:443', 'somedomain.com:443', true),
            array('somedomain.com:8443', 'somedomain.com:8443', true)
        );
    }

    public function notBooleanProvider() {
        return array(
            array(null),
            array(''),
            array('string'),
            array('true'),
            array('false'),
            array(1),
            array(0),
            array('1'),
            array('0'),
            array(0.0),
            array(new \stdClass()),
            array(array())
        );
    }

    public function uriBaseProvider() {
        return array(
            array('test', '/test/'),
            array('/test', '/test/'),
            array('test/', '/test/'),
            array('/test/', '/test/')
        );
    }

    public function notZeroOrPositiveIntegerProvider() {
        return array(
            array(-1),
            array(null),
            array(''),
            array(new \stdClass()),
            array(1.0),
            array('1'),
            array(array())
        );
    }

    public function headersProvider() {
        return array(
            array(
                array(
                    'header1' => 'header1 value',
                    'header2' => 'header2 value'
                )
            )
        );
    }

    public function buildUriProvider() {
        return array(
            array(true, 'google.com', 'base', 'action', 'https://google.com/base/action'),
            array(false, 'google.com', 'base', 'action', 'http://google.com/base/action')
        );
    }

    /**
     * @dataProvider notStringProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::validateAction
     */
    public function testValidateActionThrowsExceptions($action) {
        $this->client->get($action);
    }

    /**
     * @dataProvider emptyProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::validateData
     */
    public function testValidateDataThrowsExceptions($data) {
        $this->client->post('', $data);
    }

    /**
     * @dataProvider notStringAndEmptyProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setRemoteHost
     */
    public function testSetRemoteHostThrowsExceptions($host) {
        $this->client->setRemoteHost($host);
    }

    /**
     * @dataProvider hostProvider
     * @covers RestClient::setRemoteHost
     * @covers RestClient::getRemoteHost
     */
    public function testSetRemoteHost($hostInput, $hostOutput, $useSslSet) {
        $this->client->setRemoteHost($hostInput);
        $this->assertEquals($hostOutput, $this->client->getRemoteHost());
        $this->assertEquals($useSslSet, $this->client->getUseSsl());
    }

    /**
     * @dataProvider notStringAndEmptyProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setUriBase
     */
    public function testSetUriBaseThrowsExceptions($string) {
        $this->client->setUriBase($string);
    }

    /**
     * @dataProvider uriBaseProvider
     * @covers RestClient::setUriBase
     * @covers RestClient::getUriBase
     */
    public function testSetUriBase($stringInput, $stringOutput) {
        $this->client->setUriBase($stringInput);
        $this->assertEquals($stringOutput, $this->client->getUriBase());
    }

    /**
     * @dataProvider notBooleanProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setUseSsl
     */
    public function testSetUseSslThrowsExceptions($boolean) {
        $this->client->setUseSsl($boolean);
    }

    /**
     * @covers RestClient::setUseSsl
     * @covers RestClient::getUseSsl
     */
    public function testSetUseSsl() {
        $this->client->setUseSsl(true);
        $this->assertTrue($this->client->getUseSsl());
        $this->client->setUseSsl(false);
        $this->assertFalse($this->client->getUseSsl());
    }

    /**
     * @dataProvider notBooleanProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setUseSslTestMode
     */
    public function testSetUseSslTestModeThrowsExceptions($boolean) {
        $this->client->setUseSslTestMode($boolean);
    }

    /**
     * @covers RestClient::setUseSslTestMode
     * @covers RestClient::getUseSslTestMode
     */
    public function testSetUseSslTestMode() {
        $this->client->setUseSslTestMode(true);
        $this->assertTrue($this->client->getUseSslTestMode());
        $this->client->setUseSslTestMode(false);
        $this->assertFalse($this->client->getUseSslTestMode());
    }

    /**
     * @dataProvider emptyProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setBasicAuthCredentials
     */
    public function testSetBasicAuthCredentialsThrowsExceptionOnEmptyUser($user) {
        $this->client->setBasicAuthCredentials($user, 'password');
    }

    /**
     * @dataProvider emptyProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setBasicAuthCredentials
     */
    public function testSetBasicAuthCredentialsThrowsExceptionOnEmptyPassword($password) {
        $this->client->setBasicAuthCredentials('user', $password);
    }

    /**
     * @covers RestClient::setBasicAuthCredentials
     */
    public function testSetBasicAuthCredentials() {
        $this->client->setBasicAuthCredentials('user', 'password');
        $this->assertAttributeEquals('user', 'basicAuthUsername', $this->client);
        $this->assertAttributeEquals('password', 'basicAuthPassword', $this->client);
        $this->assertAttributeEquals(true, 'useBasicAuth', $this->client);
    }

    /**
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setHeaders
     */
    public function testSetHeadersThrowsExceptionOnEmptyArray() {
        $this->client->setHeaders(array());
    }

    /**
     * @dataProvider headersProvider
     * @covers RestClient::setHeaders
     */
    public function testSetHeaders($headers) {
        $this->client->setHeaders($headers);
        $this->assertAttributeEquals($headers, 'headers', $this->client);
    }

    /**
     * @dataProvider notZeroOrPositiveIntegerProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setTimeout
     */
    public function testSetTimeoutThrowsExceptions($int) {
        $this->client->setTimeout($int);
    }

    /**
     * @covers RestClient::setTimeout
     * @covers RestClient::getTimeout
     */
    public function testSetTimeout() {
        $this->client->setTimeout(30);
        $this->assertEquals(30, $this->client->getTimeout());
        $this->client->setTimeout(0);
        $this->assertEquals(0, $this->client->getTimeout());
    }

    /**
     * @dataProvider notBooleanProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setFollowRedirects
     */
    public function testSetFollowRedirectsThrowsExceptions($boolean) {
        $this->client->setFollowRedirects($boolean);
    }

    /**
     * @covers RestClient::setFollowRedirects
     * @covers RestClient::getFollowRedirects
     */
    public function testSetFollowRedirects() {
        $this->client->setFollowRedirects(true);
        $this->assertTrue($this->client->getFollowRedirects());
        $this->client->setFollowRedirects(false);
        $this->assertFalse($this->client->getFollowRedirects());
    }

    /**
     * @dataProvider notZeroOrPositiveIntegerProvider
     * @expectedException \InvalidArgumentException
     * @covers RestClient::setMaxRedirects
     */
    public function testSetMaxRedirectsThrowsExceptions($int) {
        $this->client->setMaxRedirects($int);
    }

    /**
     * @covers RestClient::setMaxRedirects
     * @covers RestClient::getMaxRedirects
     */
    public function testSetMaxRedirects() {
        $this->client->setMaxRedirects(1);
        $this->assertEquals(1, $this->client->getMaxRedirects());
        $this->assertTrue($this->client->getFollowRedirects());
        $this->client->setMaxRedirects(0);
        $this->assertEquals(0, $this->client->getMaxRedirects());
        $this->assertTrue($this->client->getFollowRedirects());
    }

    /**
     * @expectedException \Exception
     * @covers RestClient::curlInit
     */
    public function testCurlInitThrowsException() {
        self::$curlInitResponse = false;
        $this->client->get('action');
    }

    /**
     * @expectedException \Exception
     * @covers RestClient::curlExec
     */
    public function testCurlExecThrowsException() {
        self::$curlExecResponse = false;
        self::$curlErrorResponse = 'test error';
        $this->client->get('action');
    }

    /**
     * @dataProvider buildUriProvider
     * @covers RestClient::get
     * @covers REstClient::curlSetup
     * @covers RestClient::resetRequestResponseProperties
     * @covers RestClient::curlInit
     * @covers RestClient::setRequestUrl
     * @covers RestClient::curlExec
     * @covers RestClient::getRequestUrl
     * @covers RestClient::getResponse
     * @covers RestClient::getRequestHeader
     * @covers RestClient::getResponseCode
     * @covers RestClient::getResponseBody
     * @covers RestClient::curlTeardown
     * @covers REstClient::curlClose
     */
    public function testGet($useSsl, $host, $uriBase, $action, $expectedUrl) {
        self::$curlExecResponse = $this->curlExecMockResponse;
        self::$curlGetInfoResponse = $this->curlGetInfoMockResponse;
        $this->client->setBasicAuthCredentials('user', 'password')
                     ->setHeaders(array('header' => 'header value'))
                     ->setUseSsl($useSsl)
                     ->setUseSslTestMode(true)
                     ->setFollowRedirects(true)
                     ->setMaxRedirects(1)
                     ->setremoteHost($host)
                     ->setUriBase($uriBase)
                     ->get($action);
        $this->assertEquals($expectedUrl, $this->client->getRequestUrl());
        $this->assertEquals($this->curlGetInfoMockResponse, $this->client->getResponseInfo());
        $this->assertEquals($this->curlGetInfoMockResponse['request_header'], $this->client->getRequestHeader());
        $this->assertEquals($this->curlGetInfoMockResponse['http_code'], $this->client->getResponseCode());
        $this->assertEquals($this->curlExecMockResponse, $this->client->getResponseBody());
        $this->assertAttributeEquals(null, 'curl', $this->client);
        $this->assertEquals(null, $this->client->getRequestData());
    }

    /**
     * @covers RestClient::post
     * @covers RestClient::setRequestData
     */
    public function testPost() {
        self::$curlExecResponse = $this->curlExecMockResponse;
        self::$curlGetInfoResponse = $this->curlGetInfoMockResponse;
        $this->client->post('', 'test post data');
        $this->assertEquals($this->curlGetInfoMockResponse, $this->client->getResponseInfo());
        $this->assertEquals($this->curlGetInfoMockResponse['request_header'], $this->client->getRequestHeader());
        $this->assertEquals($this->curlGetInfoMockResponse['http_code'], $this->client->getResponseCode());
        $this->assertEquals($this->curlExecMockResponse, $this->client->getResponseBody());
        $this->assertEquals('test post data', $this->client->getRequestData());
        $this->assertAttributeEquals(null, 'curl', $this->client);
   }

    /**
     * @covers RestClient::put
     */
    public function testPut() {
        self::$curlExecResponse = $this->curlExecMockResponse;
        self::$curlGetInfoResponse = $this->curlGetInfoMockResponse;
        $this->client->put('', 'test put data');
        $this->assertEquals($this->curlGetInfoMockResponse, $this->client->getResponseInfo());
        $this->assertEquals($this->curlGetInfoMockResponse['request_header'], $this->client->getRequestHeader());
        $this->assertEquals($this->curlGetInfoMockResponse['http_code'], $this->client->getResponseCode());
        $this->assertEquals($this->curlExecMockResponse, $this->client->getResponseBody());
        $this->assertEquals('test put data', $this->client->getRequestData());
        $this->assertAttributeEquals(null, 'curl', $this->client);
    }

    /**
     * @covers RestClient::delete
     */
    public function testDelete() {
        self::$curlExecResponse = $this->curlExecMockResponse;
        self::$curlGetInfoResponse = $this->curlGetInfoMockResponse;
        $this->client->delete('');
        $this->assertEquals($this->curlGetInfoMockResponse, $this->client->getResponseInfo());
        $this->assertEquals($this->curlGetInfoMockResponse['request_header'], $this->client->getRequestHeader());
        $this->assertEquals($this->curlGetInfoMockResponse['http_code'], $this->client->getResponseCode());
        $this->assertEquals($this->curlExecMockResponse, $this->client->getResponseBody());
        $this->assertAttributeEquals(null, 'curl', $this->client);
    }

        /**
     * @covers RestClient::head
     */
    public function testHead() {
        self::$curlExecResponse = '';
        self::$curlGetInfoResponse = $this->curlGetInfoMockResponse;
        $this->client->head('');
        $this->assertEquals($this->curlGetInfoMockResponse, $this->client->getResponseInfo());
        $this->assertEquals($this->curlGetInfoMockResponse['request_header'], $this->client->getRequestHeader());
        $this->assertEquals($this->curlGetInfoMockResponse['http_code'], $this->client->getResponseCode());
        $this->assertEquals('', $this->client->getResponseBody());
        $this->assertAttributeEquals(null, 'curl', $this->client);
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ that code will be my reference for learning unit testing and documenting my codes. thanks for sharing that sexy code \$\endgroup\$ Mar 3, 2017 at 4:52
  • \$\begingroup\$ @Accountantم Glad that this helped your thinking. \$\endgroup\$
    – Mike Brant
    Mar 3, 2017 at 15:07

2 Answers 2

4
+100
\$\begingroup\$

When I look at the RestClient class, what is required to make a request?

$client = new RestClient();
$client->get("http://codereview.stackexchange.com");

At first glance, this appears all I need to do, right? But then I see method like setRequestUrl and setRequestData and I'm not so sure. I also see an empty constructor.

I think the confusion comes from this class doing too much. I see this actually requiring three classes:

  1. RestClient - A wrapper for RESTful calls
    • get($url, $params = null): HttpResponse
    • post($url, $params = null): HttpResponse
    • And put, delete, and head methods
    • Settings defining things like authentication, SSL, timeouts, etc.
  2. HttpRequest - A wrapper class defining an HTTP request
    • HTTP verb (GET, PUT, POST, DELETE, HEAD)
    • Request headers
    • Request body
    • Query string params
  3. HttpResponse - A wrapper class for the response
    • Response status
    • Response headers
    • Response body
    • A reference to the HttpRequest object that generated this response maybe?

You could even consider having the RestClient throw specific Exceptions when non 200 or 201 HTTP status codes are returned:

class RestResponseException extends Exception
{
    private $response;

    public function __construct(HttpResponse $response, int $httpStatusCode) {
        $this->response = $response;
    }

    public function getResponse() {
        return $this->response;
    }
}

// 404 Not Found
class ResourceNotFoundException extends RestResponseException
{
    public const STATUS_CODE = 404;

    public function __construct(HttpResponse $response) {
        parent::__construct($response, STATUS_CODE);
    }
}

This gives you a few more options when handling error conditions:

$client = new RestClient();

try {
    $response = $client->get("...");
} catch (ResourceNotFoundException $notFoundError) {
    echo "Couldn't find X";
} catch (RestResponseException $responseError) {
    echo "Something went wrong, try again."
} catch (Exception $ex) {
    // Log the exception, something outside the REST request/response went wrong
}

Mike Brant commented:

... I think exception-response concept is interesting and certainly something similar to what may be seen in other libraries of this sort. My initial intention was to not impose response interpretation in this class.

This is a good point. In fact, I would argue that throwing exceptions AND just returning a response with no exception is perfectly valid. It depends on the use case.

If you can gracefully handle 4xx and 5xx HTTP responses, then there is no need for an exception.

If your code absolutely must work and you can't handle the 4xx or 5xx responses, exceptions are the way to go.

Both are useful, and that might be a good option to include in the RestClient - whether or not 4xx and 5xx responses should throw exceptions.

$response = $client->get("...");

if ($response->getStatus() != 200) {
    // Show gracefull error message
}

The code above says "If a non 200 response is received, no big deal. We'll show a message to the user. Now consider the following code:

$response = $client->post("...", data);

echo "Blog post created successfully";

Here, a POST is modifying or creating data on the server. No error handling is done, because we really can't handle server problems when POST-ing data (creating a Blog Post). An exception here is useful because seeing "Blog post created successfully" on screen doesn't make sense if it wasn't created - and your PHP script can't do anything about it.

There is a use case for both kinds of error handling: Error codes and exceptions.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your thoughts. I was kind of getting to the same conclusion the you arrived at with regards to possibly refactoring this class along the lines that you have suggested, just wanted to get some initial code review thoughts first. The set* methods you mention are actually protected methods that set URL and data on cURL handle and not part of the public interface of the class. I think exception-response concept is interesting and certainly something similar to what may be seen in other libraries of this sort. My initial intention was to not impose response interpretation in this class \$\endgroup\$
    – Mike Brant
    Mar 3, 2017 at 15:04
  • 1
    \$\begingroup\$ @MikeBrant: I updated my answer after reading your comment. \$\endgroup\$ Mar 3, 2017 at 16:56
  • \$\begingroup\$ Agree with most recent thoughts. As I look to refactor, I might consider adding a setting around handling unexpected HTTP response codes as exceptions. (i.e. non 2XX unless setting to follow 3XX redirects is on). \$\endgroup\$
    – Mike Brant
    Mar 3, 2017 at 20:28
2
\$\begingroup\$
/**
  * Flag to determine if basic authentication is to be used.
 * 
 * @var boolean
 */
protected $useBasicAuth = false;

/**
 * User Name for HTTP Basic Auth
 * 
 * @var string
 */
protected $basicAuthUsername = null;

/**
 * Password for HTTP Basic Auth
 *
 * @var string
 */
protected $basicAuthPassword = null;

Looks like the indentation went wrong on the second line, but more seriously: is $useBasicAuth actually useful? I think you could eliminate it entirely and just test whether $basicAuthUsername is non-null.


public function get($action = null) {

If $action == null then the method will throw an exception. So why offer the default value? If this were

public function get(string $action) {

then good IDEs would be able to offer you better auto-completion.

This applies to most of the method definitions, and I've just taken that one as an example because it was the first.


/**
 * Sets URI base for the instance
 * 
 * @param string $uriBase
 * @return RestClient
 * @throws \InvalidArgumentException
 */

If I'm looking just at the generated documentation and not at the code then it's not obvious what the URI base is. I would have guessed that it includes the scheme, host, port, and a path prefix. Would "URI base path" or "URI path prefix" be a better name than "URI base"?


There's a lot of shared code between the five HTTP action methods. I would be tempted to factor out a private method

private function call($method, $action, $data) {
    $this->validateAction($action);

    switch ($action) {
        case 'POST':
        case 'PUT':
            $this->validateData($data);
            break;

        default:
            break;
    }

    $this->curlSetup();
    $this->setRequestUrl($action);
    curl_setopt($this->curl, CURLOPT_CUSTOMREQUEST, $method);

    switch ($action) {
        case 'POST':
        case 'PUT':
            $this->setRequestData($data);
            break;

        case 'HEAD':
            curl_setopt($this->curl, CURLOPT_NOBODY, true);
            break;

        default:
            break;
    }

    $this->curlExec();
    return $this;
}

But this is clearly a matter of taste, and I can understand if you prefer to avoid the switches.


Overall looks pretty good. I can see you've put effort into documentation and it's good to see === used to test Booleans.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for thoughts. You are right about potentially eliminate basic auth flag. I have actually already removed the default values in some refactoring I have done since posting this, so agree on that point. Have been toying with thought of consolidating some method logic for get, post, put, etc. but not very fond of the switch approach (I think it is consolidation for sake of consolidation and doesn't really simplify). Not type-hinting primitives here as I am maintaining PHP 5.6 compatibility (something I should have mentioned in post). \$\endgroup\$
    – Mike Brant
    Mar 6, 2017 at 15:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.