Using PDO to list and remove updates

Question

I started using PDO 2 days ago, but I want to know if the way I'm coding and the way I'm using it is the correct way of doing things with PDO.

Please let me know if you guys notice any security flaws or improvements in general.

Here is a function I wrote that shows updates.

function ShowRemoveUpdates() {
    ob_start();

    try {
      // connectie gegevens
      require_once('connection.php');

      // pagination
      $start  = 0;
      $limiet = 8;

      if(isset($_GET['page'])) {
        $page   = $_GET['page'];
      } else {
        $page   = 1;
      }

      // query
      $stmt = $pdo->prepare('SELECT * FROM updates ORDER BY id DESC LIMIT :start, :limiet');
      $stmt->execute(array(':start' => $start, ':limiet' => $limiet));

      //start van formulier en tabel
      echo "<form method='post' action=''>";
      echo "<table id='tb1'>";
      echo "<th align='left'>Geplaatst door</th><th align='left'>Titel</th><th align='left'>Tekst</th><th align='left'>Tijd</th>";

      // while loop door resultaten van query
      if($stmt->rowCount() > 0) {
        while ($row = $stmt->fetch()) {
          $idje = $row['id'];

          echo "<tr>";
          echo "<td width='10%'>".$row['medewerker']."</td>";
          echo "<td width='30%'>".$row['titel']."</td>";
          echo "<td width='50%'>".$row['tekst']."</td>";
          echo "<td width='10%'>".$row['datum']."</td>";
          echo "<td><input type=\"checkbox\" name=\"checkvakje\" id=\"checkvakje\" value=\"$idje\"></td>";
          echo "</tr>";
        }
        echo "</table>";
        echo "<input type='submit' value='Verwijderen'>";
      } else {
        echo "Geen resultaten gevonden.";
      }

      // Verwijder update functie
      if(isset($_POST['checkvakje'])) {
        $checkvak   = $_POST['checkvakje'];
        $stmt = $pdo->prepare('DELETE FROM updates WHERE id = :checkvak');
        $stmt->execute(array(':checkvak' => $checkvak));
        header("Refresh:0");
        exit();
      }

      echo "</form>";

      // count resultaten per rij en maak pagina nummering
      $stmt = $pdo->prepare('SELECT * FROM updates');
      $stmt->execute();
      $rows = $stmt->RowCount();
      $total=ceil($rows/$limiet);

      if($page>1) {
        echo "<a href='?page=".($page-1)."' class='button'>Vorige</a>";
      }

      for($i=1;$i<=$total;$i++)
      {
        if($i==$page) {
          echo ".$i.";
          }

        else {
          echo "<a href='?page=".$i."'>- ".$i."</a>";
        }
      }

      if($page!=$total) {
        echo "<a href='?page=".($page+1)."' class='button'> - Volgende</a>";
      }
      // vang errors op
    } catch (PDOException $e) {
        echo "Connection failed" . $e->getMessage() . "";
    }
}

Answer 1

My initial thought is that this function is doing way too much. It is:

• reading user input (with no validation)
• instantiating a database connection (possibly)
• getting a result set from the database
• deleting records from the database (possibly)
• executing a query to get a row count from the database
• generating HTML output
• providing end user error messages

This is just too much logic to be put in one function. I would encourage you to embrace separation of concerns as a key tenant in how you approach programming.

---

If you properly separate concerns, you move away from having to implement hacky uses of output buffers - something which should typically be reserved for very targeted use cases. Speaking of your output buffer, I don't see where you are flushing it. I would think it really poor practice to begin buffering in one area of code and have the buffer flushed/closed in another area of code. You are opening up your code to having to deal with the complexity/fragility of nested buffers if you have this behavior in other areas of code.

---

Typical convention would have functions/methods in PHP starting with lowercase letters.

---

Your try-catch block is used here inappropriately. First, it encompasses way too many lines of code. There are a number of PDO operations within this code that could throw, yet you only have a single catch block, which seems to consider only that the exception was due to connection failure. So perhaps any try-catch logic around database connection should be in the include file and not even in this code at all, and you could have try-catch around individual database operations (i.e. query preparations) to give more granular control over how underlying exceptions are handled.

The second problem may make all this moot though, in that you should really only be catching exceptions if you are going to do something meaningful in the recovery operation. Meaningful might be something like:

• logging some aspect of code state before re-throwing the exception to caller;
• gracefully recovering from exception such that caller can continue working in an acceptable state;
• wrapping/rethrowing the exception to abstract underlying details from calling code.

