1
\$\begingroup\$

I've put together a simple CRUD application to keep track of my library of books in an effort to learn the model-view-controller pattern, as well as better myself at PHP. Everything I have works so far, but I'm definitely looking to streamline/condense/refactor the more verbose parts of it, as well as address any possible security concerns that I may be overlooking.

In the snippets below I'm utilizing two libraries: twig/twig and bramus/router. Everything else is written by me.

index.php

<?php
/**
 * TODO: Create a proper mechanism for flash methods. Perhaps a Flash class?
 */
session_start();

require_once __DIR__ . "/vendor/autoload.php";
require_once __DIR__ . "/models.php";
require_once __DIR__ . "/controllers.php";

$loader = new \Twig\Loader\FilesystemLoader(__DIR__ . "/views");
$twig = new \Twig\Environment($loader);
$router = new \Bramus\Router\Router();

$router->get("/", function () use ($twig) {
    echo $twig->render("index.twig", ["title" => "Home"]);
});
$router->get("/books", function () use ($twig) {
    $controller = new BookController($twig);
    $controller->index();
});
$router->post("/books", function () use ($twig) {
    $controller = new BookController($twig);
    $controller->index();
});
$router->get("/books/update/(\d+)", function ($id) use ($twig) {
    $controller = new BookController($twig);
    $controller->update($id);
});
$router->post("/books/update/(\d+)", function ($id) use ($twig) {
    $controller = new BookController($twig);
    $controller->update($id);
});
$router->get("/books/delete/(\d+)", function ($id) use ($twig) {
    $controller = new BookController($twig);
    $controller->delete($id);
});
$router->post("/books/delete/(\d+)", function ($id) use ($twig) {
    $controller = new BookController($twig);
    $controller->delete($id);
});
$router->set404(function () use ($twig) {
    header("HTTP/1.1 404 Not Found");
    echo $twig->render("404.twig", ["title" => "404 Not Found"]);
});
$router->run();

function set_flash($message, $type) {
    $_SESSION["flash"] = ["message" => $message, "type" => $type];
}
function get_flash() {
    return $_SESSION["flash"] ?? null;
}
function destroy_flash() {
    unset($_SESSION["flash"]);
}

controllers.php

<?php
class BookController {
    private $model;
    private $view;

    function __construct($view) {
        $this->model = new BookModel();
        $this->view = $view;
    }
    public function index() {
        if ($_SERVER["REQUEST_METHOD"] === "POST") {
            $title = $_POST["title"];
            $status = $_POST["status"];

            if (empty($title)) {
                set_flash("Please enter a title.", "error");
            } elseif (empty($status)) {
                set_flash("Please select a status.", "error");
            } else {
                $this->model->insert($title, $status);
                set_flash("Book information added!", "success");
            }
            header("Location: /books");
            exit();
        }
        $flash = get_flash();
        destroy_flash();
        $books = $this->model->selectAll();
        echo $this->view->render("books.twig", ["title" => "Books", "books" => $books, "flash" => $flash]);
    }
    public function update($id) {
        if ($_SERVER["REQUEST_METHOD"] === "POST") {
            $title = $_POST["title"];
            $status = $_POST["status"];

            if (empty($title)) {
                set_flash("Please enter a title.", "error");
                header("Location: /books/update/" . $id);
                exit();
            } elseif (empty($status)) {
                set_flash("Please select a status.", "error");
                header("Location: /books/update/" . $id);
                exit();
            } else {
                $this->model->update($title, $status, $id);
                set_flash("Book information updated!", "success");
            }
            header("Location: /books");
            exit();
        }
        $flash = get_flash();
        destroy_flash();
        $book = $this->model->selectById($id);

        if (empty($book)) {
            set_flash("No book found with the corresponding ID to update.", "error");
            header("Location: /books");
            exit();
        }
        echo $this->view->render("books.update.twig", ["title" => "Books - " . $book["title"], "book" => $book, "flash" => $flash]);
    }
    public function delete($id) {
        if ($_SERVER["REQUEST_METHOD"] === "POST") {
            $this->model->delete($id);
            set_flash("Book successfully deleted.", "success");
            header("Location: /books");
            exit();
        }
        $flash = get_flash();
        destroy_flash();
        $book = $this->model->selectById($id);

        if (empty($book)) {
            set_flash("No book found with the corresponding ID to remove.", "error");
            header("Location: /books");
            exit();
        }
        echo $this->view->render("books.delete.twig", ["title" => "Books - " . $book["title"], "book" => $book, "flash" => $flash]);
    }
}

