Handling HTTP requests

Question

This class handles HTTP requests. It's a singleton class that parses URI to get the controller, method and parameters and then executes the controller's method.

Maybe parseUri should be in another class and maybe there is too much responsibility for this class.

I know that it can be improved. The execute method could be better because there are some repeated code.

<?php
class Request
{
 private static $_instance;
 private $_host;
 private $_request_uri;
 private $_script_name;
 private $_controller;
 private $_method;
 private $_parameters;
 private $_headers;
 private function __construct()
 {
 $this->_host = $_SERVER['HTTP_HOST'];
 $this->_request_uri = $_SERVER['REQUEST_URI'];
 $this->_script_name = $_SERVER['SCRIPT_NAME'];
 $this->_headers = array();
 $this->_parseUri();
 }
 private function _parseUri()
 {
 /* In 'http://www.example.com/foo/bar/' get the '/foo/bar/' part */
 $part = str_replace(
 dirname($this->_script_name),
 '',
 $this->_request_uri
 );
 /* break it into chunks and clean up empty positions */
 $chunks = explode('/', $part);
 $chunks = array_filter($chunks);
 if (count($chunks))
 {
 if ($this->_controller = array_shift($chunks))
 {
 $this->_controller = ucfirst($this->_controller) . '_Controller';
 }
 if ($this->_method = array_shift($chunks))
 {
 $this->_method = ucfirst($this->_method) . 'Action';
 }
 $this->_parameters = $chunks ? $chunks : null;
 }
 }
 private function _send_headers()
 {
 foreach($this->_headers as $header)
 {
 header($header);
 }
 }
 public static function instance()
 {
 if (!self::$_instance)
 {
 self::$_instance = new Request();
 }
 return self::$_instance;
 }
 public function execute()
 {
 /* There is a controller ... */
 if (!empty($this->_controller))
 {
 if (!class_exists($this->_controller, true))
 {
 array_push($this->_headers, 'HTTP/1.1 404 Not Found');
 $this->_send_headers();
 return;
 }
 $controller = new $this->_controller();
 /* ... and a method */
 if ($this->_method)
 {
 if (!method_exists($controller, $this->_method))
 {
 array_push($this->_headers, 'HTTP/1.1 404 Not Found');
 $this->_send_headers();
 return;
 }
 $method = new ReflectionMethod(
 $controller, $this->_method
 );
 /* Do we have parameters? Let's call the right method */
 $this->_parameters
 ? $method->invokeArgs($controller, $this->_parameters)
 : $method->invoke($controller);
 array_push($this->_headers, 'HTTP/1.1 200 OK');
 $this->_send_headers();
 }
 /* we don't have a method here, let's call the 'index_action' method */
 else
 {
 $method = new ReflectionMethod($controller, 'indexAction');
 $method->invoke($controller);
 array_push($this->_headers, 'HTTP/1.1 200 OK');
 $this->_send_headers();
 }
 }
 /* We don't have a Controller, let's call the default one. */
 else
 {
 if (!class_exists('Default_Controller', true))
 {
 throw new RequestException("'Default_Controller' class not found.");
 }
 $controller = new Default_Controller();
 if (!method_exists($controller, 'indexAction'))
 {
 throw new RequestException(
 "'indexAction' method not found in 'Default_Controller' class."
 );
 }
 $method = new ReflectionMethod($controller, 'indexAction');
 $method->invoke($controller);
 }
 }
}
?>

Answer 1

I think there is too much responsibility for this class. I also think that it's name is a misnomer. A more appropriate name would be a FrontController or RequestHandler rather than Request. When I see an object named Request I expect something that wraps around a HTTP request, providing easy access to the underlying request data and state. I.e. I expect something that is a request rather than something that processes requests.

If you want to reduce/divide the responsibilities of this class, have a look at how other PHP frameworks have divided these, for example, Symfony2. Symfony2 has a FrontController object (they call it a Kernel) that has two parts: a Router and a Resolver. You put Request objects into the Kernel and you get Response objects back.

Internally, the Kernel passes the Request to the Router. The Router figures out which Controller and Action are being requested. This information is then passed to the Resolver which tries to load the actual PHP class that holds the Controller and Action. The Kernel then executes the Controller/Action which returns a Response object, which is (usually) displayed by the Kernel.

Answer 2

Only something litle, don't write the closing ?> , because if there is a whitespace after and you try to modifiy the header anywhere else, it won't work and you will have very very long to find the whitespace.

• etheros: I disagree. I would argue that closing PHP mode is good practice.
• navitronic: "I disagree. I would argue that closing PHP mode is good practice" - You won't say that when you've wasted a day or more of time trying to find some errant whitespace.
• Jack: throw new NotACodeReviewException()