4
\$\begingroup\$

Following up from part one, I've attempted to convert the largest part of my project into an MVC pattern as per suggestions from the accepted answer. I've managed to split the original edit.php file into a template and the business logic part, but there's still a lot of repetition going on. I feel like there has to be a better way to handle cutting down on the repetition without introducing tons of new variables and if statements all over the place.

edit.php

<?php
require_once "bootstrap.php";

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

const MAX_LENGTH_TITLE = 32;
const MAX_LENGTH_BODY = 10000;
$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();

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) > MAX_LENGTH_TITLE) {
            $errors[] = "Title too long. Please enter a title less than or equal to " . MAX_LENGTH_TITLE . " characters.";
        } elseif (slugify($title) !== $article["slug"]) {
            $errors[] = "Title may only change in capitalization or by having additional symbols added.";
        }
        if (strlen($body) > MAX_LENGTH_BODY) {
            $errors[] = "Body too long. Please enter a body less than or equal to " . MAX_LENGTH_BODY . " 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();
        }
    } elseif (!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) > MAX_LENGTH_TITLE) {
            $errors[] = "Title too long. Please enter a title less than or equal to " . MAX_LENGTH_TITLE . " 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) > MAX_LENGTH_BODY) {
            $errors[] = "Body too long. Please enter a body less than or equal to " . MAX_LENGTH_BODY . " 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();
        }
    }
}
$title = $article["title"] ?? $title;
$template = "edit.php";
require_once "templates/layout.php";

templates/edit.php

<?php if (!empty($errors)): ?>
    <ul>
    <?php foreach ($errors as $error): ?>
        <li><?= $error; ?></li>
    <?php endforeach; ?>
    </ul>
<?php endif; ?>
<?php if ($article): ?>
    <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: ?>
    <form action="/edit.php" method="post" name="form-create-article">
        <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; ?>
\$\endgroup\$
2
\$\begingroup\$
  • I recommend that you not store htmlspecialchars() values as a general practice. Of course you can if you wish, but I prefer to keep raw values in the db as a base representation of the data and then only manipulate the value when needed and in the manner required for the given task.

  • You are (conditionally) calling slugify() twice on the $_GET["title"] value; this is in breach of DRY coding practices. You should calling it once at most. If this were my project, I don't think that I would allow the call of slugify() (or the trip the database for that matter) at all if the title is empty. I don't know if the identical title-slug check is necessary to perform -- it looks like extra work. If you want to validate the title and check if it is a slug... rather than running the multiple preg_replace() calls in that custom function and comparing the return value to the input, just make a single preg_match() call to make sure it contains only valid letters and complies with your length requirements.

  • Because I believe you can rely on the existence of $_POST["title"], empty(trim($title) can be written as !trim($title) for the same effect with one less function call.

  • Again, to espouse DRY coding practices, don't call strlen($title) twice, cache the value and check the variable.

  • All string validations should pass before making any trips to the db for best performance. You are check the body length too late. You should write it before the SELECT query.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Well, I just moved the part of code, which literally duplicates in both branches, above the condition. That's all. for the rest, I don't think there is anything significant could be made in the current paradigm.

if ($_SERVER["REQUEST_METHOD"] === "POST")
{
    $title = $_POST["title"];
    $body = $_POST["body"];
    $slug = slugify($title);

    if (empty(trim($title))) {
        $errors[] = "No title. Please enter a title.";
    } elseif (strlen($title) > MAX_LENGTH_TITLE) {
        $errors[] = "Title too long. Please enter a title less than or equal to " . MAX_LENGTH_TITLE . " characters.";
    }
    if (strlen($body) > MAX_LENGTH_BODY) {
        $errors[] = "Body too long. Please enter a body less than or equal to " . MAX_LENGTH_BODY . " characters.";
    }

    if (!empty($_POST["edit-article"]))
    {
        if (slugify($title) !== $article["slug"]) {
            $errors[] = "Title may only change in capitalization or by having additional symbols added.";
        }
        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();
        }
    } elseif (!empty($_POST["create-article"])) {

        $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 (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();
        }
    }
}
$title = $article["title"] ?? $title;
$template = "edit.php";
require_once "templates/layout.php";
| improve this answer | |
\$\endgroup\$

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.