5
\$\begingroup\$

I use prepared statements often even when I only need to execute the statement once (for security), so I implemented a function to abstract away all the function calls on the mysqli_stmt object, as well as bind_param()'s first argument since as far as my tests show it works identically even when int parameters are marked as strings.

<?php
$conn = new mysqli('localhost', 'name', 'password', 'db');

if ($conn->connect_error)
    die('Connection to database failed: ' . $conn->connect_error);

function stmt($query, $params){

   array_unshift($params, str_repeat('s', sizeof($params)));
   for($i = 1; $i < sizeof($params); $i++){
      $params[$i] = &$params[$i];
   }

   $stmt = $GLOBALS['conn']->stmt_init();
   $stmt->prepare($query);
   $method = new ReflectionMethod('mysqli_stmt', 'bind_param');
   $method->invokeArgs($stmt, $params);
   $stmt->execute();

   if($stmt->error){
      $result = ['error' => $stmt->error];
   } else {
      $result = $stmt->get_result();
   }

   $stmt->close();
   return $result;
}
?>

Usage example:

<?php
   $result = stmt('SELECT * FROM table_name WHERE id IN(?,?,?)', [1,2,3]);
?>
\$\endgroup\$
5
\$\begingroup\$

I tested your code and it does work, so far, so good. This type of code has been written many times, so by searching online you can find a lot of good examples.

The main problem I have with your code is that it is rather difficult to understand. I can work it out, but it is not obvious. Starting with prepending the $params with something, then the very weird: $params[$i] = &$params[$i] loop, followed by the usage of ReflectionMethod normally used for reverse-engineering code.

I prefer more down to earth code for a simple function like this. Something like:

function executeQuery($mysqli, $query, $parameters)
{
    $stmt = $mysqli->stmt_init();
    if ($stmt->prepare($query)) {
        $types = str_repeat("s", count($parameters));
        if ($stmt->bind_param($types, ...$parameters)) {
            if ($stmt->execute()) {
                return $stmt->get_result();
            }
        }
    }
    return ['error' => $stmt->error];
}

Short and sweet. Some notes:

  • I try to use a function name that actually reflects what the function does.
  • I supply the database connection as an argument, for more flexibility. You can use multiple database connections and they don't need to be in the global scope.
  • I check whether the query could be properly prepared.
  • My code differs quite a bit from your code when it comes to binding the parameters. As you can see this is quite straightforward. Using ... to access variable arguments has been available since PHP 5.6 which came out in 2014.
  • By directly returning the results when the execution was successful I know that an error must have occurred when the last line of the function is executed. This therefore also catches other problems.

Personally I would not have expected to get a MySQLi result object out of this function. Because it will always have to be processed. Why not do this processing inside this function? Like this:

function executeQuery($database, $query, $parameters)
{
    $stmt = $database->stmt_init();
    if ($stmt->prepare($query)) {
        $types = str_repeat("s", count($parameters));
        if ($stmt->bind_param($types, ...$parameters)) {
            if ($stmt->execute()) {
                if ($result = $stmt->get_result()) {
                    $rows = [];
                    while ($row = $result->fetch_assoc()) {
                        $rows[] = $row;
                    }
                    return $rows;
                }
            }
        }
    }
    return ['error' => $stmt->error];
}

Now you simply get an array back. I agree that is not much different from returning a MySQLi result, but I am thinking ahead. Suppose you decide to change over from MySQLi to PDO in the future. You can easily recode the function above to work with PDO, but recoding the handling of MySQLi results everywhere in your code will be a lot harder. So I am using the function to abstract away from a particular database interface.

Some people don't like the deep nesting of if () {} blocks. To prevent this you could instead write something, like the code below, for all these blocks:

 if (!$stmt->prepare($query)) {
     return ['error' => $stmt->error];
 }

I have to repeat that there are lots of ways of doing this. The answer I gave is based on the code you presented. It is, for instance, not hard to find out the type of the parameters, and adjust the $types string accordingly.

\$\endgroup\$
  • \$\begingroup\$ Assuming it is available, I'd be generating the returned result set in a loopless manner. Something like: stackoverflow.com/a/51385583/2943403 This will reduce 5 lines to 1. (...and I don't bother with init().) \$\endgroup\$ – mickmackusa Aug 15 at 22:33
  • \$\begingroup\$ Returning an array in case of error is absolutely unacceptable. Trying to fetch the data from a DML query will cause an error. There is a fetch_all(). \$\endgroup\$ – Your Common Sense Aug 16 at 3:24
