SitePoint Sponsor

User Tag List

Results 1 to 7 of 7
  1. #1
    SitePoint Enthusiast aweb4u's Avatar
    Join Date
    Jun 2007
    Location
    Auckland, New Zealand
    Posts
    75
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    MySQLi Best Practice

    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:

    Code:
    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:

    PHP Code:
    $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:

    PHP Code:
    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"); 
    PHP Code:
    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>&nbsp;</td>";
            }
          }
          if (
    $column <> 0) {
            echo 
    "</tr>";
          }
          echo 
    "<tr><td colspan=\"" MAXCOLUMNS "\">&nbsp;</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. #2
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    I'm not a mysqi user, but seeing as you asked:

    Quote Originally Posted by aweb4u
    Again, does anything jump out as being not best practice?
    If you (or anyone else) overlooks to check the incoming $category then by default you will not escape it, why risk it?

    PHP Code:
    // somewhere else in userland ...

    echo DisplayCategory($db$_POST['category']) ; 
    Also this could be a bit sharper:
    PHP Code:
    $ID = isset($_GET['ID']) ? $_GET['ID'] : ""
    if (
    $ID == "") { 
      
    $ID 1

    Just:
    PHP Code:
    $ID = isset($_GET['ID']) ? $_GET['ID'] : 1

  3. #3
    SitePoint Enthusiast aweb4u's Avatar
    Join Date
    Jun 2007
    Location
    Auckland, New Zealand
    Posts
    75
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks for that Cups, good suggestions there!

    Any other suggestions from anyone else?

  4. #4
    Programming Since 1978 silver trophybronze trophy felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, NSW, Australia
    Posts
    16,839
    Mentioned
    25 Post(s)
    Tagged
    1 Thread(s)
    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.
    Stephen J Chapman

    javascriptexample.net, Book Reviews, follow me on Twitter
    HTML Help, CSS Help, JavaScript Help, PHP/mySQL Help, blog
    <input name="html5" type="text" required pattern="^$">

  5. #5
    SitePoint Enthusiast aweb4u's Avatar
    Join Date
    Jun 2007
    Location
    Auckland, New Zealand
    Posts
    75
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by felgall View Post
    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.
    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. #6
    Programming Since 1978 silver trophybronze trophy felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, NSW, Australia
    Posts
    16,839
    Mentioned
    25 Post(s)
    Tagged
    1 Thread(s)
    Quote Originally Posted by aweb4u View Post
    even when the query input is coming from a trusted source?
    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.
    Stephen J Chapman

    javascriptexample.net, Book Reviews, follow me on Twitter
    HTML Help, CSS Help, JavaScript Help, PHP/mySQL Help, blog
    <input name="html5" type="text" required pattern="^$">

  7. #7
    SitePoint Enthusiast aweb4u's Avatar
    Join Date
    Jun 2007
    Location
    Auckland, New Zealand
    Posts
    75
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by felgall View Post
    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.
    Ah, gotcha! Many thanks for the pointers!


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •