Including PHP Variables into a MySQL query producing errors.

[Lamez]

Hello All,

I am running a local server with MySQL and PHP 5.

 

I am trying to do something like this:

<?php
function checkExist($table, $col, $var){
$q = mysql_query("SELECT * FROM $table WHERE $col = $var")or die("Function Check Exist: ".mysql_error());
$n = mysql_num_rows($q);
if($n == 0){
	$return = false;
}else if($n > 0){
	$return = true;
}
return $return;
}
?>

 

However I get errors like this:

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 'WHERE email = wizkid916@yahoo.com' at line 1

 

Maybe I am rusty, or there is a unclear error in my syntax.

 

Any ideas?

 

Thanks!

[Mchl]

If a variable is a string (like an email address) you have to enclose it into quotes.

 

Also remember to escape your variables before inserting them into query using mysql_real_escape_string.

In your case you need to escape all $table, $col and $var variables.

 

And last but not least

SELECT * FROM $table WHERE $col = $var LIMIT 1

will do the same job, but in shorter time.

[trq]

Also, string need to be within quotes in sql.

 

mysql_query("SELECT * FROM $table WHERE $col = '$var'")

[Lamez]

Okay I am getting slightly frustrated. It has been a long time, but I really think I am doing this right. If I was doing this right, then the code would compile.

 

Here is another error I am getting:

ID Query: 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

 

Here is the query:

<?php
$id = mysql_query("SELECT * FROM $_people")or die ("ID Query: ".mysql_error());
$id = mysql_num_rows($id) or die (mysql_error());
?>

 

Here is all of my code:

<?php
include($path."core/includes/db-config.php");
include($path."core/includes/wide-variables.php");
function clean($var){
  $var = htmlspecialchars($var);
  $var = strip_tags($var);
  $var = mysql_escape_string($var);
  $var = trim($var);
  $var = mysql_real_escape_string(trim(strip_tags($var)));
  $var = htmlspecialchars($var,ENT_QUOTES);
  return $var;
}
function checkExist($table, $col, $var){
$q = mysql_query("SELECT * FROM $table WHERE $col = '$var' LIMIT 1")or die("Function Check Exist: ".mysql_error());
$n = mysql_num_rows($q);
if($n == 0){
	$return = false;
}else if($n > 0){
	$return = true;
}
return $return;
}
function RegisterUser($email, $first, $last, $pass1, $pass2){
$email = clean($email);
$first = clean($first);
$last = clean($last);
$pass1 = clean($pass);
$pass2 = clean($pass2);
$check = checkEmail($email);
$error = "Reg:";
if($pass1 === $pass2){
	$error .= "0";
}else{
	$error .= "1";
}
if($check == true){
	$check = checkExist($_people, "email", $email);
	if($check == false){
		$error .= "0";
	}else if($check == true){
		$error .= "1";
	}
}else if($check == false){
	$error .= "2";
}
$first = strtolower($first);
$last = strtolower($last);
$pass = md5($id).sha1($pass).md5($first.$last);
$id = mysql_query("SELECT * FROM $_people")or die ("ID Query: ".mysql_error());
$id = mysql_num_rows($id) or die (mysql_error());

if($error == "Reg:"){
	mysql_query("INSERT INTO ".$_people." (id, email, first, last, password) VALUES ('".$id."', '".$email."', '".$first."', '".$last."', '".$pass."')")or die("Function Check Register: ".mysql_error());
	return 0;
}else{
	return $error;
}
}
function checkEmail($email){
if(preg_match("/[.+a-zA-Z0-9_-]+@[a-zA-Z0-9-]+.[a-zA-Z]+/", $email) > 0){
	$pass = true;
}else{
	$pass = false;
}
return $pass;
}
?>

Any ideas?

 

Oh ya, as you can see, I did use mysql_real_escape_string()

 

-Thanks Guys, maybe I am just overlooking something

[mrMarcus]

echo $_people and post the value here exactly as it appears.

 

P.S. this has nothing to do with your issue at hand, but it was just sticking out like a sore thumb:

 

<?php
function checkExist($table, $col, $var){
   $q = mysql_query("SELECT * FROM $table WHERE $col = $var")or die("Function Check Exist: ".mysql_error());
   if(mysql_num_rows ($q) == 0){
      //$return = false; //change me to:
      return false;
   }else if(mysql_num_rows ($q) > 0){
      //$return = true; //change me to:
      return true;
   }
   //return $return; //lose this line;
}
?>

 

you don't need to create a variable to just be returned like that.  simple example here, but the concept of reducing redundancy goes deep.

