0
\$\begingroup\$

I'm creating some web apis to sync some data between desktop/mobile apps.
Working on it, I wrote a simple wrapper for mysqli connection to simplify querying like:

// examples
$connection = new MySQLiConnection();

$connection->execute("INSERT INTO songs VALUES ('God\'s Plan', 'Drake')");
$connection->execute('INSERT INTO songs VALUES (?, ?)', 'Perfect', 'Ed Sheeran');

$result = $connection->getResult('SELECT * FROM songs');
var_dump($result);

$result = $connection->getResult('SELECT * FROM songs WHERE artist = ?', 'Ed Sheeran');
var_dump($result);


Code of the wrapper:

<?php

final class MySQLiConnection
{
    private $connection;

    public function __construct()
    {
        $this->connection = new mysqli('localhost', 'id', 'password', 'database');

        if ($this->connection->connect_errno)
        {
            throw new mysqli_sql_exception('Failed to open mysqli connection: ' . $this->connection->connect_error);
        }

        if (!$this->connection->set_charset('utf8'))
        {
            throw new mysqli_sql_exception('Failed to set utf8 character set: ' . $this->connection->error);
        }
    }

    public function __destruct()
    {
        if (!$this->connection->close())
        {
            throw new mysqli_sql_exception('Failed to close mysqli connection: ' . $this->connection->error);
        }
    }

    private function buildStatement(string $sql, ...$params) : mysqli_stmt
    {
        if ($statement = $this->connection->prepare($sql))
        {
            if (!empty($params))
            {
                $types = '';

                foreach ($params as $param)
                {
                    if (is_int($param))
                    {
                        $types .= 'i';
                    }
                    elseif (is_float($param))
                    {
                        $types .= 'd';
                    }
                    elseif (is_string($param))
                    {
                        $types .= 's';
                    }
                    else
                    {
                        $types .= 'b';
                    }
                }

                $statement->bind_param($types, ...$params);
            }

            return $statement;
        }

        throw new mysqli_sql_exception('Failed to prepare mysqli_stmt: ' . $this->connection->error);
    }

    public function execute(string $sql, ...$params) : void
    {
        $statement = $this->buildStatement($sql, ...$params);

        $success = $statement->execute();
        $statement->close();

        if (!$success)
        {
            throw new mysqli_sql_exception('Failed to execute mysqli_stmt: ' . $this->connection->error);
        }
    }

    public function getResult(string $sql, ...$params) : mysqli_result
    {
        $statement = $this->buildStatement($sql, ...$params);

        $success = $statement->execute();

        if ($success)
        {
            $result = $statement->get_result();

            $statement->close();
            return $result;
        }

        $statement->close();
        throw new mysqli_sql_exception('Failed to execute mysqli_stmt: ' . $this->connection->error);
    }
}

?>
\$\endgroup\$
0
\$\begingroup\$

Error reporting

This is much better than your initial attempt, but somehow it become more verbose and I'll tell you why: you are, so to say, abused the error reporting. mysqli_sql_exception is not intended to be thrown manually. Like it's said in the article I gave you link to, mysqli can throw exceptions by itself - so just let mysqli to do it. And therefore there is no reason to check for errors manually, and it will greatly reduce the amount of repeated code.

Automatic binding

Your automated binding function is rather confusing. Apparently you didn't have any clear idea what this code is intended to do:

                else
                {
                    $types .= 'b';
                }

And I'll tell you - there is no good scenario for it.
Don't write a code just in case, when you just don't know what to write. In such circumstances just don't write any code at all.

Besides, I am scared by functions like this that sniff a type off a variable automatically. If you will have a query that will compare a string stored in a database to a number, the consequences would be fatal. And PHP is known for converting strings to numbers on its own will. So, although quite small, but there is still a possibility that your binding function will ruin your database.

That's why I prefer either a manual explicit type setting or just blunt setting a string type for all variables. At least it won't harm anyone.

In practice it means that I recommend to use string type for all parameters by default, with a possibility of a fallback with manual type setting. So, taken from ny other article, the mysqli binding function would be like

public function execute(string $sql, $params = [], $types = '') : mysqli_stmt
{
    if (!$params) {
        return $this->connection->query($sql);
    }
    $types = $types ?: str_repeat("s", count($params));
    $statement = $this->connection->prepare($sql);
    $statement->bind_param($types, ...$params);
    $stmt->execute();
    return $stmt;
}

this function does a lot of good stuff:

  • it lets you to run a query that doesn't support prepared statements
  • it does no harm automatically sniffing the variable's type
  • at the same time it lets you to provide a type string manually, like

    $conn->execute('INSERT INTO songs VALUES (?, ?, ?)', ['foo', 'bar', 1], "ssi");
    

