2
\$\begingroup\$

I tried to create REST API in PHP because I would like to learn the basics before I will start use frameworks to create apps. I created a Model with CRUD methods which can be used in every model to avoid repeating code. Class Model will be inherited by classes like Product, Measurement etc. I want to create diet application for android and web app. I tried to separate backend from front and mobile app but because im newbie tried to use clean PHP instead using frameworks. I wonder is that a good way because when I was looking where crud methods should be placed (in model or controller) I saw a lot of answers in both ways. My model looks like:

<?php
class Model
{
    protected $connection;
    protected $tableName;

    /**
     * Model constructor.
     * @param $connection
     */
    public function __construct($connection)
    {
        $this->connection = $connection;
    }

    public function read($id)
    {
        $sql = "SELECT * FROM `$this->tableName` WHERE id = ?";
        $stmt = $this->connection->prepare($sql);
        $stmt->execute([$id]);
        $result = $stmt->fetchAll(\PDO::FETCH_ASSOC);
        return json_encode($result);
    }

    public function create($data)
    {
        $params = [];
        $values = "";
        foreach ($data as $key => $value) {
            $params[] = $key;
            $values .= "'$value',";
        }
        $params = implode(",", $params);
        $values = rtrim($values, ",");
        $sql = "INSERT INTO $this->tableName ($params) VALUES ($values)";
        $stmt = $this->connection->prepare($sql);
        $stmt->execute();
        return json_encode("Record created");
    }

    public function readAll()
    {
        $sql = "SELECT * FROM " . $this->tableName;
        $stmt = $this->connection->prepare($sql);
        $stmt->execute();
        $result = $stmt->fetchAll(\PDO::FETCH_ASSOC);
        return json_encode($result);
    }

    public function update($data, $id)
    {
        $set = "";
        foreach ($data as $key => $value) {
            $set .= $key . '=' . '"' . $value . '",';
        }
        $set = rtrim($set, " ,");
        $sql = "UPDATE $this->tableName SET $set WHERE id = $id";
        $stmt = $this->connection->prepare($sql);
        $stmt->execute();
        return json_encode("Record updated");
    }

    public function delete($id)
    {
        $sql = "DELETE FROM `$this->tableName` WHERE id = ?";
        $stmt = $this->connection->prepare($sql);
        $stmt->execute([$id]);
        return json_encode("Record deleted");
    }
}

And then I extend Model for example Product like below:

<?php
include_once 'Model.php';
class Product extends Model
{
    protected $tableName = 'products';
}

My controllers are currently empty. I dont know where add methods to validate data. I was watching tutorials and their way to print json data was creating url like /api/product/read.php and they created all methods in that way. But I think thats wrong way because if I understood well concept of rest api we should use http methods like get/post etc instead using another urls to every method. Then I created a directory api and added file products.php. In that file created switch to handle http methods and use proper methods that are inherited from Model class and I dont need to rewrite that methods for every model.

 <?php
include_once '../../bootstrap.php';
include_once '../model/Measurement.php';
include_once '../config/DatabaseConnector.php';

header("Access-Control-Allow-Origin: *");
header("Content-Type: application/json; charset=UTF-8");
header("Access-Control-Allow-Methods: GET,POST,PUT,DELETE");
header("Access-Control-Max-Age: 3600");
header("Access-Control-Allow-Headers: Content-Type, Access-Control-Allow-Headers, Authorization, X-Requested-With");

$url = $_SERVER['REQUEST_URI'];
$urls = explode('/', $url);
$id = end($urls);

$connection = new DatabaseConnector();
$products = new Product($connection->getConnection());

$input = (array) json_decode(file_get_contents('php://input'), TRUE);

switch ($_SERVER['REQUEST_METHOD']){
    case 'GET': if ($id !== "products.php")
        print_r($products->read($id));
    else
        print_r($products->readAll());
        break;
    case 'POST': print_r($products->create($input));
        break;
    case 'PUT': print_r($products->update($input, $id));
        break;
    case 'DELETE': print_r($products->delete($id));
        break;
}

I will be extremally thankful if someone will rate my code, points out my mistakes and give me an example where to create validate methods. Thank you so much, have a good day!

\$\endgroup\$
7
  • \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title and body describe the real-world purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. Thanks! \$\endgroup\$ Feb 10 at 13:29
  • \$\begingroup\$ You probably also want to tell us a bit about the schema of the database tables you're using, too. \$\endgroup\$ Feb 10 at 13:30
  • \$\begingroup\$ @TobySpeight the idea of this code is to be database schema agnostic \$\endgroup\$ Feb 10 at 13:31
  • \$\begingroup\$ Ah, so it's a library for other code to use with different data? I saw queries including WHERE id = ?, so that imposes at least one constraint on the data schema. Anyway a library makes sense for a review; can you update the title/description to explain that? Thanks. \$\endgroup\$ Feb 10 at 13:34
  • \$\begingroup\$ Yes, I will explain what I want to achive with that code. Sorry that I didnt that at start. \$\endgroup\$ Feb 10 at 13:38
2
\$\begingroup\$

The idea is very good and the structure is correct. Do not listen to anyone who would say that a crud method should be implemented in the controller. All the controller's job is to get the input and to call the CRUD method from the model. So in this regard your code is all right.

However, your model code suffers from the opposite mix-up: it shouldn't format the output. That's the controller's job. Do not return any status from your CRUD methods. Your CRUD methods should only return a raw data when applicable and let the controller to format the output.

But you are overlooking a very serious vulnerability here - an SQL injection.

You should really learn about this matter, it's a very critical issue. Nobody cares how clean or efficient your code is if it imposes a critical threat to the whole application.

To mitigate this issue you should look into two places

  • use the real protection instead of cargo cult prepared statements
  • besides the table name, throw in a class property with alist of columns in the table, and add the field names to the SQL strictly from this list only.

For the actual implementation you can look into similar CRUD implementation I posted recently

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I created protected fields in class to handle column names but after a while I deleted that because I stated that I dont need that. Now I see that I should make that in first way. I heard about SQL injection but never tried to avoid that vulnerability. I will rewrite that to remove issues. Thank you so much, your answer was really helpful! \$\endgroup\$ Feb 10 at 13:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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