[Lamez]

Thanks for the advice, if you notice anything like that, please post it.

 

Also, I thought I echoed out that variable. Should I include in the function variables?

[mrMarcus]

when you echo $_people, what does it give you?  post it here.

 

this function is a mess:

 

<?php
function clean($var){
  $var = htmlspecialchars($var);
  $var = strip_tags($var);
  $var = mysql_escape_string($var);
  $var = trim($var);
  $var = mysql_real_escape_string(trim(strip_tags($var)));
  $var = htmlspecialchars($var,ENT_QUOTES);
  return $var;
}
?>

 

you've used trim() and strip_tags() twice, as well as two versions of the mysql_real_escape_string()/mysql_escape_string() functions, as well as htmlspecialchars() twice.

 

now, there are several different ways to setup up efficient, readable, multi-purpose sanitizing functions, it just depends on your coding style.  just simplify it for now:

 

<?php
function clean($input, $html=false) {
//trim $input;
$input = trim($input);

if ($html)
{
	$input = strip_tags($input);
	$input = htmlentities($input);
}

return mysql_real_escape_string($input);
}
//usage: clean($var, true); OR: clean($var); is also fine, as $html will default to false.
?>

[Lamez]

Oh sorry, the output of people, as expected, is "people". Thanks for fixing that function. I am still confused on why this is not working.

[Lamez]

Further note:

I changed this:

<?php
$id = mysql_query("SELECT * FROM $_people")or die ("ID Query: ".mysql_error());
?>

 

to this:

<?php
$id = mysql_query("SELECT * FROM people")or die ("ID Query: ".mysql_error());
?>

 

and the error was depleted. So I found the problem, but when I echo out $_people (outside of the function, but on the same file), I get what I wanted. I wonder if I need to add the $_people into the function variables, and declare it when I need use the function? Any ideas?

[mrMarcus]

i didn't even notice that.  and yes, you must pass those values to be used within the function as an argument.  i can see just looking now, that you are not passing $pass either.  i believe it's supposed to be $pass1 anyways.

 

unless you set a $variable at a global level (not recommended), you must pass $variables as arguments (recommended).

[mattal999]

i didn't even notice that.  and yes, you must pass those values to be used within the function as an argument.  i can see just looking now, that you are not passing $pass either.  i believe it's supposed to be $pass1 anyways.

 

unless you set a $variable at a global level (not recommended), you must pass $variables as arguments (recommended).

Sorry, this is a little off-topic, but why not set $variable at a global level? I have done it once, but all of the other times I needed it I set it as an argument. Can you explain what the drawbacks are?

[Lamez]

Yes I caught that as well, I have changed it. I have altered the functions page to this:

<?php
include("db-config.php");
function clean($str){
$str = @trim($str);
if(get_magic_quotes_gpc()) {
	$str = stripslashes($str);
}
return mysql_real_escape_string($str);
}
function checkExist($table, $col, $var){
$table = clean($table);
$col = clean($col);
$var = clean($var);
$q = mysql_query("SELECT * FROM $table WHERE $col = '$var' LIMIT 1")or die("Function Check Exist: ".mysql_error());
$n = mysql_num_rows($q);
$n = 0;
if($n == 0){
	return false;
}else if($n > 0){
	return true;
}
}
function RegisterUser($table, $email, $first, $last, $pass1, $pass2){
$email = clean($email);
$first = clean($first);
$last = clean($last);
$pass1 = clean($pass);
$pass2 = clean($pass2);
$check = checkEmail($email);
$error = "Reg:";
if($pass1 === $pass2){
	$error .= "0";
}else{
	$error .= "1";
}
if($check == true){
	$check = checkExist($table, "email", $email);
	if($check == false){
		$error .= "0";
	}else if($check == true){
		$error .= "1";
	}
}else if($check == false){
	$error .= "2";
}
$first = strtolower($first);
$last = strtolower($last);
$pass = md5($id).sha1($pass1).md5($first.$last);
$id = mysql_query("SELECT * FROM $table")or die ("ID Query: ".mysql_error());
$id = mysql_num_rows($id) or die (mysql_error());

if($error == "Reg:"){
	mysql_query("INSERT INTO $table (id, email, first, last, password) VALUES ('".$id."', '".$email."', '".$first."', '".$last."', '".$pass."')")or die("Function Check Register: ".mysql_error());
	return 0;
}else{
	return $error;
}
}
function checkEmail($email){
if(preg_match("/[.+a-zA-Z0-9_-]+@[a-zA-Z0-9-]+.[a-zA-Z]+/", $email) > 0){
	$pass = true;
}else{
	$pass = false;
}
return $pass;
}

