4
\$\begingroup\$

How can I write this PHP code better? It puts together an SQL query string from user input. The task is to return search results based on one or more text fields. The user can determine if partial matches are allowed.

$compop = $allowpartial ? "LIKE" : "="; // use LIKE for wildcard matching
$any = $allowpartial ? "%" : ""; // SQL uses % instead of * as wildcard
$namecondstr = ($name === "") ? "TRUE" : ("Name $compop '$any$name$any'");
$citycondstr = ($city === "") ? "TRUE" : ("City $compop '$any$city$any'");
$itemnocondstr = ($itemno === "") ? "TRUE" : ("ItemNo $compop '$any$itemno$any'");
$ordernocondstr = ($orderno === "") ? "TRUE" : ("OrderNo $compop '$any$orderno$any'");
$serialcondstr = ($serial === "") ? "TRUE" : ("Serial $compop '$any$serial$any'");
$sortstr = ($name !== "") ? "Name" :
 (($city !== "") ? "City" :
 (($itemno !== "") ? "ItemNo" :
 (($orderno !== "") ? "OrderNo" :
 "Serial")));
$query = "SELECT * From Licenses
 LEFT JOIN Items
 ON Licenses.LicenseID = Items.LicenseID
 WHERE $namecondstr 
 AND $citycondstr 
 AND $itemnocondstr 
 AND $ordernocondstr
 AND $serialcondstr
 ORDER BY $sortstr, Licenses.LicenseID";
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Nov 29, 2012 at 4:09
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Use Prepared Statements

Your code is vulnerable to SQL injection attacks. Use PDO or mysqli prepared statements to avoid this. See this answer for how to use PDO for this. If you are using mysql_* you should know that it is already in the deprecation process.

Miscellaneous

  • Consider using empty to check for empty strings (see comment from Corbin below).
  • Personally I would use an if else rather than have two identical ternary conditions.
answered Nov 29, 2012 at 6:37
\$\endgroup\$
4
  • \$\begingroup\$ I use mysql_real_escape_string() whenever I get a parameter, I thought this would save me from SQL injections? \$\endgroup\$ Commented Nov 29, 2012 at 6:53
  • 1
    \$\begingroup\$ I didn't see it in your code above and you said it came from user input, so I thought it needed to be said. I think mysql_real_escape_string is okay so long as you remember to use it everywhere that you need to. It will suck when mysql_* is no longer a part of core PHP due to deprecation though. \$\endgroup\$ Commented Nov 29, 2012 at 7:15
  • 1
    \$\begingroup\$ On a very pedantic note: it's worth mentioning that empty($x) and $x === "" are not equivalent. You of course never said that they are equivalent, but it could catch someone unfamiliar with empty by surprise. \$\endgroup\$ Commented Nov 29, 2012 at 16:22
  • \$\begingroup\$ OK, I thought someone would comment on the (in my eyes pretty awful) chain of AND TRUE AND TRUE... terms if not all search criteria are specified by the user, or the rather dubious "Var $compop '$any$var$any'", but since there are no answers that tell me how to avoid those, I accept your answer. Thanks a lot for the deprecation note, will change to mysqli_.... \$\endgroup\$ Commented Dec 4, 2012 at 12:35

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.