Procedural function for prepared MySQL statements in PHP

Question

I'm refactoring procedural code, switching to prepared MySQL statements using mysqli_query(). I do understand writing OO would be better, but it's out of scope for this task.

Use the below function as follows:

list($result, $numrows, $err) = my_mysqli_stmt($db_handler, $query, $params);

Returns array with three values, $result, $numrows and $err.

$result is either false on error, or contains empty or multi-dimensional array.

$numrows returns number of affacted rows, number of returned rows or 0 on error.

$err returns mysqli_error() or mysqli_stmt_error() error.

function my_dbstmt($handler, $query, $params=array())
    {   
    $stmt = mysqli_prepare($handler, $query);

    if($stmt===false)
        {
        return array(false, 0, mysqli_error($handler));
        }

    //First two parameter of mysqli_stmt_bind_param
    $refarg = array($stmt, array2typestring($params));

    //create array of parameters' references
    foreach ($params as $key => $value)
        {
        $refarg[] = &$params[$key];
        }

    if(!empty($params))
        {
        //bind parameters with dynamic length
        call_user_func_array("mysqli_stmt_bind_param", $refarg);
        }

    $exec = mysqli_stmt_execute($stmt);

    if($exec===false)
        {
        return array(false, 0, mysqli_stmt_error($stmt));
        }

    $result = mysqli_stmt_get_result($stmt);

    if($result===false) 
        {
        //not a result, but successfully executed
        $numrows = mysqli_stmt_affected_rows($stmt);
        return array(true, $numrows, null);
        }
    else 
        {
        mysqli_stmt_store_result($stmt);

        $data = array();

        while ($row = mysqli_fetch_array($result, MYSQLI_ASSOC))
            {
            $data[] = $row;
            }

        $numrows = count($data);
        return array($data, $numrows, null);
        }
    }

