5
\$\begingroup\$

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

START:

<?php 

// This page is for deleting a user record.
// This page is accessed through view_users.php.

$page_title = 'Delete a User';
include ('includes/header.html');
echo '<h1>Delete a User</h1>';

// 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 '<p class="error">This page has been accessed in error.</p>';
    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 '<p>The user has been deleted.</p>';   

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

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

} 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 '<form action="delete_user.php" method="post">
    <h3>Name: ' . $row[0] . '</h3>
    <p>Are you sure you want to delete this user?<br />
    <input type="radio" name="sure" value="Yes" /> Yes 
    <input type="radio" name="sure" value="No" checked="checked" /> No</p>
    <p><input type="submit" name="submit" value="Submit" /></p>
    <input type="hidden" name="submitted" value="TRUE" />
    <input type="hidden" name="id" value="' . $id . '" />
    </form>';

    } else { // Not a valid user ID.
        echo '<p class="error">This page has been accessed in error.</p>';
    }

} // End of the main submission conditional.

mysqli_close($dbc);

include ('includes/footer.html');
?>

DELETE: PREPARED STATEMENTS:

<?php 

// This page is for deleting a user record.
// This page is accessed through view_users.php.

$page_title = 'Delete a User';
include ('includes/header.html');
echo '<h1>Delete a User</h1>';

// 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 '<p class="error">This page has been accessed in error.</p>';
    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:
        $stmt = $dbc->prepare("DELETE FROM users WHERE user_id=$id LIMIT 1");       
        $stmt->bind_param('i', $id);
        $stmt->execute();

        if ($stmt->affected_rows == 1) { // If it ran OK.

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

        } else { // If the query did not run OK.
            echo '<p class="error">The user could not be deleted due to a system error.</p>'; // Public message.
            echo '<p>' . mysqli_connect_errno() . '<br />Query: ' . $stmt->error . '</p>'; // Debugging message.
        }

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

} else { // Show the form.

    // Retrieve the user's information:

    //$query = "SELECT last_name, first_name, FROM users WHERE user_id=$id";
    $stmt = $dbc->prepare("SELECT CONCAT(last_name, ', ', first_name) AS name FROM users WHERE user_id=$id");       
    $stmt->execute();
    $stmt->bind_result($name);
    $stmt->store_result();
    $num = $stmt->num_rows;

    if ($stmt->num_rows == 1) { // Valid user ID, show the form.

        // Get the user's information:
        $stmt->fetch(); 
//print_r($row);
        // Create the form:
        echo '<form action="delete_user.php" method="post">
    <h3>Name: ' . $name . '</h3>
    <p>Are you sure you want to delete this user?<br />
    <input type="radio" name="sure" value="Yes" /> Yes 
    <input type="radio" name="sure" value="No" checked="checked" /> No</p>
    <p><input type="submit" name="submit" value="Submit" /></p>
    <input type="hidden" name="submitted" value="TRUE" />
    <input type="hidden" name="id" value="' . $id . '" />
    </form>';

    } else { // Not a valid user ID.
        echo '<p class="error">This page has been accessed in error.</p>';
    }

} // End of the main submission conditional.

//mysqli_close($dbc);
$dbc->close();      
include ('includes/footer.html');
?>

VIEW: PREPARED STATEMENTS

<?php #bd - view_users.php #1
// This script retrieves all the records from the users table.
$page_title = 'View the Current Users';
include ('includes/header.html');
// Page header:
echo '<h1>Registered Users</h1>';
require_once ('../mysqli_connect.php'); // Connect to the db.
// Make the query:
$stmt = $dbc->prepare("SELECT  last_name, first_name, user_id, DATE_FORMAT(registration_date, '%M %d, %Y') AS dr FROM users ORDER BY registration_date ASC");       
$stmt->execute();
$stmt->bind_result($last_name,$first_name,$user_id, $dr);
$stmt->store_result();
$num = $stmt->num_rows;
if($num > 0){
    // Print how many users there are:
        echo "<p>There are currently $num registered users.</p>\n";
        // Table header.
        echo '<table align="center" cellspacing="0" cellpadding="5" width="75%">
        <tr>
            <td align="left"><b>Edit</b></td>
            <td align="left"><b>Delete</b></td>
            <td align="left"><b>Last Name</b></td>
            <td align="left"><b>First Name</b></td>
            <td align="left"><b>Date Registered</b></td>
        </tr>
        ';

    while($row = $stmt->fetch()) :
    echo '<tr>
        <td align="left"><a href="edit_user.php?id=' . $user_id . '">Edit</a></td>
        <td align="left"><a href="delete_user.php?id=' . $user_id .'">Delete</a></td>
        <td align="left">' . $last_name. '</td>
        <td align="left">' . $first_name . '</td>
        <td align="left">' . $dr . '</td>
    </tr>'; 
    endwhile;

    echo '</table>'; // Close the table.
    $stmt->free_result();


} else {



    echo '<p class="error">There are currently no registered users.</p>';

}   
$dbc->close();
include ('includes/footer.html');
?>
\$\endgroup\$
4
\$\begingroup\$

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 '<p>The user has been deleted.</p>';   
} else { // If the query did not run OK.
    if ($affectedRows === 0) {
        echo '<p>The user ID you provided could not be found.</p>';
    } else {
        echo '<p class="error">The user could not be deleted due to a system error.</p>'; // Public message.
        echo '<p>' . mysqli_connect_errno() . '<br />Query: ' . $stmt->error . '</p>'; // 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 '<tr>
    <td align="left"><a href="edit_user.php?id=' . $user_id . '">Edit</a></td>
    <td align="left"><a href="delete_user.php?id=' . $user_id .'">Delete</a></td>
    <td align="left">' . $last_name. '</td>
    <td align="left">' . $first_name . '</td>
    <td align="left">' . $dr . '</td>
</tr>';

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.

\$\endgroup\$
  • \$\begingroup\$ This is my first attempt with prepared statements. The delete still works, strange. Love this forum already, will work on your suggestions and post them. Many thanks. \$\endgroup\$ – Wasabi Feb 1 '12 at 15:38
  • \$\begingroup\$ @Wasabi Glad to be of assistance. If you decide to make some changes to your code based on suggestions please be sure to post a new question so you'll get the best chance at getting your code reviewed. I also added some more details on why your delete statement is still working. \$\endgroup\$ – Charles Sprayberry Feb 1 '12 at 15:47

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.