Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created an api in php and I was hoping some clever individuals would mind reviewing it to see what I could do better. I have commented it heavily to explain my reasoning. please feel free to pick holes wherever you can!

    //On my page 'localhost/api'

    if (!$_POST) {
        die('No direct access allowed');
    }

    include('/api/gateway.php'); //Pull in my Gateway class

    try {
        $api = new Gateway($_POST);    
    }
    catch (Exception $e) { 
        //Catch any exceptions thrown in the api and return them as a bad request
        echo json_encode( array(
            'status'=> 400,
            'params'=> $_POST,
            'data'=> array(
                'message'=> $e->getMessage(),
                'file'=> $e->getFile(),
                'line'=> $e->getLine()  
            )   
        ));
    }

My Gateway class then deals with the api call.

class Gateway {

    function __construct($params) {
        $this->params = (isset($params) ? $params : array());

        if ( !isset($params['method']) ) {
            throw new Exception('No method passed to API');
        }

        $method = explode('.', $params['method']);

        if ( !class_exists($method[0]) ) {
            throw new Exception('No class by the name of ' . $method[0]);
        }    
        if ( !method_exists($method[0], $method[1]) ) {
            throw new Exception('No method in ' . $method[0] . ' called ' . $method[1]);
        }

        //Call my class and method without instantiating it.
        $result = $method[0]::$method[1]($params);

        if (!$result) {
            //This shouldn't really fire as it should be handled in the method.
            $this->error();
        }
        else {
            $this->send(200, $result, $params);
        }

    }

    public $params; //$_POST parameters stored here to return if an error is thrown

    public function error($code = 400, $data = array(), $params = array()) {
        echo json_encode( array(
            'status'=> 400,
            'params'=> $this->$params,
            'data'=> $data
        ));
    }

    public function send($code = 200, $data = array(), $params = array()) {        
        echo json_encode( array(
            'status'=> 200,
            'params'=> $this->params,
            'data'=> $data
        ));        
    }
} 

Example of my api classes. These are currently in the same file as my Gateway class. Eventually these will be moved into a separate folder and pulled in dynamically.

class Sites {
    public function getSites($params) {
        return array(
            'gotyou'=> 'message'    
        );
    }
}
share|improve this question
add comment

2 Answers 2

up vote 3 down vote accepted

What you are really trying to accomplish here is taking http requests and dispatching them to the correct class. This is much better handled by one of many mvc frameworks in existence. That said, I see several problems with what you have here.

  1. Improper check for POST requests. $_POST is always available
  2. No validation of input. Do you want to allow unrestricted access to classes and methods?
  3. Gateway class (which seems more like a dispatcher) is concerned with too much. Not only is it dispatching the request, it is also formatting responses.
  4. No use of http header to properly instruct the client. Put the status codes (200, 404, etc) in the response header, not in the payload.
  5. You are potentially exposing sensitive information by passing back the Exception object's message. When things go wrong, YOU need to know what and why, but the client does not.

Here is a rough improvement:

GatewayExeption.php

<?php
//Use your own exception to distinguish from others
class GatewayException extends Exception {}

Gateway.php

<?php
class Gateway
{
    /**
     * @var array
     */
    private $apis;

    /**
     * Pass an array of pre-configured api class and actions
     * 
     * array(
     *   'Foo' => array('bar', 'baz')
     * )
     * 
     * @param array $apis
     */
    public function __construct($apis)
    {
        $this->apis = $apis;
    }

    /**
     * Invoke a pre-configured api
     * 
     * @param string $apiName
     * @param string $action
     * @param null|array $args
     * @throws GatewayException
     */
    public function invokeApi($apiName, $action, $args = array())
    {
        if (! array_key_exists($apiName, $this->apis)) {
            throw new GatewayException('Invalid api name');
        }

        if (! in_array($action, $this->apis[$apiName])) {
            throw new GatewayException('Invalid action');
        }

        try {
            $obj = new $apiName;
            $result = call_user_func(array($obj, $action), $args);
            return $result;
        } catch (Exception $e) {
            throw new GatewayException('api error', null, $e);
        }
    }
}

Sites.php

<?php
class Sites
{
    public function getSites($params)
    {
        return array(
            'gotyou' => 'message'
        );
    }
}

index.php

<?php
require 'GatewayException.php'
require 'Gateway.php'

if ( $_SERVER['REQUEST_METHOD'] !== 'POST') {
    header('HTTP/1.0 405 Method Not Allowed');
    exit;
}

//Provide an explicit list of available apis
$validApis = array(
    'Sites' => array('getSites')
);
$gateway = new Gateway($validApis);

//initialize request parameters with defaults
$api = array_key_exists('api', $_POST) ? $_POST['api'] : '';
$action = array_key_exists('action', $_POST) ? $_POST['action'] : '';
$params = array_key_exists('params', $_POST) ? $_POST['params'] : null;

//all responses at this point will be json
header('Content-Type: application/json');

try {
    $result = $gateway->invokeApi($api, $action, $params);
    echo json_encode($result);
} catch (GatewayException $e) {
    header('HTTP/1.0 400 Bad Request');
    echo json_encode( array(
        'status'=> $e->getMessage()
    ));
} catch (Exception $e) {
    header('HTTP/1.0 500 Internal Server Error');
    //you should log $e->getMessage()
    //you probably do not want to reveal the Exception's message
    echo json_encode( array(
        'status'=> 'A system error occurred' 
    ));
}
share|improve this answer
add comment

Some links to common MVC Routers that will do exactly what you are looking for with a much better implementation; like Rob was talking about above:

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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