I also removed an argument unpacking operator from the function definition as it makes no sense. Trust me, it takes no effort to add two square brackets around parameters, but it makes your code WAY cleaner and more maintainable.

Closing statements

PHP is incredibly programmer-friendly. Resources don't have to be closed manually, they will be closed automatically when no longer needed.

The code

<?php

final class MySQLiConnection
{
    public $connection;

    public function __construct($config)
    {
        mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
        try {
            $this->connection = new mysqli('localhost', 'id', 'password', 'database');
            $this->connection->set_charset('utf8');
        } catch (\mysqli_sql_exception $e) {
            throw new \mysqli_sql_exception($e->getMessage(), $e->getCode());
        }
    }

    public function execute(string $sql, $params = [], $types = '') : mysqli_stmt
    {
        if (!$params) {
            return $this->connection->query($sql);
        }
        $types = $types ?: str_repeat("s", count($params));
        $statement = $this->connection->prepare($sql);
        $statement->bind_param($types, ...$params);
        $statement->execute();
        return $statement;
    }

    public function getResult(string $sql, $params = [], $types = '') : mysqli_result
    {
        return $this->execute($sql, $params, $types)->get_result();
    }
}

As you can see, the final code is much more concise, thanks to code reuse and proper error reporting.

I also made $connection variable public, as there is no more getConnection() function but you will need to access it from outside for sure.

An exception is caught in the constructor because of security concerns. A stack trace for the exception thrown in case of a connection error would contain the database credentials that will end up either in the server logs or - the worst case - displayed on the screen. Once re-thrown this way, credentials are no more exposed. So although as a rule you don't catch an Exception right in place, here it's a special case to take care of.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the nice code. If the error reporting is enabled as you suggested, do mysqli always throw an exception instead of just returning FALSE so that manual handling is never required? And may I ask why do you re-throw the mysqli_sql_exception, are they ignored or do they cause a problem if not caught immediately? \$\endgroup\$ – Taekmin Lee Jan 25 at 17:11
  • \$\begingroup\$ 1.yes. 2. I am doing it for connection only. A stack trace for the exception thrown from connect would contain the database credentials. Once rethrown it would not. So it's a special case to take care of \$\endgroup\$ – Your Common Sense Jan 25 at 17:45
0
\$\begingroup\$

PSR 2

Psr2 is a code standard for php use it, most ides have good integration to auto-reformat your code read about it here

Generally

Wrapping libraries for databases is a bad idea, how do I start a transaction in your class ?

@Yourcommonsense will im sure provide a link for his website demonstrating why this is a bad idea.

Return early && Use Constants

There is no point having long statements that cause the indentation level to be excessive,

Use constants for random strings because they mean nothing to anybody but you, with a good name (my names probably aren't good because I dont know that mean) it can speak a million words,

For example

private const INT_PARAM_STRING = "i";
private const FLOAT_PARAM_STRING = "d";
private const STRING_PARAM_STRING = "s";
private const DEFAULT_PARAM_STRING = "b";

private function buildStatement(string $sql, ...$params) : mysqli_stmt
{
    if (empty($params)) {
        throw new \Exception("Empty params", 1);
    }

    $statement = $this->connection->prepare($sql);

    if ($statement == false) {
        throw new mysqli_sql_exception('Failed to prepare mysqli_stmt: ' . $this->connection->error);
    }

    $types = '';

    foreach ($params as $param) {
        $types .= $this->getParamType($param);
    }

    $statement->bind_param($types, ...$params);

    return $statement;
}

private function getParamType($param) :string
{
    if (is_int($param)) {
        return MySQLiConnection::INT_PARAM_STRING;
    } elseif (is_float($param)) {
        return MySQLiConnection::FLOAT_PARAM_STRING;
    } elseif (is_string($param)) {
        return MySQLiConnection::STRING_PARAM_STRING;
    }

    return DEFAULT_PARAM_STRING;
}
\$\endgroup\$
  • \$\begingroup\$ A little correction, it is not an error when params are empty. \$\endgroup\$ – Your Common Sense Jan 25 at 16:05
  • \$\begingroup\$ Thank you. I do agree with you that wrapping a library is generally considered bad, but my intention was just to avoid the code being lengthy. Without any wrapper, it would take several lines just to create and execute a prepared statement in mysqli. And as you asked about the transaction, I should've created a getter of the $connection. \$\endgroup\$ – Taekmin Lee Jan 25 at 16:54

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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