models.php

<?php
class Model {
    private $options = [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => false
    ];
    protected $db;

    function __construct() {
        try {
            $this->db = new PDO("pgsql:host=localhost;dbname=testdb", "postgres", "doritos1~", $this->options);
        } catch (PDOException $e) {
            throw new PDOException($e->getMessage(), (int) $e->getCode());
        }
    }
}
class BookModel extends Model {
    public function selectAll() {
        $stmt = $this->db->prepare("SELECT id, title, status FROM books ORDER BY title ASC");
        $stmt->execute();
        return $stmt->fetchAll();
    }
    public function selectById($id) {
        $stmt = $this->db->prepare("SELECT id, title, status FROM books WHERE id = ?");
        $stmt->execute([$id]);
        return $stmt->fetch();
    }
    public function insert($title, $status) {
        $stmt = $this->db->prepare("INSERT INTO books (title, status) VALUES (?, ?)");
        $stmt->execute([$title, $status]);
    }
    public function update($title, $status, $id) {
        $stmt = $this->db->prepare("UPDATE books SET title = ?, status = ? WHERE id = ?");
        $stmt->execute([$title, $status, $id]);
    }
    public function delete($id) {
        $stmt = $this->db->prepare("DELETE FROM books WHERE id = ?");
        $stmt->execute([$id]);
    }
}

books.twig

{% extends "base.twig" %}

{% block content %}
<h1>Books</h1>
{% if flash %}
<p class="alert alert-{{ flash.type }}">{{ flash.message }}</p>
{% endif %}
<form action="/books" method="post">
    <div>
        <label for="input-title">Title</label>
    </div>
    <div>
        <input type="text" name="title" id="input-title" size="32">
    </div>
    <div>
        <label>Status</label>
    </div>
    <div>
        <input type="radio" name="status" value="Unread" id="input-status-unread" checked>
        <label for="input-status-unread">Unread</label>
    </div>
    <div>
        <input type="radio" name="status" value="Reading" id="input-status-reading">
        <label for="input-status-reading">Reading</label>
    </div>
    <div>
        <input type="radio" name="status" value="Read" id="input-status-read">
        <label for="input-status-read">Read</label>
    </div>
    <div>
        <button type="submit">Add Book</button>
    </div>
</form>
{% if books|length > 0 %}
<table>
    <thead>
        <tr>
            <th>Title</th>
            <th>Status</th>
            <th>Options</th>
        </tr>
    </thead>
    <tbody>
    {% for book in books %}
        <tr>
            <td class="book-title">{{ book.title|e }}</td>
            <td>{{ book.status }}</td>
            <td><a href="/books/update/{{ book.id }}">Update</a> | <a href="/books/delete/{{ book.id }}">Delete</a></td>
        </tr>
    {% endfor %}
    </tbody>
</table>
{% else %}
<p>No books found.</p>
{% endif %}
{% endblock %}

books.update.twig

{% extends "base.twig" %}

