2

I've been trying to streamline how I write my MySQLi code with PHP after learning more about SQL injection and safe coding with bind_param. I wrote a function that can estimate what the "resulting" SQL string generated by bind_param might look like. However, the function relies on the splat "..." operator to load all my variables for the estimate and into bind_param. It seems to work, but I want to know if I'm setting myself up for trouble if I deploy it throughout my site.

I borrowed the idea from this answer.

I call the function sqlBind:

function sqlBind($conn, $sql, $types = '', $aVars = [], $bTest = false, $sDescription = '')
{
    if($bTest) { // If test is set, generate what a non-injected SQL might look like
        $backtrace = debug_backtrace()[0];
        $sFile = $backtrace['file'];
        $iLine = (int) $backtrace['line'];
        $tsql = $sql; // not touching the original sql
        if ($types > '') {
            $tVars = [];
            for ($i = 0; $i < sizeof($aVars); $i++) { // Copy the variable values to another array
                if (is_null($aVars[$i])) {
                    $tVars[] = 'NULL';
                } else {
                    $tVars[] = (substr($types, $i, 1) === 's') ? "'$aVars[$i]'" : $aVars[$i];
                }
            }
            $tsql = sprintf(str_replace('?', '%s', $sql), ...$tVars); // I'm not really worried about this splat
        }
        $tsql = trim(preg_replace('/\s+/', ' ', $tsql));
        echo "<script>console.log('SQL: $tsql | $sFile, Line#$iLine' );</script>";
    }
    $stmt = $conn->prepare($sql);
    if($types > '')$stmt->bind_param($types, ...$aVars); // Using splat to inject from an array of many potential types
    $bWorked = $stmt->execute();
    if (!$bWorked) {
        if(!isset($iLine)) {
            $backtrace = debug_backtrace()[0];
            $sFile = $backtrace['file'];
            $iLine = (int) $backtrace['line'];
        }
        $sError = $stmt->error;
        echo "<script>console.log('SQL ERROR: $sError | $sFile, Line#$iLine' );</script>";
    }
    if ($bTest) {
        $sWorked = ($bWorked) ? 'Succeeded' : 'Failed';
        if ($sDescription > '')$sDescription .= ' ';
        echo "<script>console.log('$sDescription$sWorked | $sFile, Line#$iLine' );</script>";
    }
    if (strpos($sql, 'INSERT INTO') !== false) {
        return $stmt->insert_id;
    } else {
        return $stmt->get_result();
    }
}

I call it like this:

$sql = <<<SQL
    INSERT INTO tUShift (iUserKey, iDeskKey, dDate, fStart, fEnd, Comment)
    VALUES (?, ?, ?, ?, ?, ?);
SQL;
$iNew = sqlBind($conn, $sql, 'iisdds', [$uKey, $iDesk, $qdThisDay, $fStart, $fEnd, $sComment], true, 'Shift Insertion');

or

$sql = <<<SQL
    SELECT * FROM tHours
    WHERE iUserKey = ?
        AND dDate = ?
    ORDER BY dDate;
SQL;
$result = sqlBind($conn, $sql, 'is', [$uKey, $qdThisDay]);

It's designed to work even if you don't need to bind anything.

$sql = <<<SQL
    SELECT * FROM tHType
    ORDER BY sCode;
SQL;
$result = sqlBind($conn, $sql);

Also, any other comments to improve my sqlBind function would be greatly appreciated.

bookworm
  • 53
  • 1
  • 7
  • 1
    I'd call this entire function "hacky". You are yet to learn the Most Important Thing in the Entire Programming: separate your logic. Means DO NOT make an Irish Stew in your functions. There is absolutely no reason to have everything stuffed in one single function. You have the actual query execution, optional debugging, error reporting, a hacky way to detect INSERT query (what about REPLACE, INSERT DELAYED, UPDATE returning an id, etc.?), and even OUTPUT to the browser(?!) in a function that's intended to execute a query. – Your Common Sense Feb 24 '23 at 06:35
  • 1
    Better get your inspiration from https://phpdelusions.net/mysqli/simple (especially in making types optional) while everything else should be made elsewhere. Like, if you want debugging - well then, make another function that is called from this one. If you want a function that returns an id - make a separate function that uses this one. And make your error reporting **consistent** in the meaning all error reporting works the same way for the ENTIRE code, not just one function specifically, and made configurable in a **single place** – Your Common Sense Feb 24 '23 at 06:40
  • Sigh. Thank you, Common Sense. I'll look over your link. I do have output to the browser in a separate function, but I changed it for posting here (not sure why?) I've already switched to PDO from other feedback and eliminated the INSERT test - I can get the insert key on the other end. Your feedback is valuable. – bookworm Feb 25 '23 at 01:52
  • Then let me bother you once more, with the article on writing [PDO classes](https://phpdelusions.net/articles/error_reporting) :) – Your Common Sense Feb 25 '23 at 05:49
  • And one more, as you inevitably will move further, and try to create a helper function for insert. So it must be a *separate* function, or rather a class, and I'd suggest you to toy with this one: https://github.com/colshrapnel/BasicTableGateway – Your Common Sense Feb 25 '23 at 05:53

1 Answers1

2

The whole mysqli extension is one big disadvantage. Use PDO whenever you can. But if you already made a decision to use mysqli and you want to continue with it then I highly recommend using either $mysqli_stmt->execute($arrayOfParams) (available as of PHP 8.1) or using $mysqli->execute_query($sql, $arrayOfParams) (available as of PHP 8.2).

Using the splat operator with bind_param() is a surprising hack stemming from how PHP arrays work internally. It won't change anytime soon because that would be a major breaking change. But it's also not really the intended purpose of it. I have not found a way to break it yet, but it doesn't mean that there isn't one nor there won't be one in the future as new features are introduced. It should be safe to use at least until PHP 9, but if you can, use something more sensible as I mentioned above.

I have to say something about your parameter substitution, which I assume is for debugging purposes. I hope it's just a temporary measure that will never make it to a live site. You should never expose such information to the end user. If you need a debugging solution, use one of the popular loggers or write your own one, but store this information in a secure log file on the server. Do not put it in JavaScript. Additionally, all of this complex code is pretty much useless as you could just use a normal debugger and put a breakpoint on the line you want to debug. No need to substitute parameters, log messages or output anything to JavaScript. I highly recommend you stop writing your debugging solution before you waste more time on it.

Dharman
  • 30,962
  • 25
  • 85
  • 135