For completeness (detail of above referenced function:

function array2typestring($array)
    {
    $types = array();

    foreach ($array as $key => $value)  
        {
        $types[] = 's';
        }

    $types = implode('', $types);
    return $types;
    }

Example usage:

$db_handler = mysqli_connect("p:$dblocal", $dbuser, $dbpass) or die();
$query = "SELECT title FROM books WHERE id=?";
$params = array($bookid);
list($result, $numrows, $err) = my_dbstmt($db_handler, $query, $params);

Result:

$result holds multi-dimensional array with books' title (just one in this case). $numrows is 1. $err is null.

Catching errors would work as follows:

if($result===false) {print("Error finding book")}
elseif($numrows==0) {print("Book not found")}
elseif($numrows>0) {print("Book found")}

Answer 1

The main thing that needs to be refactored here is the approach itself. Calling a function like this

list($result, $numrows, $err) = my_mysqli_stmt($db_handler, $query, $params);

is absolutely not the way to go.

Definitely, there should be only one result, especially because this function returns superfluous variables which you don't really need.

$numrows is really useless as any time you need the count, you can count $result itself.

Returning $err is wrong too. You need to understand how a web-site works. If you take a look an any site around, including this one, none of them tells you silly errors like "Error finding a book". If an error occurs, the site just shuts down completely, because a database error is a disaster, and nothing useful will be shown anyway. that's why in case of error only a generalized page with excuses is shown. So in the end, in case of error, your function should return nothing, but actually throw an error to tell PHP that something terrible happened.

Therefore, the proper usage of such a function would be

$query = "SELECT title FROM books WHERE id=?";
$result = my_dbstmt($db_handler, $query, [$bookid]);

if(!$result) print("Book not found");
else print("Book found");

This is the most important thing about your function, while everything else is just nitpicks in comparison:

• There is a built-in function str_repeat which is doing exactly the same as your array2typestring.
• There is an argument unpacking operator which is doing exactly the same as your referencing magic
• there is a mysqli_fetch_all() function which is... so you get the idea.

using all this knowledge, you can write your function like this

function my_dbstmt($handler, $query, $params = [])
{
    $stmt = mysqli_prepare($handler, $query);
    $types = str_repeat('s', count($params));
    mysqli_stmt_bind_param($stmt, $types, ...$params);
    mysqli_stmt_execute($stmt);
    $result = mysqli_stmt_get_result($stmt);
    return mysqli_fetch_all($result);
}

The function above supports only SELECT queries. To make it support other queries is not a good idea, and better be implemented using different methods within the same class - the thing OOP is good for. But as you decided to stick with procedural, it could be done with different result type returned by this function: for SELECT queries it will return an array and for everything else the number of affected rows.

function my_dbstmt($handler, $query, $params = [])
{
    if ($params)
    {
        $stmt = mysqli_prepare($handler, $query);
        $types = str_repeat('s', count($params));
        mysqli_stmt_bind_param($stmt, $types, ...$params);
        mysqli_stmt_execute($stmt);
        $result = mysqli_stmt_get_result($stmt);
    } else {
        $result = mysqli_query($handler, $query);
    }
    if ($result)
    {
        return mysqli_fetch_all($result);
    } else {
        return mysqli_affected_rows($result);
    }
}

Now this function can be used with DML queries as well and even with any other type of query, like CREATE TABLE or SHOW PROCESSLIST.

Now you can ask what about errors. It should be no concern of this function at all. All database errors should be converted to PHP errors and treated exactly the same way. To convert mysqli errors to PHP errors you need only one command - mysqli_report(). So in the end your code should look like

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$db_handler = mysqli_connect($dblocal, $dbuser, $dbpass, $dbname)

$query = "SELECT title FROM books WHERE id=?";
$result = my_dbstmt($db_handler, $query, [$bookid]);

if(!$result) echo "Book not found";
echo print $result[0]['title'];

and for the DML query it would be

$query = "DELETE FROM books WHERE id=?";
$result = my_dbstmt($db_handler, $query, [$bookid]);
echo "$result books has been deleted";

Note that you'd better avoid blunt use of persistent connections as it will give you nothing you imagine but only problems.

Answer 2

My $0.02

In my opinion, if you're not going to use OOP, you should use functional programming as this is entirely too much to return from a single function. However, if you're refactoring code I'll assume you need to keep the function signature the same.

Obligatory Formatting Gripe

Some fanatics will tell you that you should absolutely always use the curlies in your loops and conditionals and they will point to the Apple SSL goto fail bug. I'm here to tell you only a Sith deals in absolutes. Apple's goto fail was caused by the conditional being on two lines. If you can fit the entire conditional or loop on a single line you can greatly improve readability and code length and there is no good reason to use the curlies.

Also, in my opinion, the way you have your curlies indented and on the next line is very difficult to read, but at least it's consistent.

Assuming variable types

Your code makes the assumption that EVERYTHING is a string. While you may not be able to go back and change every place that uses this function to pass in an additional parameter, you should at least provide for an optional parameter to provide that information in the future, and preferably update old code as time permits.

Pointless Variable

At the end of your array2typestring() function you are creating a variable and then immediately returning it. You're wasting time storing this in memory and then immediately disposing of it. Just return the result of the implode() instead.

Putting it together

$db_handler = mysqli_connect("localhost", "root", "", "sms") or die();
$query = "SELECT * FROM canned_responses WHERE campaign_id=?";
$params = array(1);
$paramTypes = array("i");
list($result, $numrows, $err) = my_dbstmt($db_handler, $query, $params, $paramTypes);
echo "<pre>"; var_dump($result);

function array2typestring($array, $paramTypes){
    $types = array();

    for($i=0; $i<count($array); $i++){
        $types[] = empty($paramTypes[$i]) ? 's' : $paramTypes[$i];
    }

    return implode('', $types);
}

function my_dbstmt($handler, $query, $params=array(), $paramTypes=array()){   
    $stmt = mysqli_prepare($handler, $query);

    if($stmt===false) return array(false, 0, mysqli_error($handler));

    //First two parameter of mysqli_stmt_bind_param
    $refarg = array($stmt, array2typestring($params, $paramTypes));

    //create array of parameters' references
    foreach ($params as $key => $value) $refarg[] = &$params[$key];

    //bind parameters with dynamic length
    if(!empty($params)) call_user_func_array("mysqli_stmt_bind_param", $refarg);

    $exec = mysqli_stmt_execute($stmt);

    if($exec===false) return array(false, 0, mysqli_stmt_error($stmt));

    $result = mysqli_stmt_get_result($stmt);

    if($result===false){

        //not a result, but successfully executed
        $numrows = mysqli_stmt_affected_rows($stmt);
        return array(true, $numrows, null);

    }else{

        mysqli_stmt_store_result($stmt);
        $data = array();

        while ($row = mysqli_fetch_array($result, MYSQLI_ASSOC)){ 
          $data[] = $row;
        }

        $numrows = count($data);
        return array($data, $numrows, null);
    }
}