1
\$\begingroup\$

This is my first PHP project - just to write to something to learn from. I made a page with form to send some quote to server and display some quotes from db.

I'm just starting with PHP so point out anything that could help me improve.

Live version.

quote-a-day.php

<?php 
    require_once $_SERVER['DOCUMENT_ROOT'] . '/logger.php';
    require_once $_SERVER['DOCUMENT_ROOT'] . '/db.php';

    $quote = $aName = "";
    $quoteErr = $nameErr = $sendErr = "";
    $success = false;

    function fail() {
        global $success;
        $success = false;

    }

    function test_input($data) {
      $data = trim($data);
      $data = stripslashes($data);
      $data = htmlspecialchars($data);
      return $data;
    }

    if ($_SERVER["REQUEST_METHOD"] === "POST") {
        $success = true;
        if (empty($_POST["quote"])) {
            $quoteErr = "Quote is required.";
            fail();
        } elseif(strlen($_POST["quote"]) > 40) {
            $quoteErr = 'Too long. Keep it under 40 characters.';
            fail();
        } else {
            $quote = test_input($_POST["quote"]);
        }
        if (empty($_POST["aName"])) {
            $nameErr = "Author's name is required.";
            fail();
        } elseif(strlen($_POST["aName"]) > 40) {
            $nameErr = 'Too long. Keep it under 40 characters.';
            fail();
        } else {
            $aName = test_input($_POST["aName"]);
        }
    }

    if ($success) { 
        try {
            $db = DB::getConn();

            // send data
            $stmt = $db->prepare("SELECT * FROM qad_author WHERE name = :name");
            $stmt->execute(array(':name' => $aName));
            if ($stmt->rowCount() === 0) {
                $stmt = $db->prepare("INSERT INTO qad_author (name) value (:name)");
                $stmt->execute(array(':name' => $aName));
                $author_id = $db->lastInsertId();
            } else {
                $author_id = $stmt->fetch(PDO::FETCH_ASSOC)['author_id'];
            }

            $stmt = $db->prepare("INSERT INTO qad_quote (quote, author) values (?, ?)");
            $stmt->execute(array($quote, $author_id));

            // Redirect
            header("Location: " . $_SERVER['REQUEST_URI']);
            exit();
        } catch (PDOException $ex) {
            Logger::alert ($ex->getMessage());
            $sendErr = 'Quote submit failed. Error logged.';
        }
    }
?>
<!DOCTYPE html>
<html>
<head>
    <title>Semicoded</title>
    <link rel="stylesheet" type="text/css" href="quote.css">
</head>
<body>
    <?php include("menu_top.php") ?>
    <div id="main">
        <div class="project">

            <h3>Submit a quote</h3> <br>
            <form action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]);?>" method="post">
                <table>
                    <tr>
                        <td>Quote:</td>
                        <td><input type="text" name="quote" maxlength="40" size="30" value="<?php echo $quote; ?>" ></td>
                        <td><span class="error"><?php echo $quoteErr; ?></span></td>
                    </tr>
                    <tr>
                        <td>Author:</td>
                        <td><input type="text" name="aName" maxlength="40" size="20" value="<?php echo $aName; ?>" ></td>
                        <td><span class="error"><?php echo $nameErr; ?></span></td>
                    </tr>
                    <tr>
                        <td colspan="2"><input type="submit"></td>
                        <td><span class="error"><?php echo $sendErr; ?></span></td>
                    </tr>
                </table>
            </form>
        </div>
        <div id="qad-out">
            <?php 
                // display quotes
                include("qad-load.php");
            ?>
        </div>
        <div class="quote">
            <a href="#" id="qad-load-more">Load more.</a>
        </div>

    </div>

<script type="text/javascript">
(function () {
   'use strict';
    var out = document.getElementById("qad-out");
    function loadMore() {
        var xmlhttp = new XMLHttpRequest();
        xmlhttp.onreadystatechange = function () {
            if (xmlhttp.readyState === XMLHttpRequest.DONE) {
                out.innerHTML = (xmlhttp.status === 200) ? xmlhttp.responseText : 
                    '<div class="quote"><span class="error">There was an error.</span></div>';
            }
        };
        xmlhttp.open("GET", "qad-load.php", true);
        xmlhttp.send();
    }
    document.getElementById("qad-load-more").onClick = loadMore;
}());
</script>
</body>
</html>

qad-load.php

<?php 
    require_once $_SERVER['DOCUMENT_ROOT'] . '/logger.php';
    require_once $_SERVER['DOCUMENT_ROOT'] . '/db.php';

    try {
        $db = DB::getConn();

        // $stmt = $db->query('SELECT * FROM qad_quote ORDER BY quote_id DESC LIMIT 10;');
        $stmt = $db->query('SELECT * FROM qad_quote ORDER BY rand() LIMIT 10;');
        $nameStmt = $db->prepare("SELECT * FROM qad_author WHERE author_id = :author_id;");
        while($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
            echo '<div class="quote">';
            $nameStmt->execute(array(":author_id" => $row["author"]));
            $rowName = $nameStmt->fetch(PDO::FETCH_ASSOC);
            echo "<q>", $row["quote"], "</q><br>";
            echo "<em>", $rowName["name"], "</em><br>";
            echo '</div>';
        }
    } catch (PDOException $ex) {
        Logger::alert ($ex->getMessage());
        echo '<br><b>Error while loading quotes.</b><br>';
    }
?>

db.php

<?php 
class DB {
    private static $conn = null;

    public static function getConn() {
        if (is_null(self::$conn)) {
            self::$conn = new PDO('mysql:host=localhost;dbname=semicode_db;charset=utf8', 
                'non-admin-user', // user
                '123456',         // pwd
                array(PDO::ATTR_EMULATE_PREPARES => false,
                    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION)
            );
        }
        return self::$conn;
    }
}
?>
\$\endgroup\$
2
\$\begingroup\$

For a first project, your code is pretty good. The structure and formatting are fine, and your code is generally quite readable. You also didn't make any really bad beginners mistakes like having SQL injection vulnerabilities, etc.

  • test_input isn't a great function. I wrote about it already a couple of times, so I'll just summarize here: It just applies some semi-random functions which mangle your data. It doesn't apply proper input filtering and is not a recommended approach to security. Right now, it does protect you from XSS, but the recommended approach is to encode variables when echoing them, not when retrieving them (it keeps your data clean, and it is secure no matter where the data comes from). If you want additional input filtering, consider a different approach.
  • don't shorten variable names, it makes code hard to read. What's aName for example? I have no idea. If you write authorName, it's immediately clear what is meant. Same for qad, xErr, etc
  • either use snake_case or camelCase. Mixing both makes code harder to read.
  • fail doesn't do anything except set success to false. It would be clearer to just do that directly.
  • some more functions could help the structure of your code and make it more readable and reusable. Examples may be insertQuote($db, $quote, $author) or insertAuthor($db, $author)
  • your HTML could use some improvements. For example, you might want to add labels for your input. You might also want to use fieldset instead of table.
\$\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.