5
\$\begingroup\$

First of all, that's a very good idea to create such a function. It says you are a programmer in your heart. Sadly, but most PHP users never come to the idea of such an automation, writing thousands of repeated lines of code over and over again.

What could be criticized about your code is already pretty much covered in the other answer. However, the solution offered there is still far from being optimal.

First of all, the error reporting is absolutely flawed in both cases. Returning an array with error information instead of the actual query result is absolutely unacceptable. It will lead to numerous errors and confusions in your code. Errors must be thrown, not returned. For mysqli it's especially simple because it can throw exceptions by itself. Check out my article on PHP error reporting principles

Next, returning the mysqli result may cause an error if you are running a DML query. So the only proper return value would be a mysqli statement.

Using "s" for all data types is a very smart move, it will serve you 999 times out of 1000. However, adding a possibility to set the types explicitly is a good idea anyway.

Taking all the above into consideration, I wrote such a function myself, a Mysqli helper function:

function prepared_query($mysqli, $sql, $params, $types = "")
{
    $types = $types ?: str_repeat("s", count($params));
    $stmt = $mysqli->prepare($sql);
    $stmt->bind_param($types, ...$params);
    $stmt->execute();
    return $stmt;
}

As you can see, it is not only much simpler but also much more flexible. Note the examples section in the article linked above. As you can see, I tested this function with many query types and return values.

On a side note, the proper mysqli connection is a bit more complex than just a single line of code.

\$\endgroup\$
  • \$\begingroup\$ Thanks, very useful articles! About the potential for error when returning result of DML queries: I think it would be better to return either the result or nothing, since you'd expect a result from statements such as SELECT, and in other cases you need nothing returned. You might want to get data such as number of affected rows, but it can be done with $mysqli. I'm not aware of other reasons to want $stmt besides getting the result. \$\endgroup\$ – potato Aug 16 at 14:53
1
\$\begingroup\$

My code after improvement based on the reviews

And another improvement: if no query parameters are passed, run a simple query instead of a prepared statement.

Note: one thing I didn't implement from the reviews is the idea of returning an array instead of the mysqli_result object - because of performance and memory considerations.

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
try {
   $conn = new mysqli('localhost', 'username', 'password', 'db');
   $conn->set_charset('utf8mb4');
} catch (Exception $e) { // catch and re-throw to avoid exposing database credentials
     throw new Exception($e->getMessage(), $e->getCode());
}

function query($mysqli, $sql, $params = [], $types = '')
{
   if(!$params){
      return $mysqli->query($sql);
   }
   $types = $types ?: str_repeat('s', count($params));
   $stmt = $mysqli->prepare($sql);
   $stmt->bind_param($types, ...$params);
   $stmt->execute();
   try{
      $result = $stmt->get_result();
   } catch (Exception $e) {
      $result = false;
   }
   $stmt->close();
   return $result;
}
\$\endgroup\$
  • \$\begingroup\$ If all of your incoming $param values are appropriately typed, you can read their types, avoid passing in a $types argument, and apply the most appropriate type character for each param. Something like this: 3v4l.org/alenU p.s. close is not necessary -- php is going to do this for you. \$\endgroup\$ – mickmackusa Aug 17 at 12:41
  • \$\begingroup\$ Numeric types are processed by MySQL just fine when passed as strings, but b for blobs is indeed useful and can be automated, I'll look into it. \$\endgroup\$ – potato Aug 17 at 13:58
  • \$\begingroup\$ Just a suggestion if you wanted to reduce passed in arguments. Also, I'd probably go for $params = [] instead of null for data type consistency. \$\endgroup\$ – mickmackusa Aug 17 at 14:02
  • \$\begingroup\$ After looking into the use case of blob type I concluded that it is currently not needed for my project (and unlikely to ever be needed) so I won't bother implementing it. It is only useful for big values (exceeding max_allowed_packet) that should be sent in multiple smaller packets via mysqli_stmt::send_long_data(). So right now I'm left wondering if I should just use s for everything and remove the option to set it to anything else for a lack of usefulness, or is it any good for optimization? (numeric types take less space, so it's slightly less bytes to send, but just slightly) \$\endgroup\$ – potato Aug 17 at 15:12
  • 1
    \$\begingroup\$ @potato there are few edge cases \$\endgroup\$ – Your Common Sense Aug 18 at 3:42

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.