2
\$\begingroup\$

I'm learning PHP, HTML and SQL - and wondered if anyone could take a look at this code and give me some feedback?

<!DOCTYPE html>
<html>
<head>
    <link rel="stylesheet" href="styles.css">
    <title>Store product information</title>
</head>
<body>

    <?php

    $host = "localhost";
    $database = "shopDB";
    $username = "root";
    $password = "";

    $conn = new mysqli($host, $username, $password);
    $db_handle = $conn->select_db($database);

    if ($db_handle)
    {
        if ($_SERVER['REQUEST_METHOD'] == 'POST')
        {
            // Upload to server
            $name = mysqli_real_escape_string($conn, $_POST['name']);
            $quantity = mysqli_real_escape_string($conn, $_POST['quantity']);
            $price = mysqli_real_escape_string($conn, $_POST['price']);
            $description = mysqli_real_escape_string($conn, $_POST['description']);

            $insert_sql = "INSERT INTO products (name, quantity, price, description) VALUES ('$name', '$quantity', '$price', '$description')";
            $insert_result = $conn->query($insert_sql);

            if ($insert_result)
            {
                // Uploaded successfully
            } else {
                print mysqli_error($conn);
            }
        }

        $products_sql = "SELECT
                           products.name,
                           products.quantity,
                           products.price,
                           products.description
                         FROM products";
        $products_result = $conn->query($products_sql);

        if ($products_result)
        {
            print "<table id='products_table'>";

            print "<thead>";
            print "<tr>";
            print "<th>Name</td>";
            print "<th>Quantity</td>";
            print "<th>Price</td>";
            print "<th>Description</td>";
            print "</tr>";
            print "</thead>";

            print "<tbody>";
            while ($db_field = mysqli_fetch_assoc($products_result))
            {
                print "<tr>";
                print "<td>" . $db_field['name'] . "</td>";

                if ($db_field['quantity'] == '0') {
                    print "<td class='out_of_stock_cell'>" . $db_field['quantity'] . "</td>";
                } else {
                    print "<td>" . $db_field['quantity'] . "</td>";
                }

                print "<td>" . $db_field['price'] . "</td>";
                print "<td>" . $db_field['description'] . "</td>";
                print "<tr>";
            }
            print "</tbody>";
            print "</table>";
        }
    }

    ?>

    <h3 id="insert_items_header">Insert items</h3>
    <form action="index.php" method="post">
        <table>
            <thead>
                <th>Name</th>
                <th>Quantity</th>
                <th>Price</th>
                <th>Description</th>
            </thead>
            <tbody>
                <td><input name="name" type="text"></input></td>
                <td><input name="quantity" type="text"></input></td>
                <td><input name="price" type="text"></input></td>
                <td><input name="description" type="text"></input></td>
            </tbody>
        </table>
    <br>
    <input id="submit_button" type="submit" value="Register item">
    </form>
</body>
</html>

Database structure:

enter image description here

\$\endgroup\$
3
\$\begingroup\$

Security

XSS

You might want to consider HTML encoding the database values such as product name, description, price, etc.

It may or may not be needed currently, but generally, you might want more than one type of user in the future. You may have a site admin, a sales manager, data entry people who add new products, and so on.

If you do not encode these values, less privileged users could escalate their privileges via XSS.

CSRF

You also do not have any CSRF protection. This means that an attacker can add products if a logged in user with the right to add products visits an attacker-controlled website.

As you are also open to XSS, an attacker can gain XSS via adding products.

SQL Injection

Escaping is really not the preferred way to defend against SQL Injection. It's certainly a lot better than nothing, and if used correctly will be secure, but in practice, prepared statements result in better and more secure code and are really the only correct way to handle SQL Injections.

Structure

Your code is generally quite readable, but it would be even more readable if you would separate code into classes and functions. This would also make adding new functionality a lot easier

For later exercises, you might want to look into MVC. But even for a small example script, you might want to at least separate the database connection , so in case the credentials change, you do not have to change them all over the place, but just in one configuration file.

Functions would help make your code even more readable. You might consider functions such as selectProductById($connection, $id), insertProduct($connection, $name, $description, $price, $quantity), showProducts($products), showProductForm etc. You might also consider extracting those methods in a separate Product class (or a Product class for the database interactions, and a ProductView class for the displaying functions).

Misc

  • why are all your inputs of type text? Quantity and Price should be numbers, allowing a user to enter anything makes for bad usability. In practice, description should also likely be a textarea.
  • having an empty if is bad for readability. Just negate the question and only have the else case.
  • regarding usability: Think about adding at least some kind of feedback after a successful add.
\$\endgroup\$
  • \$\begingroup\$ Excellent feedback! I have begun taking a look at all the terms and I'll update my code along the way \$\endgroup\$ – Erik Mar 19 '16 at 11:12

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.