PHP: Delete specific row in MySQL

Question

I'm trying to delete a row in my mysql database. Which row to be deleted is defined by a _GET tag. So if the page URL is deletePage.php?id=5 it wil delete the row with ID=5.

Here is my code so far:

<?php
 if (empty($_GET)) {echo "Error!";}
 else {
    include "../../includes/dke390d-ko.php";
    $pageID = $_GET['id'];;
    $conn = mysql_connect($dbhost, $dbuser, $dbpasswd);
if(! $conn )
{
  die('Could not connect: ' . mysql_error());
}
$sql = 'DELETE FROM page
        WHERE id="$pageID"';

mysql_select_db($dbname);
$retval = mysql_query( $sql, $conn );
if(! $retval )
{
  die('Could not delete data: ' . mysql_error());
}
echo "Deleted data successfully\n";
mysql_close($conn);
    header("Location: index.php");
    }
 ?>

My problem is that when i execute the code i am getting a error message saying: Could not delete data: Unknown column 'id' in 'where clause'

I should probably put the ID in a session. But that'll come later.

Best regards!

Answer 1

You have a variable in a string delimited by single quotes. As a result the variable is not interpolated. Swap your quotes to fix this:

$sql = "DELETE FROM page
        WHERE id='$pageID'";

Like I said in my comments above, your code is obsolete and insecure. You should make fixing that a high priority.

Answer 2

The MySQL family of PHP is deprecated and support thereof will disappear, if it hasn't yet. Please look into PDO or MySQLi.

Now for the bigger issues, never trust user input. Your SQL is not safe against SQL injection, nor do you do any checks on the $_GET parameter. This is incredibly dangerous and reeks of disaster!

In PDO, there is a prepare statement that will sanitize input for you.

$sql = $dbh->prepare('DELETE FROM page  WHERE id=?');
$sql->execute(array($id));

The execute function takes an array and will replace any ? in your code with parameters safely.

As mentioned above, I would also do a check on that $_GET parameter; keep in mind that a user can supply anything he wants, even if your form wouldn't allow it (the same applies for $_POST).

I would suggest something like:

if(!isset($_GET['id']) || !is_scalar($_GET['id']) || !is_numeric($_GET['id']))
   die('Invalid id!');

Assuming you'd want your ID to be numeric for the check with is_numeric(). Checking for is_scalar() helps prevent warnings that appear when the user visits something like delete.php?id[]=t which is really easy to do.