Using a transaction to create account records

Question

This is my first time using commit and rollback. I just need to know if this is a proper execution of the functions for this particular situation:

<?php
  include("../include/sessions.php");

  if(isset($_POST['addcriteria'])) {
    $value = $_POST['addcriteria'];

    $addname = isset($value['addname ']) ? $value['addname '] : '';
    $addemail = isset($value['addemail']) ? $value['addemail'] : '';
    $addcomment = isset($value['addcomment']) ? $value['addcomment'] : '';

    try {
      $dbc->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

      $dbc->beginTransaction();

      $selectCheck = $dbc->prepare("SELECT * FROM mainTable WHERE `name` = :newname AND `email` = :newemail");

      $selectCheck->bindParam(':newname', $addname);
      $selectCheck->bindParam(':newemail', $addemail);

      if($selectCheck->execute()) {
        $count = $selectCheck->rowCount();

        if($count > 0) {
          echo "Error: Account already exists";
        } else {
          $select = $dbc->prepare("SELECT MAX(uid)+1 AS 'NEWUID' FROM mainTable");

          if($select->execute()) {
            $row = $select->fetch(PDO::FETCH_ASSOC);
            $addnewuid = $row['NEWUID'];

            $insert = $dbc->prepare("INSERT INTO mainTable (`name`,`email`) VALUES (:addname,:addemail)");

            $insert->bindParam(':addname', $addname);
            $insert->bindParam(':addemail', $addemail);

            $insert->execute();

            $insertComment = $dbc->prepare("INSERT INTO comments (`name`,`comments`) VALUES (:name, :comment)");

            $insertComment->bindParam(':name', $addname);
            $insertComment->bindParam(':comment', $addcomment);

            $insertComment->execute();
          }
        }
      } else {
        echo "error: There was an unexpected error.";
      }

      $dbc->commit();

      $dbc = null;
      
    } catch(PDOException $e) {
        echo "error: " . $e->getMessage();
        $dbc->rollBack();
    }
  }
?>

Does this appear as proper use of the commit and rollback functions?

What, if anything, can I do to improve this code?

Am I missing anything?

Answer 1

Does this appear as proper use of the commit and rollback functions?

I honestly haven't used PDO functions directly - typically used a framework like Laravel or similar.

I do agree with the advice on phpdelusions.net and it has a page about PDO usage with a section about Transactions. It mentions:

Please note the following important things:

• PDO error reporting mode should be set to PDO::ERRMODE_EXCEPTION
• you have catch an Exception, not PDOException, as it doesn't matter what particular exception aborted the execution.
• you should re-throw an exception after rollback, to be notified of the problem the usual way.
• also make sure that a table engine supports transactions (i.e. for Mysql it should be InnoDB, not MyISAM)
• there are no Data definition language (DDL) statements that define or modify database schema among queries in your transaction, as such a query will cause an implicit commit

It appears that the error reporting mode is set to PDO::ERRMODE_EXCEPTION✅ , though the catch only catches PDOException instances and the exception isn't re-thrown.

Then after the transaction has been committed, the connection is cleared with this line:

$dbc = null;

This is not required but is okay if you want to be sure it is closed.

---

What, if anything, can I do to improve this code?

Let's look at these three lines near the top:

$addname = isset($value['addname ']) ? $value['addname '] : '';
$addemail = isset($value['addemail']) ? $value['addemail'] : '';
$addcomment = isset($value['addcomment']) ? $value['addcomment'] : '';

Should the first string index contain a space - 'addname '? Perhaps that is a typo?

The server is running PHP 7.3 or higher, right? Hopefully that is the case since at the time of writing those are the supported versions receiving Security fixes.

Presuming that is the case, then these lines could be simplified using the null-coalescing operator i.e. ??

$addname = $value['addname '] ?? '';
$addemail = $value['addemail'] ?? '';
$addcomment = $value['addcomment'] ?? '';

If the first two are actually '' then does it even make sense to run the queries? If not, then handle that case appropriately - e.g. with an error message or other means.

Next, let's look at that first query:

$selectCheck = $dbc->prepare("SELECT * FROM mainTable WHERE `name` = :newname AND `email` = :newemail");

Since that query is only used later to call $selectCheck->rowCount() then there isn't a point to select *. It could simply select 1 or a single field. This will avoid the database query returning data that isn't needed.

Answer 2

Retrieving the ID of newly-created user should not be done like this:

$select = $dbc->prepare("SELECT MAX(uid)+1 AS 'NEWUID' FROM mainTable");

Instead use PDO::lastInsertId()

But you are not using variable $addnewuid. So you are doing one query for nothing. Is your code complete and working as expected ? At the moment there is no firm relation between the two tables. The only loose connection is the person's name.

---

There should be a minimum of validation/sanitation for certain fields. You are using the raw values coming from a POST request, so you might want to trim certain values like name or E-mail perhaps, or even reject certain patterns and ask the user to check their input again.

I would also suggest renaming that table: "users" should do - mainTable is too generic and prefer lower case .