2
\$\begingroup\$

I'm building an example REST API in PHP in my local environment to learn how they work. The starting point was an online tutorial. The example is working fine; however, I have a hunch that there is room for improvement in the logic that handles the endpoints which is stored in a script called RestController.php.

Currently, I have the example working both via a UI and, separately, using Postman. The pertinent lines in .htaccess file are:

RewriteRule ^mobile/?$ controllers/RestController.php [nc,L]
RewriteRule ^mobile/([0-9]+)/?$ controllers/RestController.php?id=$1 [nc,qsa,L]

Here is an example of a Postman test for the GET one mobile API endpoint: enter image description here

The code that follows is RestController.php. I'm wondering if there is a way to refactor this into a class but still have the ability to use Postman for testing the endpoints. If so, how would one go about doing that?

<?php    
require_once("../classes/MobileRestHandler.php");

# read raw data from request body and stuff it into $_POST
$_POST = json_decode(file_get_contents('php://input'), true);

# get parameter values according to the method
$method = $_SERVER['REQUEST_METHOD'];

# initialize and populate $parameters_* variables
if (in_array($method, ['GET', 'PUT', 'DELETE'])) {
    $id = isset($_GET["id"]) ? $_GET["id"] : "";
    $parameters_id_only = ['id' => $id];
}

if (in_array($method, ['POST', 'PUT'])) {
    $name = isset($_POST["name"]) ? $_POST["name"] : "";
    $model = isset($_POST["model"]) ? $_POST["model"] : "";
    $color = isset($_POST["color"]) ? $_POST["color"] : "";
    $parameters_no_id = ['name' => $name, 'model' => $model, 'color' => $color];
}

if ($method == 'PUT') {
    $parameters_all = array_merge($parameters_id_only, $parameters_no_id);
}

# this logic controls the RESTful services URL mapping
$mobileRestHandler = new MobileRestHandler();

if (!in_array($method, ['GET', 'POST', 'PUT', 'DELETE'])) {
    $statusCode = 404;
    $statusMessage = $mobileRestHandler->getHttpStatusMessage($statusCode);
    exit($statusCode . ' - ' . $statusMessage);
} else {
    if ($method == 'GET' && strlen($id) == 0) {     
        $result = $mobileRestHandler->getAllMobiles();                      // handles method GET + REST URL /mobile/
    } else if ($method == 'GET') {      
        $result = $mobileRestHandler->getMobile($parameters_id_only);       // handles method GET + REST URL /mobile/<id>/
    } else if ($method == 'POST') {
        $result = $mobileRestHandler->addMobile($parameters_no_id);         // handles method POST + REST URL /mobile/
    } else if ($method == 'PUT') {
        $result = $mobileRestHandler->editMobile($parameters_all);          // handles method PUT + REST URL /mobile/<id>/
    } else if ($method == 'DELETE') {
        $result = $mobileRestHandler->deleteMobile($parameters_id_only);    // handles method DELETE + REST URL /mobile/<id>/
    }

    echo $result;

}
?>
\$\endgroup\$
1
  • \$\begingroup\$ Why not learn and use GraphQL instead, as REST is simply not that great design.. \$\endgroup\$ Commented Oct 3, 2019 at 14:45

1 Answer 1

1
\$\begingroup\$

Unless I am missing something in your requirements, I don't think there is a terrific need for the overhead of a class. (I could be wrong)

I do spy some opportunities to ratchet up your code though. Consider these points and if you decide to further develop your script into a class, then this should move you in that direction with some single responsibility thinking.

  1. Don't extract data until you are sure that you will use it.

  2. Don't Repeat Yourself (DRY). Don't make the server repeat any operations that provide results that its already provided.

  3. switch() blocks are verbose, but they are appropriately used when you are performing multiple evaluations on the same variable.


require_once("../classes/MobileRestHandler.php");

function getId() {
    return ['id' => $_GET["id"] ?? ''];
}

function getPost() {
    $defaults = array_fill_keys(['name', 'model', 'color'], '');
    $posted = json_decode(file_get_contents('php://input'), true);
    return array_replace($defaults, array_intersect_key($posted, $defaults));
}

function getIdAndPost() {
    return array_merge(getId(), getPost());
}

switch ($_SERVER['REQUEST_METHOD']) {
    case 'GET':
        $result = empty($_GET['id'])
            ? $mobileRestHandler->getAllMobiles();
            : $mobileRestHandler->getMobile(getId());
        break;
    case 'POST':
        $result = $mobileRestHandler->addMobile(getPost());
        break;
    case 'PUT':
        $result = $mobileRestHandler->editMobile(getIdAndPost());
        break;
    case 'DELETE':
        $result = $mobileRestHandler->deleteMobile(getId());
        break;
    default:
        $result = '404 - ' . $mobileRestHandler->getHttpStatusMessage(404);
}

exit($result);

Admittedly, my untested script doesn't validate, sanitize, or throw any errors, but your script didn't seem to mind these relevant topics.

p.s. For the record, I don't endorse the technique of deleting a record based on $_GET data -- if a crawler would hapen to come upon your script and some valid ids, it could vanquish records during the simple act of crawling. When manipulating data stores, always use $_POST.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Much obliged for the thorough, well explained answer. I'm going to incorporate all of your recommendations and modify the record deletion so it uses an id from $_POST. \$\endgroup\$
    – knot22
    Commented Oct 5, 2019 at 2:51

Your Answer

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

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