Php and sql crash
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 more efficient way to assign $ query, instead of copying sql with a different query or set of if statements each time. Basically the request will depend on the link, but I'm not sure how to show this in the logic. I'm also not sure why my SQL query on $ result is failing.
source to share
UPDATE . I have included the Eran feature in the refactored code. NOTE. I adjusted it by passing in the $ table variable and renaming it as it doesn't search just the query text, but basically returns the rows I want!
MAIN ERRORS :
- error 1: you are rewriting request with request2 in all cases, which breaks the code.
- error 2: LIKE '% $ query%' missing space between LIKE and '=> LIKE'% ... this is most likely breaking your code as well.
OTHER QUESTIONS
- security issue: sql infection danger, use mysql_real_escape_string
- \ n platform independent: use PHP_EOL
- an alternative way of writing short if blocks
- use curly braces for normal structures and all such structures for matter
here is your code with some changes, see 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
source to share
"SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME
FROM {$table} WHERE upper ARTICLE_NAME LIKE'%$query%'"
You need to put parentheses around your function parameters upper
. change your query to this and it should work:
"SELECT ARTICLE_NO, USERNAME, ACCESSSTARTS, ARTICLE_NAME
FROM {$table} WHERE upper(ARTICLE_NAME) LIKE'%$query%'"
source to share
you should do as 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
source to share
You can abstract your query into a function that takes 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 avoid user input to prevent SQL injection attacks (I used mysql_real_escape_string () here). This function also returns an error code if the request fails, so you have to 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 boolean structures (IF / ELSE) with curly braces {. This is better for readability and helps avoid unnecessary mistakes.
source to share
You have not nested statements in your IF / THEN / ELSE constructs as rewards, so only the first statement in each block is conditionally executed, the rest is always there.
In most cases, you should assign $ query2 to $, whereas $ query2 is probably undefined.
As another tip: sanitize your input, do not insert user input into your SQL, this is dangerous.
source to share