PHP HTTP request class

Question

I would like to get you opinion on this class, which is used to send HTTP requests (at that moment only POST method).

It is working ok, but I would like to get response on maintainability, reusability, security, code formatting, etc...

Class HttpRequest{

    private $POST = 'POST';
    private $PUT = 'PUT';
    private $GET = 'GET';
    private $DELETE = 'DELETE';
    private $PATCH = 'PATCH';

    private $body;
    private $options;
    private $handle;
    private $httpCode;
    private $response;

    public function __construct(){}


    /**
     * send post request
     * @param url 
     * @param header 
     * @param options
     * @param body
     * @return json object
     */
    public function post($url, $header, $options, $body){
        if(!$this->handle || get_resource_type($this->handle) == "Unknown"){
            $this->handle = curl_init(); 
        }
        curl_setopt($this->handle, CURLOPT_URL, $url);
        curl_setopt($this->handle, CURLOPT_HTTPHEADER, $header);
        curl_setopt_array($this->handle, $options);
        // curl_setopt($this->handle, CURLOPT_SSL_VERIFYPEER, true);
        // curl_setopt($this->handle, CURLOPT_SSL_VERIFYHOST, 0);
        curl_setopt($this->handle, CURLOPT_RETURNTRANSFER, true);
        curl_setopt($this->handle, CURLOPT_CUSTOMREQUEST, $this->POST);
        curl_setopt($this->handle, CURLOPT_POSTFIELDS, $body);
        $this->response = curl_exec($this->handle);
        $this->httpCode = curl_getinfo($this->handle, CURLINFO_HTTP_CODE);
        curl_close($this->handle);
        return $this->response;
    }


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

    public function getHttpCode(){
        return $this->httpCode;
    }

    /**
     * send get request
     * @param url 
     * @param header 
     * @param options
     */
    public function get($url,$header=array(), $options=array()){
        /**
         * @todo 
         * implemets this method
         */
    }

    /**
     * send patch request
     */ 
    public function patch(){
        /**
         * @todo 
         * implemets this method
         */
    }
    /**
     * send delete request
     */
    public function delete(){
        /**
         * @todo 
         * implemets this method
         */
    }
    /**
     * send put request
     */
    public function put(){
        /**
         * @todo 
         * implemets this method
         */
    }
}

?>

Answer 1

Use class constants to store values that will remain the same and are unchangeable

Your HTTP method verbs are better off set as class constants than class properties since they will remain the same and are unchangeable. (i.e. constant)

private $POST = 'POST';
private $PUT = 'PUT';
private $GET = 'GET';
private $DELETE = 'DELETE';
private $PATCH = 'PATCH';

becomes

const POST = 'POST';
const PUT = 'PUT';
const GET = 'GET';
const DELETE = 'DELETE';
const PATCH = 'PATCH';

You can then refer to them using self

self::POST

No need for an empty constructor

If your contractor doesn't have any code in it you could, and should omit it. It's just noise otherwise. You can always add it if the constructor definition changes.

Be sure to use docblock comments for your class properties

Not just class methods should have well written docblock comments, so should your class properties.

/**
 * @var string The body of the HTTP request
 */
private $body; 

Follow PSR coding standards

The PSR coding standards exist to ensure a high level of technical interoperability between shared PHP code. They also ensure conformity for projects with multiple developers.

PSR-2 says that:

Opening braces for classes MUST go on the next line, and closing braces MUST go on the next line after the body. and Opening braces for methods MUST go on the next line, and closing braces MUST go on the next line after the body.

Class HttpRequest{

becomes

Class HttpRequest
{

and

public function post($url, $header, $options, $body){

becomes

public function post($url, $header, $options, $body)
{

Use type hinting to variable types in class methods

From the manual:

Type declarations allow functions to require that parameters are of a certain type at call time. If the given value is of the incorrect type, then an error is generated

To use your code as an example, you can declare that $url must be a string and $header and $options must be arrays. This will be enforced by PHP at run time and prevent obvious and non obvious errors from occurring.

public function get($url,$header=array(), $options=array()){

becomes

public function get(string $url, array $header=[], array $options=[])
{

(I used shortened array syntax [] versus the more verbose array() for brevity.)

Use return type declarations

Just like you can enforce that class method parameters are a certain data type, you can also enforce the value returned by that method is a certain data type. Since all of your methods are stubs I'll pretend your HttpRequestget() method returns a Boolean to use as an example.

public function get(string $url, array $header=[], array $options=[]): bool
{

Now my calling code will know that a Boolean, and only a Boolean value, will be returned by that method.

When doing comparisons use === whenever possible

Unlike == which compares values only, === compares both values and type. This strict comparison helps to avoid error, and attacks, that occur when PHP encounters a comparison of two variables of different types it will coerce one of the variables into the type of the other variable in order to do the comparison.

For example

1 == '1'  // true
1 === '1' // false

How much does this matter? It depends. If you get into a situation where you are getting numbers as strings but you are trying to use them as numbers, for something like sorting, you can get unexpected results if your check only checks value instead of type. And those of us who remember phpBB remember when it was subject to a slew of high profile vulnerabilities many of which were resolved simply by using a stricter comparison. So, yes, it matters.

Answer 2

The most critical part is that this code is not a class. It's a function. A function just written in a form of a class method but it's a function all the same. Ok, it is using distinct methods to get the response and the code, but that's not enough to become a class. For this purpose you can make a function to return an array, like this

return ['response' => $response, 'code' => $code];

and no class would be ever needed.

I even have a feeling that this class is a direct attempt to convert a function into a class. But you really have to decompose this function into different methods.

Duplicated code

Suppose you are going to implement the GET method. Are you going to duplicate all this curl_init stuff in it? Come on, classes are written to reduce the duplication, not to multiply it. So you have to make a protected common execution method that would contain all the code common for all requests.

Constructor

Then remove from the function body that code to initialize curl, as it looks being a rudiment from the time when this class was a function, but now it looks alien here. This is the code that goes into constructor.

Configuration

Notice these 2 lines commented out

    // curl_setopt($this->handle, CURLOPT_SSL_VERIFYPEER, true);
    // curl_setopt($this->handle, CURLOPT_SSL_VERIFYHOST, 0);

suppose you are going to comment and uncomment them when required. This is not how classes are used. Move these lines in a distinct method and call it when you have to bypass the SSL verification.

Method chaining

Given there could be several methods called in consequence, consider returning $this from methods. It will allow you to use the neat practice called "method chaining":

$request = new HttpRequest();
$body = $request->setHeaders($headers)
    ->setNoSSLverification()
    ->post($url, $post_body)
    ->getResponse();

Answer 3

I like that you've tried to include some processing within object's methods, which is what objects should look like from OOP perspective, but you've chosen wrong case for it IMO.

Naming & API

The class name and its API is confusing. Request itself shouldn't produce the response - the remote server does. Request is usually the data structure type which contains values that can be read from it, so that class representing remote server could fetch response based on it. The API of this server (that, in your case, encapsulates bunch of curl commands) would be:

$response = $httpServer->send($request);

Your get(), post(), ...etc. methods are a good candidates for static factory methods (named constructors) producing concrete, possibly immutable request object, while replacing method parameter in primary constructor:

class HttpRequest
{
    public static function post($url, $header, $options, $body)
    {
        return self::__construct('POST', $url, $header, $options, $body);
    }

    public function __construct($method, $url, $header, $options, $body)
    {
        $this->method = $method;
        $this->url    = $url;
        ...
    }

    public function method(): string
    {
        return $this->method;
    }
    ...
}

Encapsulating I/O

Since http structure is well defined and request contains complete information, its handler (remote server object) will be generic low level library - it won't need to change depending on where this request is sent.

The layer (might be a single class) where you create this request, pass it to server object and process the response will be an adapter to abstract data port (google for ports and adapters). For example (ignore names here):

class FacebookService implements UserDataGateway
{
    public function __construct(APIConfig $config, RemoteServer $server)
    {
        $this->config = $config;
        $this->server = $server;
    }

    public function userData($userId): array
    {
        $request  = ... //prepare request using $this->config and given $userId
        $response = $this->server->send($request);

        return json_decode($response->body(), true);
    }
    ...
}

By encapsulating http, the class that will get (and call) UserDataGateway might work with this FacebookService as well as with local SQL database or other remote service providing user data - it only needds to return it based on some user's id. It can also be tested in isolation from remote/database calls (this example likely returns plain view model data, so ther will be no logic to test).

Already invented stuff

Take a look at PSR-7 and its implementations like Zend-Diactoros and remote server libraries like Guzzle which will implement PSR-18* in near future.

*) Calling remote server a Client is another kind of naming blunder IMO where you look at object from perspective of the entity it internally communicates with instead object that makes calls, but what can you do ¯\_(ツ)_/¯

Anyway, it would be strange if someone called me a "customer" at home after coming back with groceries for breakfast.