3
$name=$_GET['name'];
$sql="SELECT `productname`, `price` FROM `stock` WHERE productname=".$name."
 and ( userid=".$_SESSION['user_id']." or userid='100')";

Basically I am getting product name from one page. I am passing query where productname is the $name and the userid is either the current signed one or the admin userid (100).

But I am getting error

Notice: Undefined variable: productname in /path_to_file.php on line 431

line 431: <?php echo $productname;?>

where $productname=$row['productname'];

I sense something is wrong in sql query. But what is wrong?

Santosh
  • 1,871
  • 1
  • 19
  • 36
  • Please post the full code. – Vivek Sadh Jul 05 '13 at 17:27
  • For your error: Your variable is, as the error says, undefined. Typical solution would be to utilize isset() (http://php.net/manual/en/function.isset.php) to check if the variable has been set initially, and if so (using an if() (http://php.net/manual/en/control-structures.if.php) statement), proceed to set the variable. I would refer you to here: http://stackoverflow.com/questions/4261133/php-notice-undefined-variable-and-notice-undefined-index – Justice Cassel Jul 05 '13 at 17:28
  • You should be [using a modern interface like PDO](http://net.tutsplus.com/tutorials/php/why-you-should-be-using-phps-pdo-for-database-access/) to have [reliable SQL escaping](http://bobby-tables.com/php). Using string concatenation is a very bad idea and you have a **massive** [SQL injection bug](http://bobby-tables.com/) here. – tadman Jul 05 '13 at 18:02

4 Answers4

3
$name = $_GET['name'];
$sql = sprint("SELECT `productname`, `price` FROM `stock` WHERE productname='%s'
    and ( userid=%d or userid=100);",
    mysql_escape_string($name), intval($_SESSION['user_id']));

You need to wrap the productname='%s' in single-quotes. See above.

CodeAngry
  • 12,760
  • 3
  • 50
  • 57
  • @RolandoMySQLDBA Hope he'll figure out the use of that... I also forced the `user_id` to `int` to prevent SQL injections :) – CodeAngry Jul 05 '13 at 17:31
  • I think I will have to switch to mysqli from mysql pretty soon to get rid of sql injections! Thanks for pointing out mistake. – Santosh Jul 05 '13 at 17:49
  • 1
    Its just like c . printf("My name is %s.My age is %d",name,age); Thanks I will try to improve my sql now. – Santosh Jul 05 '13 at 17:58
  • @HS Yes. And it allows you to see the string better when you are formatting variables into it. Keeps it short, simple and allows type forcing like using `%d` for integers. – CodeAngry Jul 05 '13 at 18:02
2

You need single quotes around the $name

$name=$_GET['name'];
$sql="SELECT `productname`, `price` FROM `stock` WHERE productname='". mysql_escape_string($name). "' and ( userid=".$_SESSION['user_id']." or userid='100')";
RolandoMySQLDBA
  • 43,883
  • 16
  • 91
  • 132
1

There are a couple things wrong:

First, you are not escaping your input data, so you are vulnerable to SQL injection.

Second, if $name is indeed a string value, it will need to be enclosed by single quote in your query.

Other than that, there isn't anything wrong with your query, so long as it matches with your table schema and the data you are looking for exists in the DB.

Mike Brant
  • 70,514
  • 10
  • 99
  • 103
1

Here's an example of writing this in a safer way with PDO:

$name=$_GET['name'];
$sql="SELECT `productname`, `price` FROM `stock` 
  WHERE productname = ? AND userid IN (100, ?)";
$stmt = $pdo->prepare($sql);
$stmt->execute(array($name, $_SESSION["user_id"]));

Using parameters instead of interpolating PHP variables into strings is not only safer for SQL injection defense, it's also easier to write the code and easier to read the code.

You can also do something similar with mysqli, but I find PDO the easiest.

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • PDO also supports named placeholders like `:name` and `:user_id` which usually makes queries easier to read. Having `$sql` as an intermediate variable is also redundant. – tadman Jul 05 '13 at 18:03
  • @tadman, I like to put the SQL in a variable to help debugging and testing. – Bill Karwin Jul 05 '13 at 18:14