PDO prepared statement, correctly used?

Question

I just to need make sure I've got the PDO prepare statements correctly, will the following code be secured by SQL Injection?

$data['username'] = $username;
$data['password'] = $password;
$data['salt'] = $this->generate_salt();
$data['email'] = $email;

$sth = $this->db->prepare("INSERT INTO `user` (username, password, salt, email, created) VALUES (:username, :password, :salt, :email, NOW())");  
$sth->execute($data);

Seems ok. You're protected against SQL injection this way. This question more something for codereview then stackoverflow. — Arend, May 06 '12 at 16:02
Don't the keys require colon prefixes? As in `$data[':username'] = $username;` See example 2 at [PDOStatement::execute()](http://www.php.net/manual/en/pdostatement.execute.php) — Bob Stein, May 30 '13 at 23:28

Florian Margaine · Accepted Answer · 2012-05-06T16:14:41.923

7

Yes, your code is safe. It can be shortened however:

$data = array( $username, $password, $this->generate_salt(), $email );

// If you don't want to do anything with the returned value:
$this->db->prepare("
    INSERT INTO `user` (username, password, salt, email, created)
    VALUES (?, ?, ?, ?, NOW())
")->execute($data);

edited May 06 '12 at 16:14

answered May 06 '12 at 16:09

Florian Margaine

58,730
15
91
116

If you want it even shorter you could chain the execute in it like `$sth = $this->db->prepare("INSERT INTO user (username,password,salt,email,created) VALUES (?, ?, ?, ?, NOW()")->execute($data);` :) – Robert Heine May 06 '12 at 16:11
Many thanks. Actually I always tend to chain much code to make it short. :) – Robert Heine May 06 '12 at 16:18
2

Named variables are fine! Why would you want to get rid of them? Beyond trivial statements, I feel they are very helpful when debugging, and also allow you to use the named parameter more than once, if needed in the query. – Brad May 06 '12 at 16:22
1

This was just to show this was possible. Each has their pros and cons :) – Florian Margaine May 06 '12 at 16:25
Even shorter! `$this->db->prepare(" INSERT INTO user (username, password, salt, email, created) VALUES (?, ?, ?, ?, NOW()) ")->execute(array($username, $password, $this->generate_salt(), $email));` – Gavin Jan 16 '14 at 09:06

Robert Heine · Answer 2 · 2012-05-06T16:16:48.543

1

You could start with an empty array for your $data like

// start with an fresh array for data
$data = array();

// imagine your code here

Your code looks good so far.

EDIT: I missed your NOW() call. Imho you should add it with a bind variable as well, like

// bind date
$data['created'] = date("Y-m-d H:i:s");

// updated prepare statement
$sth = $this->db->prepare("INSERT INTO `user` (username, password, salt, email, created) VALUES (:username, :password, :salt, :email, :created)");

edited May 06 '12 at 16:16

answered May 06 '12 at 16:06

Robert Heine

1,820
4
29
61

2

NOW() is not a PHP function so this would throw an error. If you really wanted to use the timestamp of the PHP server instead of the database server you should use `date("Y-m-d H:i:s")` http://stackoverflow.com/questions/1995562/now-function-in-php – shanethehat May 06 '12 at 16:12
This doesn't answer the *why*, though. I'd be interested in knowing why you want to do that instead of trusting your DBMS. – Florian Margaine May 06 '12 at 16:19
2

The only reason you would want to use PHP's date vs. the storage engine's date would be for timezone, or reference point. If you needed data referenced from what time the web server says it is, go with that. Otherwise, use `NOW()`. It all depends on what you want to do with that data. – Brad May 06 '12 at 16:23
Actually that's a more philosophical question; do you want the "timestamp" for creation to be created, while the code is processing the data or do you want it being processed, while the database transaction is processed. Depends on the situation, but both is valid and correct. – Robert Heine May 06 '12 at 16:24

PDO prepared statement, correctly used?

2 Answers2