Prepared statements syntax review

Migrating to using prepared statements and would like some feedback on CRUD modules, concerned with syntax (usage and DRY) and speed:

START:

```
Delete a User';

// Check for a valid user ID, through GET or POST:
if ( (isset($_GET['id'])) && (is_numeric($_GET['id'])) ) { // From view_users.php
$id = $_GET['id'];
} elseif ( (isset($_POST['id'])) && (is_numeric($_POST['id'])) ) { // Form submission.
$id = $_POST['id'];
} else { // No valid ID, kill the script.
echo 'This page has been accessed in error.';
include ('includes/footer.html');
exit();
}

require_once ('../mysqli_connect.php');

// Check if the form has been submitted:
if (isset($_POST['submitted'])) {

if ($_POST['sure'] == 'Yes') { // Delete the record.

// Make the query:
$q = "DELETE FROM users WHERE user_id=$id LIMIT 1";
$r = @mysqli_query ($dbc, $q);
if (mysqli_affected_rows($dbc) == 1) { // If it ran OK.

// Print a message:
echo 'The user has been deleted.';

} else { // If the query did not run OK.
echo 'The user could not be deleted due to a system error.'; // Public message.
echo '' . mysqli_error($dbc) . 'Query: ' . $q . ''; // Debugging message.
}

} else { // No confirmation of deletion.
echo 'The user has NOT been deleted.';
}

} else { // Show the form.

// Retrieve the user's information:
$q = "SELECT CONCAT(last_name, ', ', first_name) FROM users WHERE user_id=$id";
$r = @mysqli_query ($dbc, $q);

if (mysqli_num_rows($r) == 1) { // Valid user ID, show the form.

// Get the user's information:
$row = mysqli_fetch_array ($r, MYSQLI_NUM);

// Create the form:
echo '
Name: ' . $row[0] . '
Are you sure you want to delete this user?
Yes
No



';

} else { // Not a valid user ID.
echo 'This page has been accessed in error.';
}

} // End of the main sub

First, I'd like to say it is good that you are swapping over to use prepared statements. This is definitely the way to go. However, you aren't using them correctly in your code. If you read the documentation on mysqli::prepare() you'll notice that the place holder for parameters should be a ? whereas you are using the actual variable you want to put in. While this statement may execute correctly it is not a prepared statement and is prone to SQL Injection. The user input is being inserted directly into the query and you are not gaining the benefit of prepared statements; but since the query is not syntactically wrong when you execute it the query goes through. However, if you check the return value of $stmt->bind_param it will likely be returning false.

You would need to change your code to fix this and prevent SQL injection attacks:

$stmt = $dbc->prepare("DELETE FROM users WHERE user_id=? LIMIT 1");

Perhaps also adding some more error checking on your database calls would help as well; for example, make sure that your $stmt->bind_param was successful before attempting to execute the query.

Second, your error checking on whether or not the query succeeded is not accurate. The query may return 0 affected rows but have no error with the actual query itself. Simply, the user_id that you gave wasn't found in the database; this isn't an error per say as nothing was actually wrong with the query. Keeping in line with your already in-place algorithm you would likely need to change it to something like:

$affectedRows = $stmt->affected_rows;
if ($affectedRows === 1) { // If it ran OK.
    echo 'The user has been deleted.';   
} else { // If the query did not run OK.
    if ($affectedRows === 0) {
        echo 'The user ID you provided could not be found.';
    } else {
        echo 'The user could not be deleted due to a system error.'; // Public message.
        echo '' . mysqli_connect_errno() . 'Query: ' . $stmt->error . ''; // Debugging message.
    }
}

Third, you're susceptible to XSS. This can be prevented by escaping your data on output in your view.

$user_id = htmlspecialchars($user_id);
$last_name = htmlspecialchars($last_name);
$first_name = htmlspecialchars($first_name);
$dr = htmlspecialchars($dr);

echo '
    Edit
    Delete
    ' . $last_name. '
    ' . $first_name . '
    ' . $dr . '
';

Lastly, I think it might ease the future maintenance if you split your different tasks up into various objects/files. An object/file to gather the data from the database and an object/file to accept that data and plug it into the view template might be a great place to start. Aim for removing any calls to the database connection in the same code that is displaying HTML.

StackExchange Code Review Q#8525, answer score: 4