2

I have a query below which I did with mysql_query before and it executed properly.. But using PDO it's showing some error

Fatal error: Uncaught exception 'PDOException' with message '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 ')' at line 1'

This is my code with mysql_query :

$sql1 = "SELECT * FROM product WHERE id IN (";
                    foreach($_SESSION['cart'] as $id => $value){
                        $sql1 .= $id.',';
                    }
                $sql1 = substr($sql1, 0, -1) .")";  
                $query = mysql_query($sql1);

Using PDO without prepare statement.. : 

$sql1 = "SELECT * FROM product WHERE id IN (";
                    foreach($_SESSION['cart'] as $id => $value){
                        $sql1 .= $id.',';
                    }

                $sql1 = substr($sql1, 0, -1) .")";

                $query = $db->query($sql1);
nick
  • 333
  • 2
  • 9
  • 1
    You should use prepared queries for this. Insert `?`, bind parameters. – Brad Mar 06 '14 at 20:58
  • Magically changing `mysql_` to `PDO` does not fix the fact that you are injecting variables directly into your query meaning it might fail (and is **very** insecure!). – h2ooooooo Mar 06 '14 at 20:58
  • I know that very well... Using prepared statement and bind parameter won't fix this problem.. I will prevent sql injection later – nick Mar 06 '14 at 21:00
  • 2
    I don't understand why one would take the thought process of "I will fix SQL injection later". Why not just code it right the first time? Why come back and have to refactor your code a second time when you will know you need to do it from the start? – Mike Brant Mar 06 '14 at 21:01
  • Please echo out $sql1 and post. It looks like it should work. I suspect maybe the cart is empty. IN () will trigger a sql error. – Cerad Mar 06 '14 at 21:01
  • 3
    ignoring all the other problems with the code, why not just `$sql = "..." . implode(',', array_keys($_SESSION['cart']))`? Boom, instant proper number of commas. – Marc B Mar 06 '14 at 21:05
  • @Brad You Can't bind a parameter in a prepared statement in a case where you are using an `IN()` type construct. Depending on how the cart data is set to session (which is secure from user tampering), you might not need to worry about SQL injection. If your system controls all values put into `$_SESSION['cart']` (or does appropriate data cleansing before placing that data in session variable), then there may not be an SQL injection vulnerability here. – Mike Brant Mar 06 '14 at 21:05
  • @MikeBrant Why can't you? At least, I've never had trouble doing this with MySQL and PDO. – Brad Mar 06 '14 at 21:13
  • @Brad - Sadly, prepared statements do not accept arrays as input for individual parameters. There are workarounds but nothing simple. – Cerad Mar 06 '14 at 21:16
  • @Brad I guess I should clarify. You can't do something like `SELECT * WHERE foo IN(?)`, where `?` is something like `1,2,3,4` but you can do something like `SELECT * WHERE FOO IN(?,?,?,?)`. You would just have to bind each individual value in the IN statement independently. If you were trying to use a single prepared statement repeatedly with a variable number of `IN` values, this can not be achieved. So it is a little bit anti-thetical to the concept of a true prepared statement. – Mike Brant Mar 06 '14 at 21:19
  • @MikeBrant It can be achieved... I've done it. Get the length of array, repeat that number of place holders, bind an array of values. – Brad Mar 06 '14 at 21:42
  • @Brad That is what I just mentioned. Though like I said, you can't reuse that prepared statement in the case where the number of value in the `IN()` condition would change. So for a single-use prepared statement this can be done. – Mike Brant Mar 06 '14 at 21:47
  • @MikeBrant I understand what you are getting at. Yes, I agree. – Brad Mar 06 '14 at 22:50

2 Answers2

4

You miss to "add" the string here:

$sql1 = substr($sql1, 0, -1);
$sql1 .=  ")";
djot
  • 2,952
  • 4
  • 19
  • 28
  • Nope. `.=` is NOT the answer. you're appending the "fixed" query to the unfixed one, essentially `SELECT ...SELECT..` – Marc B Mar 06 '14 at 21:05
  • Didn't see you tried to "cut of" `$sql1` first. So it should work like you did or perhaps the way I show here. – djot Mar 06 '14 at 21:10
  • @user3361854 Echo your `$sql1` ... I bet you have `SELECT * FROM product WHERE id IN )`, which would mean your `$_SESSION['cart']` is empty ... and use the comment of @Marc B avoiding the loop at all. – djot Mar 06 '14 at 21:14
  • @djot yes I did something wrong with the session.. It was totally my mistake... But I don't understand how to do it in prepared statement. like some bindParam for this part `foreach($_SESSION['cart'] as $id => $value){ $sql1 .= $id.',';}` – nick Mar 06 '14 at 21:55
  • @user3361854 search SO ... http://stackoverflow.com/questions/920353/can-i-bind-an-array-to-an-in-condition ... see the answer there! – djot Mar 06 '14 at 21:56
3

In the PDO tag (info) you will find the correct procedure for PDO Prepared statements and IN.

PDO Tag

The following code uses this method to add unnamed placeholders from your SESSION array

$in = str_repeat('?,', count($_SESSION['cart']) - 1) . '?';
$sql1 = "SELECT * FROM product WHERE id IN ($in)";
$params = $_SESSION['cart'] ;
$stmt = $dbh->prepare($sql1); 
$stmt->execute($params);

DEMO

david strachan
  • 7,174
  • 2
  • 23
  • 33
  • Screen shot a little heavy-handed when a link would suffice, but good example using placeholders. – tadman Mar 06 '14 at 22:51
  • 1
    Very few people know about this feature as shown by the volume of questions being asked in this topic which could be answered by reading the info. – david strachan Mar 07 '14 at 00:27