1
\$\begingroup\$

I'm attempting to create a basic wiki for personal use. I've got a pretty good start, but the code is beginning to become cumbersome to modify, and some of the things I'm doing feel very fragile. I've heard and looked into using the MVC pattern, but I'm having trouble understanding how I would even implement my current project using that pattern. If at all I'd like to avoid suggestions such as "use a framework," as my goal is to learn how to do things by hand.

Highlights include:

  • Trying to cut down on edit.php. There's two forms that are pretty much exactly the same, but call slightly different database queries.
  • Making sure my routing is solid with the /wiki.php?title=article-name format (although I'd love to figure out how to properly implement a front controller).
  • I'm pretty sure I don't have any error-checking going on like I should.

index.php

<?php
session_start();
require_once "database.php";
/**
 * TODO: Basic user authentication.
 * TODO: Categories for articles.
 * TODO: Articles broken down into sections that can be edited independently?
 * TODO: Make links to articles that don't exist a different color?
 */
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Webdev</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php if (isset($_SESSION["message"])): ?>
    <p><?= htmlspecialchars($_SESSION["message"]); ?></p>
<?php endif; ?>
<?php unset($_SESSION["message"]); ?>
    <div><a href="/edit.php">[Create Article]</a> <a href="/documentation.php">[Documentation]</a></div>
<?php
$stmt = $pdo->query("SELECT title, slug FROM articles");
$articles = $stmt->fetchAll();
?>
<?php if ($articles): ?>
    <ul>
    <?php foreach ($articles as $article): ?>
        <li><a href="/wiki.php?title=<?= $article["slug"]; ?>"><?= htmlspecialchars($article["title"]); ?></a></li>
    <?php endforeach; ?>
    </ul>
<?php endif; ?>
    <script src="main.js"></script>
</body>
</html>

wiki.php

<?php
session_start();
require_once "vendor/autoload.php";
require_once "database.php";

function slugify($str) {
    $str = strtolower($str);
    $str = trim($str);
    $str = preg_replace("/[^A-Za-z0-9 -]/", "", $str);
    $str = preg_replace("/\s+/", " ", $str);
    $str = str_replace(" ", "-", $str);

    return $str;
}
$ParsedownExtra = new ParsedownExtra();
$ParsedownExtra->setSafeMode(true);
$title = htmlspecialchars($_GET["title"]);
$slug = "";

if ($title !== slugify($title)) {
    $slug = slugify($title);
} else {
    $slug = $title;
}
$stmt = $pdo->prepare("SELECT id, title, slug, body FROM articles WHERE slug = ?");
$stmt->execute([$slug]);
$article = $stmt->fetch();
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Webdev | Wiki - <?= $article["title"] ?? "Article"; ?></title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php if (isset($_SESSION["message"])): ?>
    <p><?= htmlspecialchars($_SESSION["message"]); ?></p>
<?php endif; ?>
<?php unset($_SESSION["message"]); ?>
<?php if ($article): ?>
    <div><a href="/">[Home]</a> <a href="/edit.php?title=<?= $article["slug"]; ?>">[Edit Article]</a> <a href="/documentation.php">[Documentation]</a></div>
    <?= $ParsedownExtra->text($article["body"]); ?>
<?php else: ?>
    <div><a href="/">[Home]</a></div>
    <p>Unknown article ID. <a href="/edit.php?title=<?= $title; ?>">[Create Article]</a></p>
<?php endif; ?>
    <script src="main.js"></script>
</body>
</html>

edit.php

<?php
session_start();
require_once "database.php";

function slugify($str) {
    $str = strtolower($str);
    $str = trim($str);
    $str = preg_replace("/[^A-Za-z0-9 -]/", "", $str);
    $str = preg_replace("/\s+/", " ", $str);
    $str = str_replace(" ", "-", $str);

    return $str;
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Webdev | Edit</title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php if (!empty($_SESSION["message"])): ?>
    <p><?= htmlspecialchars($_SESSION["message"]); ?></p>
<?php endif; ?>
<?php unset($_SESSION["message"]); ?>
    <div><a href="/">[Home]</a> <a href="/documentation.php">[Documentation]</a></div>
<?php
$errors = [];
$title = htmlspecialchars($_GET["title"] ?? "");
$slug = "";

if ($title !== slugify($title)) {
    $slug = slugify($title);
} else {
    $slug = $title;
}
$stmt = $pdo->prepare("SELECT id, title, slug, body FROM articles WHERE slug = ?");
$stmt->execute([$slug]);
$article = $stmt->fetch();
?>
<?php if ($article): ?>
<?php
if ($_SERVER["REQUEST_METHOD"] === "POST") {
    if (!empty($_POST["edit-article"])) {
        $title = $_POST["title"];
        $body = $_POST["body"];
        $slug = slugify($title);

        if (empty(trim($title))) {
            $errors[] = "No title. Please enter a title.";
        } elseif (strlen($title) > 32) {
            $errors[] = "Title too long. Please enter a title less than or equal to 32 characters.";
        } elseif (slugify($title) !== $article["slug"]) {
            $errors[] = "Title may only change in capitalization or by having additional symbols added.";
        }
        if (strlen($body) > 10000) {
            $errors[] = "Body too long. Please enter a body less than or equal to 10,000 characters.";
        }
        if (empty($errors)) {
            $stmt = $pdo->prepare("UPDATE articles SET title = ?, body = ? WHERE id = ?");
            $stmt->execute([$title, $body, $article["id"]]);
            $_SESSION["message"] = "Article successfully updated.";
            header("Location: /wiki.php?title=" . $article["slug"]);
            exit();
        }
    }
}
?>
<?php if (!empty($errors)): ?>
    <ul>
    <?php foreach ($errors as $error): ?>
        <li><?= $error; ?></li>
    <?php endforeach; ?>
    </ul>
<?php endif; ?>
    <form action="/edit.php?title=<?= $article["title"]; ?>" method="post" name="form-edit-article">
        <div><label for="title">Title</label></div>
        <div><input type="text" name="title" id="title" size="40" value="<?= htmlspecialchars($article["title"]); ?>" required></div>
        <div><label for="body">Body</label></div>
        <div><textarea name="body" id="body" rows="30" cols="120" maxlength="10000"><?= htmlspecialchars($article["body"]); ?></textarea></div>
        <div><span id="character-counter"></span></div>
        <div><input type="submit" name="edit-article" value="Edit Article"></div>
    </form>
<?php else: ?>
<?php
if ($_SERVER["REQUEST_METHOD"] === "POST") {
    if (!empty($_POST["create-article"])) {
        $title = $_POST["title"];
        $body = $_POST["body"];
        $slug = slugify($title);

        if (empty(trim($title))) {
            $errors[] = "No title. Please enter a title.";
        } elseif (strlen($title) > 32) {
            $errors[] = "Title too long. Please enter a title less than or equal to 32 characters.";
        }
        $stmt = $pdo->prepare("SELECT title, slug FROM articles WHERE title = ? OR slug = ?");
        $stmt->execute([$title, $slug]);
        $article_exists = $stmt->fetch();

        if ($article_exists) {
            $errors[] = "An article by that title already exists. Please choose a different title.";
        }
        if (strlen($body) > 10000) {
            $errors[] = "Body too long. Please enter a body less than or equal to 10,000 characters.";
        }
        if (empty($errors)) {
            $stmt = $pdo->prepare("INSERT INTO articles (title, slug, body) VALUES (?, ?, ?)");
            $stmt->execute([$title, $slug, $body]);
            $_SESSION["message"] = "Article successfully created.";
            header("Location: /wiki.php?title=" . $slug);
            exit();
        }
    }
}
?>
<?php if (!empty($errors)): ?>
    <ul>
    <?php foreach ($errors as $error): ?>
        <li><?= $error; ?></li>
    <?php endforeach; ?>
    </ul>
<?php endif; ?>
    <form action="/edit.php" method="post" name="create-article-form">
        <div><label for="title">Title</label></div>
        <div><input type="text" name="title" id="title" size="40" value="<?= htmlspecialchars($title); ?>" required></div>
        <div><label for="body">Body</label></div>
        <div><textarea name="body" id="body" rows="30" cols="120" maxlength="10000"><?= htmlspecialchars($_POST["body"] ?? ""); ?></textarea></div>
        <div><span id="character-counter"></span></div>
        <div><input type="submit" name="create-article" value="Create Article"></div>
    </form>
<?php endif; ?>
    <script src="main.js"></script>
</body>
</html>

database.php

<?php
try {
    // TODO: Move database to a directory above the root directory.
    $pdo = new PDO("sqlite:wiki.db");
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $pdo->exec("CREATE TABLE IF NOT EXISTS articles (
        id INTEGER PRIMARY KEY,
        title TEXT,
        slug TEXT,
        body TEXT
    )");
    $pdo->exec("CREATE TABLE IF NOT EXISTS categories (
        id INTEGER PRIMARY KEY,
        name TEXT
    )");
} catch (PDOException $e) {
    $error_message = date("Y-m-d G:i:s") . " ERROR: " . $e->getMessage() . "\n";
    file_put_contents("pdoerrors.txt", $error_message, FILE_APPEND);
    $pdo = null;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I am sure you can figure out how to deduplicate edit.php. To decide which database query to run you need just a single if statement, don't you? \$\endgroup\$ Commented May 14, 2019 at 15:50
  • 1
    \$\begingroup\$ @Robert Why do you have A-Z in your negated character class if you have already called strtolower() on the input string? \$\endgroup\$ Commented May 14, 2019 at 22:52
  • \$\begingroup\$ @mickmackusa Didn't even think about that, haha. Thanks for pointing that out! \$\endgroup\$ Commented May 15, 2019 at 14:08

1 Answer 1

2
\$\begingroup\$

I would say your code is pretty good for a beginner.

Of course in time it could grow up, along with your own experience but for the current level it is mostly ok.
There are mostly minor improvements

bootstrap.php

You may notice that a certain block of code is repeated in every file. Once you see a repetition it's signal for refactoring. Here it's simple one - just put this code in a distinct file and then include it in every other file

<?php
session_start();
require_once "vendor/autoload.php";
require_once "database.php";

function slugify($str) {
    $str = strtolower($str);
    $str = trim($str);
    $str = preg_replace("/[^A-Za-z0-9 -]/", "", $str);
    $str = preg_replace("/\s+/", " ", $str);
    $str = str_replace(" ", "-", $str);

    return $str;
}

But it is very important not to add any HTML in this file. HTML is a completely different thing, some of your files aren't supposed to display anything, so HTML is never a part of a bootstrap file.

template/design.php

However, it is still a very good idea to put the site design into a distinct file, to again avoid duplication. Imagine you will decide to add some rich design - then you will have to edit every file. It won't do. What I would propose is to implement a very simple template system. Let's create a folder called template and put there all files responsible for showing HTML to the user. the first one would be design.php to hold the global design of your site, that you would include in your scripts.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Webdev | Wiki - <?= $title  ?></title>
    <link href="style.css" rel="stylesheet">
</head>
<body>
<?php if($message): ?>
    <p><?= htmlspecialchars($message) ?></p>
<?php endif; ?>

<?php include __DIR__."/".$template ?>

<script src="main.js"></script>

</body>
</html>

as you can see, it contains two variables that must be defined before including this file.

index.php

Now let's refactor your index.php splitting it into two files, the business logic part and the presentation logic part

<?php
include 'bootstrap.php';

$message = $_SESSION["message"] ?? '';
unset($_SESSION["message"])

$articles = $pdo->query("SELECT title, slug FROM articles")->fetchAll();

$title = "Articles";
$template = "index.php";
include 'template/design.php';

here we are preparing all the necessary data, so we won't litter the template with the business logic.

template/index.php

here we have all the output required for the index page

    <div>
         <a href="/edit.php">[Create Article]</a> 
         <a href="/documentation.php">[Documentation]</a>
    </div>
<?php if ($articles): ?>
    <ul>
    <?php foreach ($articles as $article): ?>
        <li><a href="/wiki.php?title=<?= $article["slug"]; ?>">
                <?= htmlspecialchars($article["title"]); ?>
            </a>
        </li>
    <?php endforeach; ?>
    </ul>
<?php endif; ?>

as you can see, both files became much more tidy.

So you can refactor all other files the same way.

wiki.php

<?php
include 'bootstrap.php';

$ParsedownExtra = new ParsedownExtra();
$ParsedownExtra->setSafeMode(true);
$title = htmlspecialchars($_GET["title"]);

if ($title !== slugify($title)) {
    $slug = slugify($title);
} else {
    $slug = $title;
}
$stmt = $pdo->prepare("SELECT id, title, slug, body FROM articles WHERE slug = ?");
$stmt->execute([$slug]);
$article = $stmt->fetch();

$title = $article["title"];
$template = "wiki.php";
include 'template/design.php';

template/wiki.php

<?php if ($article): ?>
    <div>
        <a href="/">[Home]</a> 
        <a href="/edit.php?title=<?= $article["slug"]; ?>">[Edit Article]</a>
        <a href="/documentation.php">[Documentation]</a>
    </div>
    <?= $ParsedownExtra->text($article["body"]); ?>
<?php else: ?>
    <div><a href="/">[Home]</a></div>
    <p>Unknown article ID. 
        <a href="/edit.php?title=<?= $title; ?>">[Create Article]</a>
    </p>
<?php endif; ?>

again, as you can see, separating the pure PHP from HTML mixed with PHP for the output makes both files much cleaner.

Sorry for not covering your edit.php, it's a distinct review of it's own and I have my own work to do. I am sure someone else will show you this. Or you can try it yourself and then post as another question for the review.

One last part.

database.php

You did it almost right, it just makes no sense to have a distinct error log for PDO connection errors. All errors should go into a single error log and it's none of database.php's business to decide which one. Just make PDO throw the error the safe way (as not to reveal the database credentials), as it shown in my article on how to properly connect with PDO:

$options = [
    \PDO::ATTR_ERRMODE            => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES   => false,
];
try {
    // TODO: Move database to a directory above the root directory.
    $pdo = new PDO("sqlite:wiki.db", null, null, $options);
} catch (\PDOException $e) {
    throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

$pdo->exec("CREATE TABLE IF NOT EXISTS articles (...

and that's all you need to do regarding error reporting in this file. The rest should be configured elsewhere, as it's explained in detail in my article on PHP error reporting.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much for this write-up; it's been very enlightening! A few questions: Would it make sense to also include the $message handling in bootstrap.php as well? I use it across multiple pages, and it looks like a case of repeating code. The part of setting the article title, template, and rendering the base layout seems like another instance where I could reduce it to, say, a function. Would this be a good route? In database.php, is there any significance to doing throw new \PDOException... vs. throw new PDOException? I'm confused as to what the back-slash is for. \$\endgroup\$ Commented May 15, 2019 at 13:29
  • 1
    \$\begingroup\$ Good questions. Not sure about $massage. It is a little bit specific. At least name it less generic. Regarding thetemplate, better already look towards Twig. And a backslash is a namespace issue. You will learn it in time, but with \ it's just universal, works both with or without namespaces and without a baskslash it would fail with namespaces, so we have it just in case \$\endgroup\$ Commented May 15, 2019 at 13:43
  • \$\begingroup\$ Ah, gotcha on the namespace thing. $message (and in turn, $_SESSION["message"]) is what I believe is called a flash message. It's used to let users know when something succeeds after a POST request. \$\endgroup\$ Commented May 15, 2019 at 13:59

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.