You are not really doing such a thing here. You are basically swallowing the exception such that the calling code, which is probably in a better position to understand how to deal with such an exception (i.e. by messaging end user or similar), has no idea at all that anything went wrong. Don't swallow exceptions.

I appreciate that you are trying to move into using exceptions in your code, but you really need to think of exceptions as a way for one piece of code to communicate to calling code that it was not able to fulfill the contract/promise of what the calling code expects it to do. If that calling code doesn't know how to recover from or handle such an exception in the code it calls, then it should let that exception continue to "bubble" up the call stack until there is code that is designed to handle the failure.

---

My next comment is related to the above exception handling comment, in that code that is not designed to message the end user about error conditions, should not do so. Don't just leak out end user error messaging about DB connections failing with technical exception messages. For the end user you likely want to clean up the messaging to something more meaningful as well as something that doesn't potentially leak sensitive application state information to the end user. Typically, code up the call stack from code that is actually interacting with the database is much better positioned to handle this end user messaging.

---

Consider changing $start to $offset which is much more common terminology with regards to pagination. Consider changing $limiet to $limit (the correct English word) if you are going to be writing your variable names in English. Also consider naming your GET/POST parameters in English for consistency (again, that is if your desire is to have your code written in English). Mixed-language code can be confusing to the reader.

---

You are not using $page at all in determining your offset for the LIMIT clause, so your pagination is not really working.

---

I would think your delete use case should be in a separate function altogether.

---

Consider using SQL_CALC_FOUND_ROWS in your LIMIT query in conjunction with FOUND_ROWS() to optimize this call pattern.

---

I would encourage you to move away from SELECT * when querying the database in favor of explicitly naming the fields you need in the result set. This may take you a minute or two extra when first writing the query, but it:

• makes it clear to the reader of the code as to what fields will be available in the result set - they don't need to go look at the database schema to know what fields are available.
• is oftentimes more efficient with regards to bandwidth and memory utilization. Why pull and store fields that you are not going to work with?
• can make your application less fragile to schema changes. For example, if at some point you add a field to this table, if a particular section of code does not need that field, you don't have to change it at all. With a * select, you could actually break your application if you start introducing new fields that are not expected.

---

Why are you setting a refresh header here? It doesn't make sense by itself here. Do you actually want to do a redirect? If you move the code into different function or before the output from section above, can you get rid of the output buffer?

Again separation of concerns will help you here, as the typical approach one should take in a PHP web application is to do all your PHP back-end logic (dealing with user input, interacting with databases, setting header, performing redirects, preparing data for output, etc.) before you ever get to the point of rendering a view to the browser.

---

You should validate user input. Prepared statements are great defense against SQL injection, but they are no defense against useless input. What if your values for page and checkvakje are not integer representations? Why would you even attempt a (relatively expensive) query with non-sensical input?

---

Your spacing around assignment operators is inconsistent.

---

Is there any reason you are using loose comparison operators (==, !=) instead exact comparisons here? I would recommend getting in the habit if using exact comparisons by default and only using loose comparisons when there is a true use case for doing so. this will make your could less fragile to unexpected truthy/falsey condition evaluations.

---

You have potential XSS vulnerability in that you are working with unvalidated user input in $page. Never trust user input.

---

You are inconsistent in bracket placement after flow control structures. Sometimes they start on same line, sometimes on following line. You should strive to be consistent in your styling. I would recommend a good PHP static code analysis tool like code sniffer or phpMd to help to enforce consistent styling as well as point our other questionable coding practices.

---

  if(isset($_GET['page'])) {
    $page   = $_GET['page'];
  } else {
    $page   = 1;
  }

Avoid unnecessary code branches (such as the unnecessary else shown in the example above). This could be:

  // here with a bonus validation suggestion
  $page = filter_input(INPUT_GET, 'page', FILTER_VALIDATE_INT)) {
  if(empty($page) || $page < 1) {
      $page = 1;
  }

  // here a suggestion without meaningful validation
  $page = 1;
  if(!empty($_GET['page'])) {
      $page = $_GET['page'];
  }

This may seem trivial, but this is more about getting the right mindset that code branches are to be designed away if possible, as the more code branches you have, the harder your code is to test and the more buggy your code is likely to be.

Answer 2

I can see three main issues here.

1. try..catch is rather useless here if not harmful. Remove this statement for the much better error handling.
2. Never use rowcount() to get the number of rows. Basically you are selecting whole table into PHP's memory. Select the count only using select count(*) query.
3. Overall code structure is hugely inconsistent but that's too big topic to discuss. But in short it shouldn't be a function, there shouldn't be ob_start. Data selection, selected data displaying and form processing should be separated from each other.