PHP Response Wrapper

Question

I'm writing a routing system that may or may not be part of a public API later for a personal project. A main part of the routing system is a response object for the user to send headers, status code, and the body of the response that is sent from the server.

I've created a response class that I think covers everything the user would want to do with the response (it could use a few more "convenience" functions however). I wanted to post it here to make sure my code is clean, and that I didn't miss anything that the end user might want to do.

An instance of the request class is provided to the user when a request is made to a specified URL (see my last code review)

I present my response class:

    class Response {

    private $headers = array();
    private $code = 200;
    private $body = '';
    private $sent = false;

    private $log;

    public function __construct() {
        $this->log = Logger::getLogger(get_class($this));
    }

    public function headers ($headers) { // Add some headers to the headers array
        $this->headers = array_merge($this->headers, $headers);
        return $this;
    }

    public function header ($key, $value) { // set a single header in the headers array
        $this->headers[$key] = $value;
        return $this;
    }

    public function code ($code) { // Set the status code
        $this->code = $code;
        return $this;
    }

    public function status ($code) { // Alternate method for setting the status code
        return $this->code($code);;
    }

    public function json($str) { // respond with json, set the body text and set the content-type header
        $this->header('Content-Type', 'application/json');
        if(is_array($str)) { // handle either raw JSON text, or php arrays
            $this->body = json_encode($str);
        } else {
            $this->body = $str;
        }
        return $this;
    }

    public function html ($str){ // respond with HTML
        $this->header('Content-Type', 'text/html');
        $this->body = $str;
        return $this;
    }

    public function form ($str) { // Respond with form data
        $this->header('Content-Type', 'application/x-www-form-urlencoded');
        // TODO: Allow the user to user an array
        $this->body = $str;
        return $this;
    }

    public function render ($file){ // Render an HTML file from the templates folder
        //TODO: Restrict to templates folder open
        //TODO: Add server-side rendering code
        $this->body = file_get_contents($file);
        return $this;
    }

    public function sent() { // Check if the request has been sent
        return $this->sent;
    }

    public function send () { // send the request
        if($this->sent()){
            $log->error('Attempted to call send on a sent response!');
            exit('Attempted to call send on a sent response!');
        }
        // Log the request for debugging
        $this->log->info($this->headers);
        $this->log->info('HTTP Responce Code: ' . $this->code);
        $this->log->info($this->body);
        // Set the headers
        foreach($this->headers as $key => $value) {
            header($key .': ' . $value);
        }
        // Set the status code
        http_response_code($this->code);

        // Send out the body
        echo $this->body;

        // Set the sent variable
        $this->sent = true;
    }
}

EDIT: The idea with the short and clear function names that all return $this is to be able to have clear and simple chainable responses. (e.g $res->code(200)->json(... Json...);)

Answer 1

I would tend to agree with comments from @sensorario that fluent interface may not make much sense here.

I am going to operate under the assumption that this Response class if being instantiated from a controller or the like. If that is the case, why would it make sense to have this fluent style? Presumably that controller know all the information it need to know in order to create a response and there are not going to be other things that decorate the response during it's short lifecycle.

If this is that case, why make code more complex in the controller and make this Response object more fragile in terms of being potentially put into a bad state (i.e incomplete field completion, etc.).

I would prefer to see the constructor enforce the data to be set on the object at the point of instantiation such that you know that the object has all the dependencies it needs to fulfill it's main method calls. For example:

public function __construct(
    $content,
    int $statusCode,
    Logger $logger,
    array $headers = [],
) {
    $this->content = $content;
    $this->statusCode = $statusCode;
    $this->logger = $logger;
    $this->headers  = $headers;
}

By doing this, you guarantee that all dependencies are met and you can totally eliminate all your setters.

This makes code to instantiate this object much cleaner and more foolproof as well, as it is way easier for someone to see all dependencies you need to pass an object while working in an IDE via code completion and method signatures than it is for a coder to have to know that they have to not only instantiate an object, but also call methods a, b, c, d, e, etc. on it to get it set up in a proper state.

---

Note the suggested use of dependency injection for your Logger. I assume the controller has access to the Logger already, so why should this class have to know how to instantiate it? Note that you only have happy-path case here for logger. What if instantiation fails? Agree with other suggestion about using PSR-3 compliant logger.

---

You have no validation on your public method parameters at all, either via type hinting or checking the passed values themselves. An object of this class can easily be put into a bad state, something I think would be critical for something as important as the response sending mechanism.

---

Do you really need key value format for your headers? You are kind of implying some level of header management here which does not really exist. What value do you get from storing headers as key-value store considering you have to transform them back to values usable by header() anyway? Unless you are performing more sophisticated header management (header content validation, ASCII-encoding of headers, header replacement, etc.) why not deal with accepting headers from caller transparently (i.e "send me headers as formatted for header()") as opposed to creating your own sort of header API.

---

If you decide to keep setters, why have two methods that do the same thing in code() and status(). Why have one break method chaining? There really isn't much value in returning the value the caller just sent you back to it.

---

The render() method seems out of place in this class. It should be up to controller or caller up the stack to prepare the content and inject into the Response object.

---

Your different format-specific methods seem oddly designed and incongruent with regards to sending headers. I would think that only a send() tpye message should begin output to the browser.

Right now, the call could do some wonky stuff like:

$response->json(...); // sends JSON header
$response->html(...); // sends HTML header
$response->send(); // sends other headers and response body

If you follow suggestion above about injecting dependencies (including content) at point of Response instantiation, then perhaps this simply looks like:

$response = new Response(...);
$response->sendJson();
//  or
$response->sendHtml();

With format-specific methods being simple wrappers to main send() method

public function sendJson() {
    $this->headers[] = 'Content=Type: application/json';
     return $this->send('json_encode');
}

public function send(callable $transformation = null) {

    if(!is_null($transformation)) {
        $this->content = call_user_func($transformation, $this->content);
    }
    $this->sendHeaders();
    echo $this->content;
    // if you need to pass back success to caller
    // alternately, perhaps this just exits
    return true;
}

---

exit('Attempted to call send on a sent response!');

Don't output system-level messages to standard out. Consider throwing exceptions in this class and letting calling code handle user-friendly error messaging.

---

Why log every request? Oftentimes code to perform debug-level logging might be triggered in a conditional controlled by an application-wide configuration setting. It seems odd to take on a logger dependency, just for informational logging anyways. Your server logs should be able to to give you the header and status information (if properly formatted). Logging each and every response seems like a little much and also a good way to potentially leak secure information into your application logs.

---

    if(is_array($str)) { // handle either raw JSON text, or php arrays
        $this->body = json_encode($str);
    } else {
        $this->body = $str;
    }

Why conditionally encode JSON? If you are sending application\json header, you better well be sending well-formatted JSON to the client (why would you break your own API contract?). What about passing objects? JSON allows for encoding more than just arrays.

---

You have a couple styling issues that you really should address.

• inconsistent spacing in function signatures (spacing around parentheses for parameters and bracing).
• a lot of comments same line as the code there are commenting on. This makes code hard to read due to horizontal scrolling. Comments should go on the line before the code they reference.

Answer 2

I this this class is not a Response. But a bundle of things.

class Response
{

Name its clear. But if it is clear, I suggest you to make this class final.

    private $headers = array();
    private $code = 200;
    private $body = '';
    private $sent = false;

By default a non sent response is 200 and without headers?

    private $log;

Here my first big confusion. Why a response should log something? A Response is just a response.

    public function __construct() {
        $this->log = Logger::getLogger(get_class($this));
    }

Alt ^_^. I really suggest you to inject a logger. If you like standards, I suggest to inject Psr\Log\LoggerInterface

    public function headers ($headers) { // Add some headers to the headers array
        $this->headers = array_merge($this->headers, $headers);
        return $this;
    }

I suggest 'set' prefix. From the api I do not understand if the method is a setter or a getter.

    public function header ($key, $value) { // set a single header in the headers array
        $this->headers[$key] = $value;
        return $this;
    }

As above: if this is a setter, ... I suggest 'set' prefix.

    public function code ($code) { // Set the status code
        $this->code = $code;
        return $this;
    }

Instead of comment "set the status code" change the method in `public function setStatusCode()".

    public function status ($code) { // Alternate method for setting the status code
        return $this->code($code);;
    }

Why?

    public function json($str) { // respond with json, set the body text and set the content-type header
        $this->header('Content-Type', 'application/json');
        if(is_array($str)) { // handle either raw JSON text, or php arrays

Why dont you use type hinting and create a method that accept array and one that accept text?

            $this->body = json_encode($str);
        } else {
            $this->body = $str;
        }
        return $this;
    }

    public function html ($str){ // respond with HTML

A method called toHtml is more appropriate here.

        $this->header('Content-Type', 'text/html');
        $this->body = $str;
        return $this;

Fluent interface is ok just for builders. This class should be rename in ResponseBuilder.

    }

    public function form ($str) { // Respond with form data

Why you comment your code instead of make it more readable?

        $this->header('Content-Type', 'application/x-www-form-urlencoded');
        // TODO: Allow the user to user an array
        $this->body = $str;
        return $this;
    }

    public function render ($file){ // Render an HTML file from the templates folder
        //TODO: Restrict to templates folder open
        //TODO: Add server-side rendering code
        $this->body = file_get_contents($file);
        return $this;
    }

    public function sent() { // Check if the request has been sent
        return $this->sent;
    }

This method return a boolan. Should be named to isSent. And also should have a return type hint like public function isSent() : bool.

    public function send () { // send the request

Comment is useless. $request->send() speaks very well.

        if($this->sent()){
            $log->error('Attempted to call send on a sent response!');
            exit('Attempted to call send on a sent response!');

I prefer Exceptions every time something unexpected happens.

        }

        // Log the request for debugging
        $this->log->info($this->headers);
        $this->log->info('HTTP Responce Code: ' . $this->code);
        $this->log->info($this->body);

Anytime you use a comment to group some lines of code, you should remove the comment and make a private method like $this->logRequestForDebugging().

        // Set the headers
        foreach($this->headers as $key => $value) {
            header($key .': ' . $value);
        }

$this->setHeaders()

        // Set the status code
        http_response_code($this->code);

$this->setResponseStatusCode()

        // Send out the body
        echo $this->body;

...

        // Set the sent variable
        $this->sent = true;
    }
}