MySQLi Best Practice

PHP
1

Ok, so I’m finally make the switch from using mysql functions to mysqli. I’ve read a bit and have hacked around some old code to experiment with. Before I go much further I’d like to make sure that I’m on the right track with how I’m doing things. I’ve got lots of scripts to update to use MySQLi and my goal is to change just the data access code and leave all of the rest untouched.

I’ve taken an old page for a website designer’s website that shows the designer’s portfolio. The portfolio details are stored in a MySQL database, just a single table that looks like this:

CREATE TABLE IF NOT EXISTS `portfolio` (
  `ID` int(11) unsigned NOT NULL AUTO_INCREMENT,
  `Category` enum('Commercial','Non-Profit','Personal','Professional','Retail') DEFAULT NULL,
  `SiteName` varchar(50) DEFAULT NULL,
  `URL` varchar(100) DEFAULT NULL,
  `Comments` mediumtext,
  `Picture` varchar(50) DEFAULT NULL,
  PRIMARY KEY (`ID`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=66 ;

So we can see that each client in the portfolio has its own ID and is given a Category to belong to. The rest is pretty straight forward.

The portfolio.php page expects to be passed an ID, if none is passed then it defaults to 1. The row that matches the passed ID is retrieved and details shown:

$ID = isset($_GET['ID']) ? $_GET['ID'] : "";
if ($ID == "") {
  $ID = 1;
}

$sql = "SELECT ID, Category, SiteName, URL, Comments, Picture FROM portfolio WHERE ID=?";
$selQuery = mysqli_prepare($db, $sql);
mysqli_stmt_bind_param($selQuery, 'i', $ID);
mysqli_stmt_execute($selQuery);
mysqli_stmt_store_result($selQuery);
$portfolioRow = array();
mysqli_stmt_bind_result($selQuery, $portfolioRow['ID'], $portfolioRow['Category'], $portfolioRow['SiteName'], $portfolioRow['URL'], $portfolioRow['Comments'], $portfolioRow['Picture']);
if (mysqli_stmt_num_rows($selQuery) < 1) {
  die("No records selected");
}
mysqli_stmt_fetch($selQuery);

// Display code removed

I haven’t shown the connection code, but assume that it sets a variable called $db that contains the connection link. I’ve elected to go with a prepared statement to handle the passed in ID.

The reason I’m using an array called $portfolioRow to hold the result is because the rest of the code that displays the result was already set up to use it and so I thought it made sense to keep it that way. Also, I think it’s tidier to keep it all in an array rather than set up lots of individual variables. Also I prefer procedural style over object.

I know this code works ok, but does it follow best practice rules? Should I be doing it any other way or doing more or different error checking?

After showing the details of the passed in ID the script then goes on to list all of the rows in the table, split up into Category sections (i.e. all the rows with Category = “Commercial”, then with Category = “Non-Profit”, etc) with links that call the page again with a new ID. I do this by putting the main logic in a function and then calling the function once for each category:

define("MAXCOLUMNS", 3);  // Number of columns for the display table
DisplayCategory($db, "Commercial");
DisplayCategory($db, "Professional");
DisplayCategory($db, "Retail");
DisplayCategory($db, "Personal");
DisplayCategory($db, "Non-Profit");


function DisplayCategory($db, $category) {
  $sql = "SELECT * FROM portfolio WHERE Category=\\"$category\\" ORDER BY SiteName";
  if ($portfolioRS = mysqli_query($db, $sql)) {
    if (mysqli_num_rows($portfolioRS) > 0) {
      echo "<table width=\\"100%\\" border=\\"0\\" cellpadding=\\"2\\" cellspacing=\\"1\\">";
      echo "<tr><td colspan=\\"" . MAXCOLUMNS . "\\"><h1>$category</h1></td></tr>";
      $column = 0;
      while($portfolioRow = mysqli_fetch_array($portfolioRS)) {
        if ($column == 0) {
          echo "<tr>";
          $column++;
        }
        echo "<td width=\\"33%\\" valign=\\"top\\" align=\\"left\\">";
        echo "<a href=\\"{$_SERVER['PHP_SELF']}?ID=" . $portfolioRow['ID'] . "\\">" . htmlentities($portfolioRow['SiteName']) . "</a>";
        echo "</td>";
        $column++;
        if ($column == MAXCOLUMNS + 1) {
          echo "</tr>";
          $column = 0;
        }
      }
      if ($column <> 0) {
        for ($i = $column; $i < MAXCOLUMNS + 1; $i++) {
          echo "<td> </td>";
        }
      }
      if ($column <> 0) {
        echo "</tr>";
      }
      echo "<tr><td colspan=\\"" . MAXCOLUMNS . "\\"> </td></tr>";
      echo "</table>";
    }
  } else {
    echo "<p>Error: " . mysqli_error($db) . "</p>";
  }
}

This time I didn’t use prepared statements because the input is coming directly from within the script.

Again, does anything jump out as being not best practice? Could I use more or different error checking?

Any advice would be greatly received.

Thanks in advance!

2

I’m not a mysqi user, but seeing as you asked:

If you (or anyone else) overlooks to check the incoming $category then by default you will not escape it, why risk it?


// somewhere else in userland ...

echo DisplayCategory($db, $_POST['category']) ;

Also this could be a bit sharper:


$ID = isset($_GET['ID']) ? $_GET['ID'] : ""; 
if ($ID == "") { 
  $ID = 1; 
}

Just:


$ID = isset($_GET['ID']) ? $_GET['ID'] : 1;
3

Thanks for that Cups, good suggestions there!

Any other suggestions from anyone else?

4

Now that you’re using mysqli you could look at replacing the query call with separate prepare and bind calls instead so that the data is kept completely separate from the SQL.

5

You mean in my DisplayCategory() function (because I did use prepare and bind in my first DB access)? So you’d recommend always using mysqli_prepare() and mysqli_stmt_bind_param() over mysqli_query(), even when the query input is coming from a trusted source?

6

It makes the difference between injection being unlikely and being impossible. Also there’s nothing to prevent the DisplayCategory() function being reused where the data is not from a trusted source.

7

Ah, gotcha! Many thanks for the pointers!