-1

I have a table with four columns, and one of them holds user-entered email addresses. I'm trying to use PHP/SQL to delete an email from the "email" column, but only if it matches with what the users enter in my "removeemail" form.

Here's my code for the table:

<?php
require_once("connectvars.php"); 

$dbc = mysqli_connect(DB_HOST, DB_USER, DB_PASSWORD, DB_NAME);

$query = "CREATE TABLE email_list (
    id INT AUTO_INCREMENT,
    first_name VARCHAR(20),
    last_name VARCHAR(20),
    email VARCHAR(60),
    PRIMARY KEY (id) )";
...
?>

The remove email form:

<form method="post" action="removeemail.php">
     <label for="email">Email address:</label><br/>
     <input type="text" id="email" name="email" /><br/>
     <input type="submit" name="submit" value="Remove" />
</form>

And my php to remove the email:

<?php
require_once('connectvars.php');

$dbc = mysqli_connect(DB_HOST, DB_USER, DB_PASSWORD, DB_NAME);
$email = $_POST['email'];
$query = "DELETE FROM email_list WHERE email = $email"; 

mysqli_query($dbc, $query) or die('Error querying database.');

echo 'Customer removed: ' . $email;

mysqli_close($dbc);
?>

I keep getting the or die error for some reason every time I try to delete an email. Any help would be greatly appreciated!

strictlyhdmi
  • 153
  • 1
  • 1
  • 5
  • The same way *any* data is used in an SQL query - [*with placeholders*.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php). (Not only do placeholders prevent malicious SQL injection, but placeholders also avoid such trivial quoting issues and arguably make queries tidier.) – user2864740 Feb 27 '14 at 18:39
  • 1
    Ah, SQL Injection! http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Lkopo Feb 27 '14 at 18:40
  • @user2864740 Placeholders are not magic bullets, and it's pointless using something if you don't understand why you're using and what they're there for. – ollieread Feb 27 '14 at 18:49
  • Get some useful error information in there. `mysqli_query($dbc, $query) or die('Error querying database: '.mysqli_error());` – Sammitch Feb 27 '14 at 18:54
  • @ollieread Which is why it's a *comment*, not an answer. While your answer does clearly explain the cause of the observed error, which is why I didn't down-vote it, it *doesn't* show front and foremost the use of placeholders (or even the antiquated mysqli_real_escape_string) and is thus a *bad advice*. Note that "validation/sanitization" is *not* same thing as either placeholders or previously mentioned antiquated escaping techniques and they are orthogonal constructs: *information* is validated/sanitized while *data* is bound in placeholders or escaped. – user2864740 Feb 27 '14 at 19:05
  • If you're doing away with validation and/or sanitisation in favour of placeholders, then there is a seriously huge problem right there. Also, if you pay closer attention to my answer, you'll see that I make absolutely no reference what so ever to a single specific PHP function. That's because I'd never use it in this way, and tbh, the mysqli OO implementation is much better. The question as on the theory of how to achieve something, not the theory, plus all other related subjects. – ollieread Feb 27 '14 at 19:07
  • @ollieread If you're not encouraging the use of placeholders (or at least including an example of using an antiquated by acceptable escaping mechanism), then there is a seriously a huge problem right there. Again: Note that *"validation/sanitization" is not same thing as either placeholders* or previously mentioned antiquated escaping techniques and *they are orthogonal constructs*: *information* is validated/sanitized while *data* is bound in placeholders or escaped. (They are *both* needed, and to claim one replaces the other is wrong.) – user2864740 Feb 27 '14 at 19:08
  • I'm completely aware of the difference, and your repeating yourself serves no purpose. You imply that by not encouraging the use of placeholders, I am in fact, discouraging the use of placeholders. – ollieread Feb 27 '14 at 19:10

4 Answers4

1

email is string so you need to use quotation.

$query = "DELETE FROM email_list WHERE email = '$email'";

And you also need to use real_escape_string()

 $email = mysqli_real_escape_string($dbc,$_POST['email']);

so your full script should be like this:

require_once('connectvars.php');

$dbc = mysqli_connect(DB_HOST, DB_USER, DB_PASSWORD, DB_NAME);

$email = mysqli_real_escape_string($dbc,$_POST['email']);
$query = "DELETE FROM email_list WHERE email = '$email'"; 

mysqli_query($dbc, $query) or die('Error querying database.');

echo 'Customer removed: ' . $email;

mysqli_close($dbc);
Awlad Liton
  • 9,366
  • 2
  • 27
  • 53
  • `real_escape_string()`? :) – Lkopo Feb 27 '14 at 18:46
  • Sorry, but late with this "mistake". But too +1 – Lkopo Feb 27 '14 at 18:48
  • @user2864740 should not use input from user directly in query as it can become vulnerable to sql injection. see link for example http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – rakeshjain Feb 27 '14 at 19:07
  • @rakeshjain I agree. So I am asking, *Why* do people keep recommend using `mysqli_real_escape_string` when there are *better alternatives*? (Namely, placeholders. Time to apply a new concept instead of some antiquated cruft that is continuously perpetuated.) – user2864740 Feb 27 '14 at 19:12
