2
\$\begingroup\$

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")}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1 for the Sith... I don't use mysqli but when I was researching for my answer I looked at mysqli_fetch_all and several people in the manual were suggesting using the loop structure instead for either speed or because of limited support. Care to elaborate? \$\endgroup\$ – I wrestled a bear once. Aug 3 '17 at 11:09
  • \$\begingroup\$ 1. Good catch on the mysqli_fetch_all() 2. str_repeat() does not exactly the same, it would add one comma too much at the end which could be hacked by substr() of course 3. I don't think $numrows is useless, because $result will be empty when there is an delete/update/insert. \$\endgroup\$ – bart Aug 3 '17 at 20:40
  • \$\begingroup\$ @bart comma? Substr? What on the earth you are talking about? Did you bother to look at the code at all? \$\endgroup\$ – Your Common Sense Aug 3 '17 at 20:48
  • \$\begingroup\$ @YourCommonSense Oops, my wrong, I was mixing it up with generating the ?,?,?,? string. \$\endgroup\$ – bart Aug 3 '17 at 20:53
  • 1
    \$\begingroup\$ @bart add damned THREE DOTS \$\endgroup\$ – Your Common Sense Aug 4 '17 at 23:26
1
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why is the FOR loop you suggest better than the FOREACH loop? Seems more complex. \$\endgroup\$ – bart Aug 3 '17 at 6:00
  • 1
    \$\begingroup\$ @bart It's not that it's better but I added the second optional array of variable types and both of those arrays need to be looped simultaneously (if they both exist) so we need the index. Sure you could have gotten it from the for each loop loop but my brain is just trained to think of for each loops as being associated with associative arrays (which is not accurate). In short, there is no real reason I changed the loop type, just force of habit. \$\endgroup\$ – I wrestled a bear once. Aug 3 '17 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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