MySQL Delete row with PHP

Question

I'm having a problem in deleting a row from one of my table in my mySQL DB. Below you can see the code I am using (This is the php file, which is calling the SQL query from a form):

                   <?php 
                  echo '
<form style="padding-left:30px; padding-bottom:40px; padding-top:10px; clear:both;" action="deletingcontactgroups.php">
<label><strong>Please Select A Group to Delete!</strong></label>
<select name="dropdown" style="float:left;">
<option value="">Select a Contact Group:</option>';

$con=mysqli_connect("localhost","username","password","my_db");
// Check connection
if (mysqli_connect_errno()) {
  echo "Failed to connect to MySQL: " . mysqli_connect_error();
}
$result = mysqli_query($con,"SELECT * FROM ContactsGroup");


while($row = mysqli_fetch_array($result)) {
  echo '<option value="' . $row["GroupName"] . '">' . $row["GroupName"] . '</option>';
  echo "<br>";
}
echo '<input type="submit" style="clear:both; float:left; margin-left: 300px;">';
echo '</form>';

mysqli_close($con);

                   ?>

This is the data in the file deletingcontactgroups.php :

<?php
$con=mysqli_connect("localhost","username","password","my_db");
// Check connection
if (mysqli_connect_errno()) {
  echo "Failed to connect to MySQL: " . mysqli_connect_error();
}
$groupname = mysqli_real_escape_string($con, $_POST['dropdown']);
$sql = "DELETE FROM `bulletproofaccounting`.`ContactsGroup` WHERE `ContactsGroup`.`GroupName` = '" . $groupname . "' LIMIT 1;";
mysqli_query($sql);

echo "1 record has been deleted successully!!!";

mysqli_close($con);
?>

I looked into various tutorials, but nothing is helping. Any help would be highly appreciated!

EDIT!!! I'm extremely sorry for the typo, but the quotation marks were actually placed during concatenation of the variable $sql. Sorry for the trouble.

should it not be `"DELETE FROM bulletproofaccounting.ContactsGroup WHERE ContactsGroup.GroupName = ' " . $groupname . " ' LIMIT 1;";` ??? eg the `$groupname` is never concatenated? — davidkonrad, Jul 12 '14 at 13:44
If you would use a prepared statement with a placeholder instead of your string concatenation, you wouldn't be prone to sql injection and your error would vanish too. — VMai, Jul 12 '14 at 13:48

score 2 · Answer 1 · answered Jul 12 '14 at 13:52

Change your delete statement to this

  $sql = sprintf("delete from contactsgroup where groupname = '%s'", $groupname);

Aside, the above is very bad code as it is open to SQL injection attack. You should never string concatenate SQL commands. Use parameterized queries instead:

$stmt = $con->prepare('delete from contactsgroup where groupname = ?');
$stmt->bind_param('s', $groupname);
$stmt->execute();

score 1 · Answer 2 · answered Jul 12 '14 at 13:45

You open this string in a way (") and you close this in another way (')

  "DELETE FROM `bulletproofaccounting`.`ContactsGroup` WHERE `ContactsGroup`.`GroupName` = '

try to change from this:

"DELETE FROM `bulletproofaccounting`.`ContactsGroup` WHERE `ContactsGroup`.`GroupName` = "

score 1 · Answer 3 · edited May 23 '17 at 12:29

First, you should consider a prepared statement for this one. Why? Safety reasons.

Second of all you should use some debugging on the queries to see where is the actual problem.

Here is a example of debug, prepared statement for tests:

if($stmt=$con->prepare("DELETE FROM bulletproofaccounting.ContactsGroup WHERE ContactsGroup.GroupName = ? LIMIT 1;")){
    if (!$stmt->bind_param("s", $groupname)) {
        echo "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
    }
    if (!$stmt->execute()) {
        echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
    }
    $stmt->close();
}else{
    echo "Prepare failed: (" . $con->errno . ") " . $con->error;
}

And here you can find a detailed explanation as to how to evade sql injections.

EDIT:

As pointed out in the comments, it's questionable if the solution is complete. Let me explain:

the query was:

DELETE FROM `bulletproofaccounting`.`ContactsGroup` WHERE `ContactsGroup`.`GroupName` = ' . $groupname . ' LIMIT 1;

and became:

DELETE FROM bulletproofaccounting.ContactsGroup WHERE ContactsGroup.GroupName = ? LIMIT 1;

as many people pointed out, the problem is in the quotes, prior to adding the $groupname variable, which is actually completely evaded with the prepared statement since the '?' substitutes the proper syntax for adding a variable inside the query, and thus it fixes the incorrect query syntax, written inside the question.

Nice, but it does not answer the question. It only express your opinion regarding OP's style of coding. — davidkonrad, Jul 12 '14 at 13:53
Actually it does, since it alters the query. And if it doesn't, then it will point out where is the problem. I'll edit the post to explain. — Hristo Valkanov, Jul 12 '14 at 13:56

score 0 · Accepted Answer · answered Jul 13 '14 at 00:14

The answer to the question was that I made a stupid mistake. I completely forgot to add a method to the form, which was why the variable was not getting any values.

However, I would like to thank Hristo Valkanov & itsben for the SQLi advice. Since I am not that great at coding and still learning, it's great advice, and something I will be using in the future.

MySQL Delete row with PHP

4 Answers4