MySQL connection using a static connection

Question

I have been using a static class to connect and retrieve information from a MySQL database and also reading about how static works in php. I'm still not sure if the code is bad or not and why (besides the usual static are dangerous). Also, code improvements suggestions will be appreciated.

class Database {

    const STOREDPROCEDURE = 1;
    const ADHOC = 2;

    private static $instance;

    private static function call() {

        if (!isset(self::$instance)) {
            self::$instance = new MySQLi('localhost', 'root', '', 'alfred');
            if (self::$instance -> connect_error) throw new Exception(self::$instance -> connect_error);
            self::$instance -> set_charset('utf8');
        }

        return self::$instance;

    }

    public static function query() {

        //
        $arguments = func_get_args();
        $count = count($arguments);

        //
        $references = array();
        for ($i = 2; $i <= count($arguments) - 1; $i++) $references[$i] = &$arguments[$i];

        //
        $query = $count === 0 ? '' : str_repeat('?, ', $count - 4) . '?';
        $query = 'CALL' . ' ' . $arguments[1] . '(' . $query . ');';

        //
        $stmt = self::call() -> prepare($query);
        if ($count > 2) call_user_func_array(array($stmt, 'bind_param'), $references);

        //
        $stmt -> execute();
        $stmt -> store_result();

        //
        $result = array();
        while ($metadata = $stmt -> result_metadata()) {

            //
            $fields = array();
            foreach ($metadata -> fetch_fields() as $field) {
                $reference[$field -> name] = '';
                $fields[$field -> name] = &$reference[$field -> name];
            }

            //
            call_user_func_array(array($stmt, 'bind_result'), $fields);

            //
            $rows = array();
            while ($stmt -> fetch()) {
                $row = new stdClass();
                foreach ($fields as $name => $value) $row -> $name = $value;
                array_push($rows, $row);
            }

            //
            array_push($result, empty($rows) ? null : $rows);

            //
            $stmt -> next_result();

        }

        //
        $stmt -> free_result();
        $stmt -> close();

        //
        return $result;

    }

}

You can use the code by doing:

        $response = Database::query(
            Database::STOREDPROCEDURE,
            'UserRead',
            'sss',
            $useremail,
            $userpassword,
            $userhash
        );

Answer 1

Right now, it does not seem possible to run every type of query. For example: "SHOW FULL PROCESSLIST" or just "SELECT id FROM xyz LIMIT 0,10". Apart from this, there are a lot of things which might possibly go wrong.

What if I call the function with just one argument? arguments[1] would not exist and the query would become CALL (?). This statement is prepared and executed, but there is nothing executed.

I would suggest keeping things more easy instead of trying to merge several things together.

1. create a single query method which just runs "ADHOC" queries (no prepare)
2. create a prepare statement where you can bind parameters to the query (and execute)

Answer 2

DO NOT REINVENT THE WHEEL

Especially when it comes to such sensitive stuff as database.

If pure PDO or simple wrapper like this is not enough then look for ORM or Active Record implementation.

Since it is code review let's do some code review as well :).

You are using MySQLi object (Not going to mention reinventing the wheel anymore). I suppose it is mysqli wrapper. But what about when somebody wants to connect to other database type than MySQL? You should rename it to Connection or something other that does not give out internals of this class.

What is up with all those //? Delete them.

During refactoring of this code i realized that no matter what I do readability will not reach acceptable level. It's because MySQLi function result_metadata() should not be iterated upon. Intuitively you should only iterate over fetch() results to get all the rows.

Basicly thats how this function should flow:

1. Prepare query (prepare parameters).
2. Iterate over fetched results creating objects.
3. Return result.

This code interrupts the flow and you should refactor it heavily or write it from scratch with different approach:

$references = array();
for ($i = 2; $i <= count($arguments) - 1; $i++) $references[$i] = &$arguments[$i];

$query = $count === 0 ? '' : str_repeat('?, ', $count - 4) . '?';
$query = 'CALL' . ' ' . $arguments[1] . '(' . $query . ');';

$stmt -> execute();
$stmt -> store_result();

while ($metadata = $stmt -> result_metadata())

$stmt -> next_result();
$stmt -> free_result();
$stmt -> close();

Basicly this function should call 3-4 private functions that do exactly what is outlined in points above.