1

Change

$email = $_POST['email'];
$query = "DELETE FROM email_list WHERE email = $email";

To

$email = mysqli_real_escape_string ($dbc, $_POST['email']);
$query = "DELETE FROM email_list WHERE email = '$email'";

String should be enclosed in single quote inside the query and using user input directly in query makes it vulnerable to sql injection

rakeshjain
  • 1,791
  • 11
  • 11
  • First full currect answer. – Lkopo Feb 27 '14 at 18:47
  • answer with no description? – Awlad Liton Feb 27 '14 at 18:49
  • True, true, while writing my comment, I expected an edit with explanation. – Lkopo Feb 27 '14 at 18:51
  • @Mr.Smith weren't the two changes as obvious as it can get? Not sure why the upvote was taken back – rakeshjain Feb 27 '14 at 18:55
  • Yes, but as you can see, he is a beginner and learning it by - change this to this and it's solved - is not the very good solution. He needs to know why. But I'm sorry for taking it back, it wasn't very fair from me. – Lkopo Feb 27 '14 at 18:58
  • It's a prevention to SQL Injection by escaping strings. – Lkopo Feb 27 '14 at 19:06
  • @Mr.Smith So I am asking, *Why* do people keep recommend using `mysqli_real_escape_string` when there are *better alternatives*? (Namely, [placeholders](http://stackoverflow.com/a/60496/2864740). Time to apply a new concept instead of some antiquated cruft that is continuously perpetuated.) – user2864740 Feb 27 '14 at 19:14
  • @AwladLiton Did you explain in your answer why the use of mysqli_real_escape_string is required? That might/might not be known to beginner. Enclosing string inside single quotes inside query is expected to be known to even beginners – rakeshjain Feb 27 '14 at 19:15
  • Using placeholders *should* be known to "beginners", if not, it's only because *good techniques haven't been taught*. – user2864740 Feb 27 '14 at 19:22
-1

Add apostrophes around your $email variable

$query = "DELETE FROM email_list WHERE email = '$email'";
steinmas
  • 398
  • 3
  • 9
  • If you add apostrophes into a query, it'll return an error, those are single quotes. Believe it or not, the difference is important. – ollieread Feb 27 '14 at 18:41
  • Single quotes are different from apostrophes on the keyboard? What key are you referring to then, because i'm certain they are the same key? – steinmas Feb 27 '14 at 18:43
  • I have used single-quotes everytime since I started with MySQL and believe it or not, I didn't get any error! – Lkopo Feb 27 '14 at 18:45
  • 1
    But it's the same key on the keyboard as apostrophe! I feel like i'm taking crazy pills. – steinmas Feb 27 '14 at 18:47
  • It's not, the single quote is just used in place of an apostrophe. Have you ever copied something written in Word document, surrounded by ' or "? You'll get errors as they aren't the correct characters. The key you're referring to is known as the apostrophe-quote, and switches names depending on the context. For bonus points, I didn't once say that using quotes gives you an error. – ollieread Feb 27 '14 at 19:05
  • We're not talking about copying and pasting things from word documents, we're programming in php. The key is the same. – steinmas Feb 27 '14 at 19:10
-1

Firstly, you'll want to sanitise user input, always. On top of that you'd want to do some sort of validation before passing it into the database and you may want to consider using prepared statements.

The reason you're receiving the error is because say I pass in the email address 'test@test.com', your query will read as:

DELETE FROM email_list WHERE email = test@test.com;

When you want it to read:

DELETE FROM email_list WHERE email = 'test@test.com';

In this instance, you can simply surround $email with single quotes when entering it into the query.

$query = "DELETE FROM email_list WHERE email = '$email'";
ollieread
  • 6,018
  • 1
  • 20
  • 36
  • 1
    If you have single quotes in a user-supplied email, you may have a bad day! Believe it or not, the difference between a bad day and a good day is important! - Imagine, if $email is `foo@bar.com' OR ''='` – user2864740 Feb 27 '14 at 18:43
  • 1
    I'm well aware of that, I'm not going into detail regarding the practises surrounding it. I mentioned those sufficiently above, the question was about having the contents of a variable present in a query, not 'how do I sanitise user input?', which from the fact that the op isn't aware that values need to be surrounded by single quotes, would likely be over his head at this point. – ollieread Feb 27 '14 at 18:47
  • @ollieread "I mentioned those sufficiently above". No you didn't, apostrophe is the same key as single quote. – steinmas Feb 27 '14 at 18:58
  • "Firstly, you'll want to sanitise user input, always. On top of that you'd want to do some sort of validation before passing it into the database" <-- – ollieread Feb 27 '14 at 19:04