{% block content %}
<h1>Books</h1>
{% if flash %}
<p class="alert alert-{{ flash.type }}">{{ flash.message }}</p>
{% endif %}
<form action="/books/update/{{ book.id }}" method="post">
    <div>
        <label for="input-title">Title</label>
    </div>
    <div>
        <input type="text" name="title" id="input-title" size="32" value="{{ book.title|e }}">
    </div>
    <div>
        <label>Status</label>
    </div>
    <div>
        <input type="radio" name="status" value="Unread" id="input-status-unread" {{ book.status is same as("Unread") ? "checked" : ""}}>
        <label for="input-status-unread">Unread</label>
    </div>
    <div>
        <input type="radio" name="status" value="Reading" id="input-status-reading" {{ book.status is same as("Reading") ? "checked" : ""}}>
        <label for="input-status-reading">Reading</label>
    </div>
    <div>
        <input type="radio" name="status" value="Read" id="input-status-read" {{ book.status is same as("Read") ? "checked" : ""}}>
        <label for="input-status-read">Read</label>
    </div>
    <div>
        <button type="submit">Update Book</button>
    </div>
</form>
{% endblock %}

books.remove.twig

{% extends "base.twig" %}

{% block content %}
<h1>Books</h1>
{% if flash %}
<p class="alert alert-{{ flash.type }}">{{ flash.message }}</p>
{% endif %}
<p>Do you really want to remove the book <span class="book-title">{{ book.title|e }}</span>? Click <a href="/books">here</a> to go back.</p>
<form action="/books/delete/{{ book.id }}" method="post">
    <div>
        <button type="submit" class="remove-book">Remove Book</button>
    </div>
</form>
{% endblock %}
\$\endgroup\$
2
\$\begingroup\$

That's pretty solid an implementation, I can find only a few spots.

Database connection

First and foremost, never a Model should create a database connection. All right now you have only a single model. What if you'd decide to extend it for CDs? Magazines? Add users? How many simultaneous connections will be made from a single script instance? A model should never connect on its own, it should just accept the existing connection as a constructor parameter.

It constitutes quite a problem as there is no database connection to pass into a model in the controller. To solve it, you have to adapt a Dependency Injection Container.

I am yet to learn this approach myself so I cannot provide a ready made code. But examples are plenty.

On a bright side, you could use the same container in order to supply other services for your models, such as logger service, email service, and such. And even twig, so your router won't have to create an instance of a template engine, which is quite embarrassing, if you think of it.

Duplicated code

I see a condition to test the request method is duplicated in both the router and the controller. Why? You already determined the method in the router and there is no common code in the GET and POST processing. Why not to create separate functions for them?

public function updateSave($id) {
        $title = $_POST["title"];
        $status = $_POST["status"];

        if (empty($title)) {
            set_flash("Please enter a title.", "error");
            header("Location: /books/update/" . $id);
            exit();
        } elseif (empty($status)) {
            set_flash("Please select a status.", "error");
            header("Location: /books/update/" . $id);
            exit();
        } else {
            $this->model->update($title, $status, $id);
            set_flash("Book information updated!", "success");
        }
        header("Location: /books");
        exit();
}
public function update($id) {
    $flash = get_flash();
    destroy_flash();
    $book = $this->model->selectById($id);

    if (empty($book)) {
        set_flash("No book found with the corresponding ID to update.", "error");
        header("Location: /books");
        exit();
    }
    echo $this->view->render("books.update.twig", ["title" => "Books - " . $book["title"], "book" => $book, "flash" => $flash]);
}

Better user experience.

It is considered a bad practice to feed a user with one error message at a time. Get all errors and show them at once

    $error = '';
    $error .= ($title) ? '' : "Please enter a title.\n";
    $error .= ($status) ? '' : "Please enter a status.\n";

    if ($error) {
        set_flash($error, "error");
        $location = "/books/update/$id";
    } else {
        $this->model->update($title, $status, $id);
        set_flash("Book information updated!", "success");
        $location = "/books";
    }
    header("Location: $location");
    exit();
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the advice! I've refactored my code to split things such as update() into updateGet() and updatePost() for each respective request method, as well as implementing the suggested error messaging model. I also separated the database connection from the model, supplying it instead in the use part of the router (for now). I definitely need to implement some kind of dependency injection, instead of passing around half a dozen arguments. \$\endgroup\$ – Robert Holman Mar 25 '20 at 15:04

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.