I am creating a new webpage with database connection. I want to make sure that I am handling the MySQLi query correctly for Search, Insert, and Update. I also want to make sure if any errors do pop up, that the function will catch the error for type of query I am running.

Is the if (!$Results) the correct way to test for Error/Failure on both Select and Insert/Update?

I am planning on having another section of functions for each table in my database for Select, Insert/Update.

function DB_Select($Query, $Results, $Rows) {
    DB_Test($DB_Connection);
    $Results = mysqli_query($DB_Connection, $Query);
    if (!$Results) {
        $Message = 'Failed Search: ' . mysqli_error();
        include $NAV_DB_Failure;
        exit();
    } else {
        $Rows = mysqli_affected_rows($Results);
    }
}

function DB_Insert_Update($Query, $Results, $Rows) {
    DB_Test($DB_Connection);
    $Results = mysqli_query($DB_Connection, $Query);
    if (!$Results) {
        $Message = 'Failed Insert/Update: ' . mysqli_error();
        include $NAV_DB_Failure;
        exit();
    } else {
        $Rows = mysqli_affected_rows($Results);
    }
}
  • I wouldn't use include in that way. Why not just call another function? – Stanislav Palatnik Aug 26 '17 at 0:28
  • @StanislavPalatnik Please put all critiques and suggestions in answers, not comments. – 200_success Aug 26 '17 at 1:11
  • What does $NAV_DB_Failure look like? – 200_success Aug 26 '17 at 1:11
  • The $NAV_DB_Failure pulls up a HTML webpage with my Navigation bar at top and $Message in body. – John Williams Aug 26 '17 at 1:37
up vote 1 down vote accepted

Sorry to say, but you are doing it all wrong, from getting a mysqli connection to your ideas on the error reporting. I've got an article on this latter topic, Error reporting basics but it is rather long, so here is a gist:

On a live site, not a single word from the error message should be shown to a site user. Because:

  • for the casual user it is pretty cryptic and confusing.
  • it it essentially useless for the site programmer/admin as they likely won't be browsing the site at the moment and will have no idea that error has been occurred.
  • the information provided in the error message extremely useful for a potential hacker, as it provides them with the feedback for their actions and may leak some sensitive information on the site.

Instead, the error message should be logged for the future reference, whereas just a generic error page should be shown.

From this you can tell that your idea of showing the complete error message to the site user is essentially flawed. Instead of manually handling every single error message, you should just let it go, and then create a handler that will dispatch the error message according to the current mode.

The idea of adding the query type is rather useless as well. Given PHP will always provide you with the file name and line number where the error occurred, you can always find the problem query and thus learn its type.

Now, to the code review.

First, I would put mysqli into Exception mode, to make it report all errors automatically. To do so, I will add the following line to mysqli connection code:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

So, given you are using a function to get the mysqli connection, it could be written this way

function DB_Connect {
    static $conn;
    if (!$conn) {
         mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
         $conn = mysqli_connect('your credentials');
    }
    return $conn;
}

this function will ensure that we are using only a single connection and also will turn Exception mode for mysqli

Then I would make it only one function,

function DB_query($Query) {
    $DB_Connection = DB_connect();
    $result = mysqli_query($DB_Connection, $Query);
    return $result;
 }

with such a function you'll be able to perform any query and also get any result type you need. For example,

// select query returns an array with rows
$users = DB_Query("SELECT * FROM users")->fetch_all();

// update query returns the number of rows affected
$rows_updated = DB_Query("UPDATE users SET active=1")->num_rows();

However, there is another drawback. In fact, for such a function it is mandatory to support prepared statements. Otherwise it will be wide open to SQL injection. Therefore, we will change it to make it allow variables used in the queries

function DB_query($query, $params = [])
{
    $conn = DB_connect();
    if ($params)
    {
        $stmt = $conn->prepare($query);
        $types = str_repeat('s', count($params));
        $stmt->bind_param($types, ...$params);
        $stmt->execute();
        $result = $stmt->get_result();
    } else {
        $result = mysqli_query($conn, $query);
    }
    if ($result)
    {
        return mysqli_fetch_all($result);
    } else {
        return mysqli_affected_rows($result);
    }
}

This function will return you an array of rows for SELECT and other queries that return any data, and the number of affected rows otherwise. Examples:

$workers = DB_query("SELECT * FROM workers WHERE salary >= ?",[$salary]);

DB_query("INSERT INTO workers SET name=?, salary=?",[$name,$salary]);
$id = DB_connect()->last_insert_id();

As for the $NAV_DB_Failure - create the error handler like this

set_error_handler("myErrorHandler");
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
    error_log("$errstr in $errfile:$errline");
    header('HTTP/1.1 500 Internal Server Error', TRUE, 500);
    readfile("NAV_DB_Failure.html");
    exit;
}

and this file will be shown in case of any error, while keeping the error message for your eyes only.

  • Thank you, that is really going to help out when making MySQLi statements, and handling the error messages. – John Williams Aug 27 '17 at 22:20

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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