2
\$\begingroup\$

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);
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ I wouldn't use include in that way. Why not just call another function? \$\endgroup\$
    – stan
    Aug 26, 2017 at 0:28
  • \$\begingroup\$ @StanislavPalatnik Please put all critiques and suggestions in answers, not comments. \$\endgroup\$ Aug 26, 2017 at 1:11
  • \$\begingroup\$ What does $NAV_DB_Failure look like? \$\endgroup\$ Aug 26, 2017 at 1:11
  • \$\begingroup\$ The $NAV_DB_Failure pulls up a HTML webpage with my Navigation bar at top and $Message in body. \$\endgroup\$ Aug 26, 2017 at 1:37
  • \$\begingroup\$ In addition to the answer below, there's a bunch of general PHP stuff that could be improved. For one, use type annotations. Then, adhere to PSR coding guidelines concerning naming of classes, methods, and variables. \$\endgroup\$
    – uli
    Nov 23 at 18:03

1 Answer 1

3
\$\begingroup\$

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 === true) // insert, update and such
    {
        return mysqli_affected_rows($result);
    } else {
        return mysqli_fetch_all($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.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, that is really going to help out when making MySQLi statements, and handling the error messages. \$\endgroup\$ Aug 27, 2017 at 22:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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