The code is vulnerable to
SQL injection. Switch to
PDO and
prepared statements. Prepared statements will also be more efficient, since you're repeatedly executing the same query with different values. If you want a tutorial, try "
Writing MySQL Scripts with PHP and PDO".
Switching from assembled statements to prepared statements also makes the query more readable, since the SQL statement isn't scattered in pieces all around the PHP code. The query you're trying to execute (when assembled) is:
Code:
INSERT INTO store (id,name,price,quantity)
VALUES ((${db['id']},${db['name']},${_POST["price[${db['id']}]"]},$_POST["quantity[${db['id']}]"]),)
Not only does the final comma cause problems, so do the extra parentheses, lack of quotes around the values and improper array indexes. Since the form inputs are named with
array syntax (e.g. "
price[$db[id]]"; the HTML elements should also have double quotes around the attribute values), certain $_POST elements are arrays and should be accessed as such (e.g. "
$_POST[price][$db['id']]").
Since the values you're inserting include what is presumably a unique column, your code should handle duplicates with the
ON DUPLICATE KEY UPDATE clause.
The
SELECT in update_action.php is an unnecessary use of resources. Instead, include hidden (or read-only) fields in the form.
Unless the "store" table is supposed to replace the "products" table, repeating data in each is premature
denormalization. You'll have to do extra work to keep the two tables synchronized. You could have a "product" table (
product (id, name, description)) storing information about each product and an "inventory" table (
inventory (product_id, co, price, quantity)) storing what each company has in stock, and how much it charges.
Instead of using
SELECT *, select only the fields you need.
Outputting DB error messages is all right for quick-and-dirty debugging, but you should never do it in production code anyplace that a non-administrator can access, as it
discloses too much information. Be warned that scaffolding (debugging code) has a tendency to be left in place longer than it needs to and might show up in production code; it's safest to avoid quick-and-dirty techniques and log the error where only an admin can read it.
In some library script, part of a display module:
PHP:
<?php
function echoProductField($product, $field, $type="text") {
?><input name="product[<?php echo $product['id'], '][', $field; ?>]" value="<?php echo $product[$field]?>" type="?<php echo $type; ?>"/>
<?php
switch ($type) {
case 'radio':
case 'checkbox':
case 'hidden':
<label for="product[<?php echo $product['id'], '][', $field; ?>]"><?php echo $product[$field]; ?></label>
<?php
break;
default:
break;
}
}
Form page:
PHP:
<?php
try {
$getCoProducts = $db->prepare("SELECT id, name, company AS co FROM product WHERE company = ?")
$products = $db->execute(array('abc'));
?>
<form method="post" action="update_action.php">
<table>
<tr>
<th>ID</th><th>Name</th><th>Company</th><th>Price</td><th>Quantity</th>
</tr>
<?php foreach ($products as $product): ?>
<tr>
<td><?php echoProductField($product, 'id', 'hidden'); ?></td>
<td><?php echoProductField($product, 'name', 'hidden'); ?></td>
<td><?php echoProductField($product, 'co', 'hidden'); ?></td>
<td><input name="product[<?php echo $product['id']; ?>][price]" value="" /></td>
<td><input name="product[<?php echo $product['id']; ?>][quantity]" value="" /></td>
</tr>
<?php endforeach; ?>
</table>
<input type="submit" />
</form>
} catch (PDOException $error) {
error_log($error);
?>
<div class="error">There was an internal error. It's been logged, and we'll look into it. Please give us some time to fix it and try again later.</div>
<?php
}
update_action.php:
PHP:
<?php
$fields = array(
'id' => array('type' => 'hidden', 'label' => 'ID'),
'name' => array('type' => 'hidden', 'label' => 'Name'),
'co' => array('type' => 'hidden', 'label' => 'Company'),
'price' => array('type' => 'text', 'label' => 'Price'),
'quantity' => array('type' => 'text', 'label' => 'Quantity'),
);
try {
$insert = $db->prepare('INSERT INTO inventory (`id`, `company`, `price`, `quantity`)
VALUES (:id, :co, :price, :quantity)
ON DUPLICATE KEY UPDATE `company`=:co, `price`=:price, `quantity`=:quantity');
$failed = array();
foreach ($_POST['product'] as $product) {
$productData = array();
foreach ($product as $key => $val) {
$productData[':' . $key] = $val;
}
if (False === $insert->execute($productData)) {
$failed[] = $product;
}
}
} catch (PDOException $error) {
error_log($error);
?><div class="error">There was an internal error. It's been logged, and we'll look into it. Please give us some time to fix it and try again later.</div><?php
}
if ($failed) {
// include information about what user can do to correct the failure.
?><p>Insertion of the following products failed:</p>
<form action="<?php echo $_SERVER['REQUEST_URI']; ?>" method="POST">
<table>
<tr>
<?php foreach ($fields as $field): ?>
<td><?php echo $field['label']; ?></td>
<?php endforeach; ?>
</tr>
<?php foreach ($failed as $product):
?>
<tr>
<?php foreach ($fields as $field => $info): ?>
<td><?php echoProductField($product, $field, $info['type']); ?></td>
<?php endforeach; ?>
</tr>
<?php endforeach; ?>
</table>
<input type="submit" />
</form>
<?php
}
Many additional tasks need to be added to the sample, such as input validation (hidden fields are cosmetic, rather than providing security).
Database access, the display logic and the
domain logic should be separated into different modules, but that can be taken care of later. To ease this, you can create a class to hold inventory information for a product and a company, and replace
echoProductField with a
view class that can display form inputs.