?>

 

Looks good to me, I also get no MySQL errors! However I have encountered another, annoying, problem.

 

The page that processes the information does not work all of a sudden, I have no idea why. I get no output what so ever, here is the page.

 

<?php
$path = "../../";
include($path."core/main.php");
$type = $_POST['type'];
if($type == 0){
$e = $_POST['e'];
$f = $_POST['f'];
$l = $_POST['l'];
$p = $_POST['p'];
$p2 = $_POST['p2'];
$check = RegisterUser($_people, $e, $f, $l, $p, $p2);
if($check == 0){
	echo "User Added.";
}else{
	//handleError($check);
	echo "Error!";
}
echo $check;
}else if($type == 1){
//add later.
}else{
header("Location: ".$_main_error);
}
?>

 

I am lost at this point.

[mrMarcus]

i didn't even notice that.  and yes, you must pass those values to be used within the function as an argument.  i can see just looking now, that you are not passing $pass either.  i believe it's supposed to be $pass1 anyways.

 

unless you set a $variable at a global level (not recommended), you must pass $variables as arguments (recommended).

Sorry, this is a little off-topic, but why not set $variable at a global level? I have done it once, but all of the other times I needed it I set it as an argument. Can you explain what the drawbacks are?

 

they are considered bad practice.  i look at them as the lazy-mans alternative (no offense intended, honestly).  by dropping global $vars throughout your code, you will get the point where you have no idea where these variables are coming from and what their values are anymore.  i've seen some pretty poor usage of global variables.

 

functions have the ability to take arguments .. use them.  it keeps code clean, as well as easy to read in case you need to go back and make changes, etc.

[trq]

echo $_people and post the value here exactly as it appears.

 

P.S. this has nothing to do with your issue at hand, but it was just sticking out like a sore thumb:

 

<?php
function checkExist($table, $col, $var){
   $q = mysql_query("SELECT * FROM $table WHERE $col = $var")or die("Function Check Exist: ".mysql_error());
   if(mysql_num_rows ($q) == 0){
      //$return = false; //change me to:
      return false;
   }else if(mysql_num_rows ($q) > 0){
      //$return = true; //change me to:
      return true;
   }
   //return $return; //lose this line;
}
?>

 

you don't need to create a variable to just be returned like that.  simple example here, but the concept of reducing redundancy goes deep.

 

This idea could be simplified even further.

 

function checkExist($table, $col, $var){
   $q = mysql_query("SELECT * FROM $table WHERE $col = $var")or die("Function Check Exist: ".mysql_error());
   return mysql_num_rows();
}

 

But I would also be inclined to have the function return false if the query failed.

 

 

function checkExist($table, $col, $var){
  if ($q = mysql_query("SELECT * FROM $table WHERE $col = $var")) {
    return mysql_num_rows();
  }
  return false;
}

[Lamez]

define global variables. Also, any help would be appreciated!

[Lamez]

Interesting thorpe, but I don't want to know how many rows exist, I just want to know if one exist. So what I have now seems sufficient.

[trq]

Interesting thorpe, but I don't want to know how many rows exist, I just want to know if one exist. So what I have now seems sufficient.

 

Same logic. My function returns true if one or more rows exists, false otherwise.

[mattal999]

they are considered bad practice.  i look at them as the lazy-mans alternative (no offense intended, honestly).  by dropping global $vars throughout your code, you will get the point where you have no idea where these variables are coming from and what their values are anymore.  i've seen some pretty poor usage of global variables.

 

functions have the ability to take arguments .. use them.  it keeps code clean, as well as easy to read in case you need to go back and make changes, etc.

Thanks for that. I've only ever used it once, but I'll have to fix up that code

 

@Lamez: Echo the value of these vars:

 

$e = $_POST['e'];
$f = $_POST['f'];
$l = $_POST['l'];
$p = $_POST['p'];
$p2 = $_POST['p2'];

Either one of those has a ' in it and is not being escaped OR the comparison here:

 

if($type == 0){ // Needs to be if($type == "0"){

Needs to be changed to check for a string, not an integer. You get the value from a POST, which comes through as a string. Not sure if this actually matters, but it's worth a try eh?

[Lamez]

Thorpe, the rows are suppose to be unique, if you function is more efficient, then I will use it, regardless. 

 

Also, I have echoed out the POST values, and they do return with the expected values. I do believe it is the function. I think it is time for a re-write.

 

-Thanks guys!