php and sql query failing

Question

Hello,

I have this code

http://www.nomorepasting.com/getpaste.php?pasteid=22580

which is part of a small ajax application. I would like to know a better, more efficient way to assign $query, instead of copying the sql each time with a different query or a bunch of if clauses. Basically the query will be dependant on the link clicked, but I am not sure how to show that in the logic. I am also unsure why my SQL query in $result fails.

Answer 1

UPDATE: I integrated Eran's function into the refactored code. NOTE: I corrected it by passing the $table variable into it and renamed it since it doesn't search the query text only but mainly returns the needed rows!

MAIN MISTAKES:

• mistake 1: you overwrite query with query2 in all cases which breaks the code.
• mistake 2: LIKE'%$query%' there is a space missing between LIKE and ' => LIKE '%... this most probably breaks your code too

OTHER ISSUES

• security problem: sql injection danger, use mysql_real_escape_string
• \n not platform independent: use PHP_EOL
• alternative way of writing short if blocks
• use curly brackets for normal if structures and all such structures for the matter

here is your code with some changes, look at the comments:

<?php
session_start(); //ommit, no session var used

//use braces, always!
//you may write such statements with the short form like
if (isset($_GET['cmd'])) : $cmd = $_GET['cmd']; else : die (_MSG_NO_PARAM); endif;

$query = '';
//escpae your input - very important for security! sql injection!
if ( isset ($_GET["query"]))
{
    $query = mysql_real_escape_string($_GET["query"]);
}
//no need for the other part you had here

$con = mysql_connect("localhost", "root", "geheim");

if (!$con) : die ('Connection failed. Error: '.mysql_error()); endif;

mysql_select_db("ebay", $con);

if ($cmd == "GetRecordSet")
{
    $table = 'Auctions';
    $rows = getRowsByArticleSearch($searchString, $table);

    //use PHP_EOL instead of \n in order to make your script more portable

    echo "<h1>Table: {$table}</h1>".PHP_EOL;
    echo "<table border='1' width='100%'><tr>".PHP_EOL;
    echo "<td width='33%'>Seller ID</td>".PHP_EOL;
    echo "<td width='33%'>Start Date</td>".PHP_EOL;
    echo "<td width='33%'>Description</td>".PHP_EOL;
    echo "</tr>\n";

    // printing table rows
    foreach ($rows as $row)
    {
        $pk = $row['ARTICLE_NO'];
        echo '<tr>'.PHP_EOL;
        echo '<td><a href="#" onclick="GetAuctionData(\''.$pk.'\')">'.$row['USERNAME'].'</a></td>'.PHP_EOL;
        echo '<td><a href="#" onclick="GetAuctionData(\''.$pk.'\')">'.$row['ACCESSSTARTS'].'</a></td>'.PHP_EOL;
        echo '<td><a href="#" onclick="GetAuctionData(\''.$pk.'\')">'.$row['ARTICLE_NAME'].'</a></td>'.PHP_EOL;
        echo '</tr>'.PHP_EOL;
    }
}
mysql_free_result($result);
//mysql_close($con); no need to close connection, you better don't


function getRowsByArticleSearch($searchString, $table) 
{
    $searchString = mysql_real_escape_string($searchString);
    $result = mysql_query("SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME FROM {$table} WHERE upper ARTICLE_NAME LIKE '%" . $searchString . "%'");
    if($result === false) {
            return mysql_error();
    }
    $rows = array();
    while($row = mysql_fetch_assoc($result)) {
            $rows[] = $row;
    }
    return $rows;
}

// ?> ommit closing php tag

Answer 2

You haven't enclosed the statements in your IF/THEN/ELSE constructions in accolades so only the first statement in every block is conditionally executed, the rest allways is.

In most cases you'd be assigning $query2 to $query while $query2 probably hasn't been defined.

As another tip: sanitize your input, don't go pasting user input into your SQL like that, it's dangerous.

Answer 3

"SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME
FROM {$table} WHERE upper ARTICLE_NAME LIKE'%$query%'"

You need to put brackets around the parameters of your upper function. change your query to this, and it should work:

"SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME
FROM {$table} WHERE upper(ARTICLE_NAME) LIKE'%$query%'"

Answer 4

for a feature use:

$result = mysql_query($sql_query) or die(mysql_error());

To see what kind of mysql error you get.

Answer 5

You may need a space between LIKE and '%$query%'. Also, you should look into the mysql_error() function - let MySQL tell you exactly what the error is.

Answer 6

you should do like nickf said.

and you are definitely prone to SQL-Injection:

wikibooks: http://en.wikibooks.org/wiki/Programming:PHP:SQL_Injection long article: http://www.securiteam.com/securityreviews/5DP0N1P76E.html

Answer 7

You can abstract your query in a function that accepts the search text as a parameter. Something like:

function searchQuery($text) {
 $text = mysql_real_escape_string($text);
 $result = mysql_query("SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME FROM {$table} WHERE upper ARTICLE_NAME LIKE '%" . $text . "%'");
 if($result === false) {
  return mysql_error();
 }
 $rows = array();
 while($row = mysql_fetch_assoc($result)) {
  $rows[] = $row;
 }
 return $rows;
}

Note that you should escape user input to prevent SQL injection attacks (here I used mysql_real_escape_string() to do that). This function also returns the error code if the query fails, so you should check the result to see if it's an array or not:

 $result = searchQuery($_GET['query']);
 if(!is_array($result) ) {
      echo 'An error has occurred:' . $result;
 } else {
   //iterate over rows
 }

Wrap your logical structures (IF/ELSE) with curly brackets {. It's better for readability and helps avoid unnecessary mistakes.