0

I am new to PHP and PDO. I have a very basic question about PDO and escaping of single quotes. I have assumed that escaping is not necessary but I still have had some problems.

For example...the following method in my database class creates a new object in the database. It is abstracted and all my class extend the database class.

public function  create() {
    $attributes = $this->attributes();
    $question_marks = array();
    foreach ($attributes as $key => $value) {
        $question_marks[] = "?";
    }
    $place_holder = array_intersect_key($attributes, get_object_vars($this));
    $place_holder = array_values($place_holder);
    $sql = "INSERT INTO ".static::$table_name." (";
    $sql .= join(", ", array_keys($attributes));
    $sql .= ") VALUES (";
    $sql .= join(", ", array_values($question_marks));
    $sql .= ")";
    $query = $handler->prepare($sql);
    $query->execute($place_holder);
}

If I create say for example a new User. I can enter the name James O'Reilly into the database without difficulty...

But when I want to use my update method....

public function update() {
   $attributes = $this->attributes();
   print_r($attributes);
    $attribute_pairs = array();
    foreach($attributes as $key => $value) {
      if(isset($value))
      $attribute_pairs[] = "{$key}='{$value}'";
    }
    $sql = "UPDATE ".static::$table_name." SET ";
    $sql .= join(", ", $attribute_pairs);
    $sql .= " WHERE id=". $this->id;
    $query = $handler->prepare($sql);
    $query->execute(array());
}

It fails. If I manually remove the quote from the name in the database, this Update method works so its this quotation which I expected to be escaped that seems to be the issue.

Is it completely unecessary to escape single quotes when using PDO, and if so have I set my update method up as shown above incorrectly.

To add to these problems I can't seem to get my error outout to work right...this is my set up which from reviewing the excellent Wiki here, I think is correct... but of course feel free to flame.

$opt = array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,   PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC);
$handler = new PDO('mysql:host=localhost;dbname=lung', 'root', 'fidelio', $opt);
GhostRider
  • 2,109
  • 7
  • 35
  • 53
  • why did you assume that quoting is not necessary? – Your Common Sense Apr 30 '14 at 12:38
  • I had thought that with PDO, prepare escaped single quotes. From your comment I assume I am wrong about this. Is this the case? If you do still need to escape single quotes is their a PDO method that helps with this? – GhostRider Apr 30 '14 at 12:43
  • PDO is not a magic ball that makes your data safe just by it's presence. There is certain mechanism, supporded by PDO, you have to use – Your Common Sense Apr 30 '14 at 12:44
  • If you could offer some guidance into how this is done then that would be helpful. I have looked at PDO::quote and bindValue solutions but haven't been able to get them to work – GhostRider Apr 30 '14 at 13:04
  • If I remember right, you were given numerous times already. It's either famous question http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php or PDO tag wiki (the latter contains a ready made working function for thee task) – Your Common Sense Apr 30 '14 at 13:12
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/51750/discussion-between-ghostrider-and-your-common-sense) – GhostRider Apr 30 '14 at 13:16
  • If so, could you explain this http://stackoverflow.com/questions/10750377/mysql-real-escape-string-with-pdo-php – GhostRider Apr 30 '14 at 14:57
  • Yes, I can. refer to the link from my comment – Your Common Sense Apr 30 '14 at 15:00
  • Unfortunately that hasn't helped me and this vague attempt force me to 'learn for myself' is unhelpful and arrogant. Everyone has to start at some point....and that is always at the bottom - even easy documentation for experts can be difficult for those beginning. – GhostRider Apr 30 '14 at 15:08
  • 1
    I don't see the problem: In the first example you correctly use a prepared statement so you don't need to quote or escape your values (the question marks) and in the second example you dump your values straight in your sql query. The solution is simple: Use a prepared statement there as well. – jeroen Apr 30 '14 at 21:21
  • Jeroen, you are right and I figured this out. I didn't realise that my update statement wasn't done properly. Many thanks for your response. – GhostRider May 01 '14 at 05:22
  • @YourCommonSense He's just trying to learn how to do things properly, bless his rare soul :) Remember, there's a _heap_ of questions related for those to go through, and everyone starts somewhere. It can be very easy to get a little lost here. – Tim Post May 01 '14 at 12:02

1 Answers1

5

Merely using prepare() does not automatically quote all your variables appropriately. All prepare() sees is a string, after you have interpolated variables into it. There is no way for it to tell what part of the string came from a variable and what part came from literal strings in your code.

You must use parameters for all values, instead of interpolating them into the string.

public function update() {
    $attributes = $this->attributes();
    print_r($attributes);
    $attribute_pairs = array();
    $attribute_values = array();
    foreach($attributes as $key => $value) {
        if(isset($value)) {
            $attribute_pairs[] = "`{$key}`=?"; // use parameter placeholders
            $attribute_values[] = $value; // collect the corresponding values
        }        
    }
    $sql = "UPDATE `".static::$table_name."` SET ";
    $sql .= join(", ", $attribute_pairs);
    $sql .= " WHERE id=?"; // use a placeholder here too
    $attribute_values[] = $this->id; // and add to the collected values
    $query = $handler->prepare($sql);
    $query->execute($attribute_values); // pass the values here
}

Note I also put your table name and column names in back-ticks to help delimit them in case you use reserved words or special characters in your SQL identifiers.

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • Many thanks Bill, got it. This works well, and I think is cleaner than my solution hence marking as accepted answer. – GhostRider May 01 '14 at 07:32