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:
1 Answer 1
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.
-
\$\begingroup\$ Excellent feedback! I have begun taking a look at all the terms and I'll update my code along the way \$\endgroup\$Erik– Erik2016年03月19日 11:12:54 +00:00Commented Mar 19, 2016 at 11:12