I'm receiving an error when running this code in PHP

Question

I keep receiving some variant of this error message:

Warning: PDO::exec(): SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '@email.com",5,2)' at line 1 in C:\xampp\htdocs\donations\index.php on line 31

The PHP it is referring to is this:

$db->exec("INSERT INTO donations(name, email, donation_amount, item_id) VALUES(\"" . $_POST['name'] . "\"," . $_POST['email'] . "\"," . $_POST['amount'] . "," . $_POST['radioButtons'] . ");");

Am I not escaping correctly or do I have too many quotes? Any help is appreciated!

Answer 1

You're already on a right track using PDO. Now the next step is to use it properly by utilizing prepared statements.

That being said your code might look something like this:

//TODO Check, validate, sanitize your input...
$name = $_POST['name'];
$email = $_POST['email'];
$donation_amount = $_POST['amount'];
$item_id = $_POST['radioButtons'];

try {
    $db = new PDO('mysql:host=localhost;dbname=your_db_name', 'user', 'password');
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

    //Construct your query with placeholders
    $sql = "INSERT INTO donations (name, email, donation_amount, item_id) 
            VALUES (?, ?, ?, ?, ?)";
    //Prepare your query
    $query = $db->prepare($sql);

    //Execute it passing parameters
    $query->execute(array($name, $email, $donation_amount, $item_id));

} catch (PDOException $e) {
    echo "Exception: " . $e->getMessage(); //TODO better error handling
}
$query = null;
$db = null;

Further reading:

• PDO tag wiki
• A PDO tutorial
• Are PDO prepared statements sufficient to prevent SQL injection?

Answer 2

Your problem is actually a problem with escaping quotes. If you would have used more standard single quotes for enclosing values in SQL statement you probably would have noticed this more easily, but you do not currently have an opening quote before your email value.

I would highly suggest use of prepared statements like this:

$query = 'INSERT INTO donations (name, email, donation_amount, item_id) VALUES (:name, :email, :amount, :radioButtons)';
$sth = $db->prepare($query);
$sth->execute(array(
    ':name' => $_POST['name'],
    ':email' => $_POST['email'],
    ':amount' => $_POST['amount'],
    ':radioButtons' => $_POST['radioButtons']
));

Of course this doesn't should proper error handling that you would also want to put in place along the way.

This prepared statement will protect you against SQL injection, and also has the benefit of making you SQL much more readable by eliminating the need for quotes.

I actually prefer to use the more verbose method of binding all the parameters rather than passing an array of values to execute. This allows you to specify the input type explicitly (i.e. integer, string, etc.). So based on the assumption that the last two values are integers taht might look like this:

$query = 'INSERT INTO donations (name, email, donation_amount, item_id) VALUES (:name, :email, :amount, :radioButtons)';
$sth = $db->prepare($query);
$sth->bindParam(':name', $_POST['name'], PDO::PARAM_STR);
$sth->bindParam(':email', $_POST['email'], PDO::PARAM_STR);
$sth->bindParam(':amount', $_POST['amount'], PDO::PARAM_INT);
$sth->bindParam(':radioButtons', $_POST['radioButtons'], PDO::PARAM_INT);
$sth->execute();

I didn't write it this way initially, as I think that, for whatever reason, the PHP community largely gravitates towards passing the value via array to execute(). They also more commonly tend to use ? placeholders rather than named placeholders, but, to me, this is just being lazy. I mean are you really saving that much time in writing a few extra characters to sacrifice clarity of the code?

Answer 3

Add spaces between the field/table names and parantheses

INSERT INTO donations (name...) VALUES (...)

Also, use single quotes ' ' around values.

VALUES ( '" . $_POST['name'] . "' )

Also, never inject $POST data directly into your SQL queries. This is begging for SQL Injection. And by all means, go learn PDO/Prepared Statements and stop using mysql functionality. Its dead.