3
\$\begingroup\$

I believe with everyone's help on Stack Overflow, I got my code safe guarded from SQL injection. I'm trying to confirm that is correct, just in case I misinterpreted the help and advice I received.

<?php
$username = "xxxx";
$password = "xxxx"; 
?>

<font size="+3" face="Verdana">xxxx</font>
<br><br>
<form name="form" action="xxxx.php" method="get">
<input type="text" name="q" size="60" />
<input type="submit" name="Submit" value="Search">
</form>

<table>

<?

$var = $_GET['q'];

try {
$conn = new PDO('mysql:host=localhost;dbname=xxxx', $username, $password);
$stmt = $conn->prepare("SELECT * FROM xxxx WHERE xxxx LIKE :search");
$stmt->execute(array(':search' => '%'.$var.'%'));

$result = $stmt->fetchAll();

if ( count($result) ) {
foreach($result as $row) {
echo "<tr><td align=center width=100>" 
. $row['xxxx'] .
"</td><td align=center width=100>"
. $row['xxxx'] .
"</td><td align=center width=100>"
. $row['xxxx'] .
"</td><td align=center width=100>"
. $row['xxxx'] .
"</td><td align=center width=100><img src=http://www.xxx.com/"
. $row['image'] . " height=50>" .
"</td><td align=left width=500>http://www.xxx.com/" . $row['image'] . "</a>
</td><td align=center valign=middle width=100>
<form action=xxx.php method=POST>
<input type=hidden value=" . $row['id'] . " name=edit>
<input type=image src=http://www.xxx.com/images/edit.png height=30>
</form>
</td><td align=center valign=middle width=100>      <form action=xxx.php    
method=POST>
<input type=hidden value=" . $row['id'] . " name=delete>
<input type=image src=http://www.xxx.com/images/delete.png height=30>
</form>
</td></tr>";  
}  
} 
else {
echo "No results found.";
}
} 
catch(PDOException $e) {
echo 'ERROR: ' . $e->getMessage();
}
?>
</table>
\$\endgroup\$
10
\$\begingroup\$
  • You should not mix PHP logic and HTML. All logic should be placed on top of the document and HTML (+ some minor PHP loops/echos/ifs) below it;
  • $var = $_GET['q']; is unneeded and you loose memory;
  • never use the shorttag (<?);
  • You forgot to set the errormode for PDO, that means it doesn't throw exceptions. To set it, use PDO::setAttribute:

    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    
  • I do not recommend to use * in your queries. The query will be faster and easier to use if you specify which columns you want;
  • You should specify the fetch style of PDOStatement::fetchAll. Otherwise it defaults to PDO::FETCH_BOTH, which means your array get to big and your script slower. You can specify it with a parameter in the function:

    $stmt->fetchAll(PDO::FETCH_ASSOC);
    

    Or specify it for the PDOstatement:

    $stmt->setFetchMode(PDO::FETCH_ASSOC);
    
  • A small tip: Indent your code within each block with a tab or 4 spaces, that makes your code much easier to read. Also, choose a coding standard and use it everywhere. Consistent scripts are a pleasure to write, read and use!

Some none PDO/PHP things:

  • The <font> element is outdated. You should do this with CSS;
  • You should not use inline CSS (like style=, align=, width=), put it in a CSS stylesheet instead;
  • When you are using 2 <br> next to eachother, you almost always know you do something wrong and you should take a look at how to solve it with CSS margins/paddings.
\$\endgroup\$
  • \$\begingroup\$ What if I NEED to select every column in the database when displaying results? Should I still type them out manually rather than using *? I kind of understand what you are saying about mixing PHP logic & HTML but do you have a good example of why not to do this or just something that shows where I'm going wrong? I believe you are saying the form should be at the end of the page not the top. But is using <table> before and after the output ok? I was unable to put it in the foreach loop unless I want a new table every time. Thanks! \$\endgroup\$ – gb2016 Mar 25 '13 at 18:21
  • \$\begingroup\$ 1) Yes, even if you need all columns I recommend to type them out. It's more readable, you know your column names so you know which data you get back, otherwise you need to go to PmA to see how you database look like and go back to use it in your scripts. And it's faster most of the times. \$\endgroup\$ – Wouter J Mar 25 '13 at 18:31
  • \$\begingroup\$ 2) It's about Seperation of Scripts. You shouldn't mix 2 languages. And you avoid errors like 'Headers already sent'. 3) In your HTML, you can use some PHP echo's, loops, ect. \$\endgroup\$ – Wouter J Mar 25 '13 at 18:32
  • 1
    \$\begingroup\$ it's also a go idea to list out the columns you want, in the case that columns are added to the table, you may not want those columns and the extra columns could break your query, whereas identifying the columns should keep the long term use of the query to it's longest. \$\endgroup\$ – Malachi Apr 25 '14 at 17:04
  • \$\begingroup\$ Out of curiosity, what's wrong with the <?=?> short tag? I understand that not all servers have it enabled, but assuming you have control over your servers is there another drawback? \$\endgroup\$ – Matthew Johnson Apr 25 '14 at 17:43
1
\$\begingroup\$

You can use PHP to output the page to your own for action="xxxx.php" in the form tag.

action="<?= htmlentities($_SERVER['PHP_SELF']); ?>"

It looks like a lot to type, but it'll help if you might decide to change file name later. Other than that, I don't see any other cause of concern. You might want to think about using fetchAll too. It might slow down your page generation because it takes up a lot of memory to store the entire result into a single array. You can use a while loop too, as follows:

$stmt->execute(array(':search' => '%'.$var.'%'));
while( $row = $stmt->fetch() ) {

This way, if no result was found, nothing will output to the page. And it'll be considerably faster than fetchAll request.

\$\endgroup\$
  • \$\begingroup\$ Thanks. I updated my code with the information you posted here. \$\endgroup\$ – gb2016 Mar 24 '13 at 5:38
  • 1
    \$\begingroup\$ the $_SERVER['PHP_SELF'] is not really needed. You can also leave it empty or use #. I would not recommend to use $_SERVER['PHP_SELF'], because that can be a security issue (you solved it here, but that makes the code much harder than it needs to be) \$\endgroup\$ – Wouter J Mar 24 '13 at 14:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.