PHP MySQLi Wrapper Class For API

Question

I've never had my code reviewed before, so I am taking this opportunity to see what can be improved with my coding style. I am still somewhat "new" to PHP, in the sense that I'm an old dog finally learning new tricks.

Here is a MySQLi wrapper I've coded in PHP. Overall, this code is part of an API project. I'm making an API in PHP which I will use for my WPF Desktop Application for the Database Models.

Here is Class.Config.php:

<?php
namespace Database;
use Exceptions\DatabaseException;
use Exceptions\ConnectionException;
class Config
{
 /**
 * @var string $host The connection host.
 * @var string $username The connection username.
 * @var string $password The connection password.
 * @var string $dbname The connection database name.
 */
 private string $host, $username, $password, $dbname;
 /**
 * @var \mysqli $conn The mysqli connection object.
 */
 private \mysqli $conn;
 /**
 * Configures the Config object with the default connection string.
 *
 * @param array $config The config array, expects indexes of: host, username, password, and dbname.
 * @param mysqli $connection The optional mysqli connection object.
 */
 public function __construct(?array $config = null, \mysqli $connection = null)
 {
 if ($connection !== null && !($connection instanceof \mysqli))
 {
 throw new \InvalidArgumentException(
 "Invalid mysqli connection object passed. Instance of 'mysqli' expected, '" . gettype($connection) . "' given."
 );
 }
 if ($config === null)
 {
 // Retrieve an array of the connection data from the database.ini file.
 $config = parse_ini_file("database.ini");
 }
 // Retrieve the database credentials from the parsed file
 foreach($config as $key => $value)
 {
 $this->$key = $value ?? null;
 }
 // Verify that the config file properly sets up the variables.
 try
 {
 if (!isset($this->host, $this->username, $this->password, $this->dbname)
 || empty($this->host)
 || empty($this->username)
 || empty($this->password)
 || empty($this->dbname))
 {
 throw new \InvalidArgumentException("Connection string expects values for ['host', 'username', 'password', 'dbname']");
 }
 }
 catch (\InvalidArgumentException $e)
 {
 error_log("Missing Configuration Index: {$e}");
 throw $e;
 }
 // Create a new MySQLi connection
 $this->conn =
 $connection ??
 new \mysqli(
 $this->host,
 $this->username,
 $this->password,
 $this->dbname
 );
 }
 /**
 * Validates the connection.
 *
 * @return mysqli The mysqli connection object.
 */
 public function connect()
 {
 try
 {
 /**
 * Checks to see if there are any errors with the validated connection string.
 *
 * @var \mysqli $this->conn The connection object configured in the constructor.
 */
 if ($this->conn->connect_error)
 {
 throw new ConnectionException(
 $this->conn->connect_error
 );
 }
 else
 {
 return $this->conn;
 }
 }
 catch (ConnectionException $e)
 {
 error_log("Connection Error: " . $e->getMessage());
 throw $e;
 }
 catch (DatabaseException $e)
 {
 error_log("Database Error: " . $e->getMessage());
 throw $e;
 }
 catch (Exception $e)
 {
 error_log("Error: " . $e->getMessage());
 throw $e;
 }
 }
}
?>

Here is class.MySQLi.php:

<?php
namespace Database;
use Database\Config;
class MySQLi
{
 /**
 * @var \mysqli $conn The mysqli connection object.
 */
 private \mysqli $conn;
 /**
 * Initialize the connection between the database config.
 */
 public function __construct()
 {
 $conn = new Config;
 $this->conn = $conn->connect();
 }
 /**
 * Method for getting the connection variable.
 *
 * @return \mysqli | bool $this->conn The mysqli object configured in the constructor.
 */
 public function getConnection() : \mysqli | bool
 {
 return $this->conn;
 }
 /**
 * Close the connection once the script ends.
 */
 public function __destruct()
 {
 $this->conn->close();
 }
 /**
 * Build a parameterized query to protect against SQL injections by default.
 *
 * @param string $query The SQL query string with placeholders for parameters.
 * @param mixed ...$params The array? of parameters to be bound to the query.
 * @return \mysqli_result|bool Returns the result of the query or false if an error is encountered.
 */
 public function Query(string $query, ...$params) : \mysqli_result | bool
 {
 // Prepare the SQL statement or trigger an error if preparation fails.
 $statement = $this->getConnection()->prepare($query) or trigger_error($this->getConnection()->error, E_USER_ERROR);
 if (!$statement) return false;
 // Build the parameters if provided.
 if(!empty($params))
 {
 $types = [];
 $bindParams = [];
 foreach ($params as $param)
 {
 // Get the type of each parameter: i, d, s, or b.
 $types[] = $this->getParamType($param);
 $bindParams[] = $param;
 }
 try
 {
 $types = implode("", $types);
 $statement->bind_param($types, ...$bindParams);
 }
 catch (\ArgumentCountError $e)
 {
 return false;
 }
 }
 // Execute the statement and return the result if successful
 if ($statement->execute())
 {
 $result = $statement->get_result();
 $statement->close();
 return $result;
 }
 return false;
 }
 /**
 * Insert rows of data into a database table.
 *
 * @param string $query The SQL query string with placeholders for parameters.
 * @param mixed ...$params The array? of parameters to be bound to the query.
 */
 public function Insert(string $query, ...$params)
 {
 // Still debating usage.
 }
 /**
 * Fetch rows of data from a database table.
 *
 * @param string $query The SQL query string with placeholders for parameters.
 * @param mixed ...$params The array? of parameters to be bound to the query.
 * @return array|false Returns an array of rows on success, or false on failure.
 */
 public function Rows(string $query, ...$params) : array | bool
 {
 if($result = $this->Query($query, ...$params))
 {
 $rows = [];
 while ($row = $result->fetch_assoc())
 {
 $rows[] = $row;
 }
 return $rows;
 }
 return false;
 }
 /**
 * Get the parameter type for binding in a prepared statement.
 *
 * @param mixed $param The parameter value.
 * @return string Returns the parameter type character.
 */
 private function getParamType($param) : string
 {
 if (is_int($param))
 {
 return "i";
 }
 elseif (is_float($param))
 {
 return "d";
 }
 elseif (is_string($param))
 {
 return "s";
 }
 else
 {
 return "b";
 }
 }
}
?>

Usage

Simplify Fetching Rows From Database

With my wrapper class, I can simply do this:

// Get the database connection
$conn = new MySQLi;
// SQL query to select all data from the table
$query = "SELECT * FROM structure_ranks";
// Fetch all rows from the result set and store them in the array
$rows = $conn->Rows($query);
// Output the array as JSON
echo json_encode($rows, JSON_PRETTY_PRINT);

As opposed to doing this:

// Create an instance of the Config class
$config = new Config();
// Get the database connection
$conn = $config->connect();
// Check the connection
if ($conn->connect_error) {
 die("Connection failed: " . $conn->connect_error);
}
// SQL query to select all data from the table
$sql = "SELECT * FROM structure_ranks";
// Execute the query
$result = $conn->query($sql);
// Fetch all rows from the result set and store them in the array
$array = [];
if ($result->num_rows > 0) {
 while ($row = $result->fetch_assoc()) {
 $array[$row['id']] = $row;
 }
}
// Close the connection
$conn->close();
// Output the array as JSON
echo json_encode($array, JSON_PRETTY_PRINT);

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! There is a beginner tag which could be added, if you so choose.
• KIKO Software: Just a small remark. I was reading your code and came accross a $conn variable in several places. In your code $conn can be an instantiation of the mysqli class, or an instantiation of the Config class. So, what does "conn" really mean? Oh, and you also have a $connection variable. I'm confused. I think it is good practice when a variable name uniquely describes what it contains. Same is true for class names. Your Config class actually makes the database connection and your MySQLi class queries it. Those were the best names you could come up with?
• KIKO Software: My point is that naming things properly makes your code easier to read. Readability is more important than shorter code. If a variable name needs 10 characters instead of 5 to be comprehensible and unique, then so be it. Good code starts with choosing good names.
• Bellator: In all use cases, that $conn variable returns this: // Create a new MySQLi connection $this->conn = $connection ?? new \mysqli($this->host, $this->username, $this->password, $this->dbname);, which is from the constructor of the Config class. So, $conn = new Config; just means $conn = new mysqli(...) The config class could've been called something else, but it just sets up and validates the mysqli connection. That one of connection variable was my attempt to say "this connection variable is from the outside, different from everything else going on here"
• KIKO Software: I'm sorry to be stubborn, but $conn = new Config(); isn't the same as $conn = new mysqli() or even $conn = new MySQLi(). The confusion seems real. Anyway, all I wanted to say is that your "naming of things" needs some work, the details are less important. Here's a bit of a backgrounder. Just think what will happen when you need to configure something else? Will you use class Config2?

Answer 1

There is quite a lot of issues and possible improvements. For example, the entire Config class being one big pile of useless cargo cult code that you wrote not because it is really needed but just to try your hand at those "new tricks". Just one quick example. The following condition will never be evaluated to true, being essentially useless:

if ($connection !== null && !($connection instanceof \mysqli))

simply because one line above you already asked PHP to check the $mysqli variable type and throw exception in case it's incorrect:

public function __construct(?array $config = null, \mysqli $connection = null)

Here you can see a live demonstration. You are expecting this code to throw \InvalidArgumentException with custom error message in case $mysqli is not null and not mysqli. But, as you can see, it's another error, which is thrown even before the execution reached that condition.

And it's the case with almost every other exception thrown in your code.

Yet I am not sure whether you are inclined to listen (or even still have any interest in a review at all). So I would rather concentrate on the false assumptions that led you to the creation of these two rather monstrous classes.

The "As opposed to doing this", being rewritten using the correct approach and the most recent PHP version, should be read as

// get the configuration options
$config = require 'config.php';
// Get the database connection
$conn = new mysqli(...$config['db']);
// SQL query to select all data from the table
$sql = "SELECT * FROM structure_ranks";
// Fetch all rows from the result set and store them in the array
$result = $conn->query($sql)->fetch_all(MYSQLI_ASSOC);
// Output the array as JSON
echo json_encode($array, JSON_PRETTY_PRINT);

If you have any questions regarding the code present or not present in this example, feel free to ask in the comments or you may help yourself to the short article on mysqli I wrote.

As you can see, when used properly, mysqli is already as concise as your custom solution. And even with prepared statements:

// SQL query using a variable
$sql = "SELECT * FROM structure_ranks WHERE rank > ?";
// Fetch all rows from the result set and store them in the array
$result = $conn->execute_query($sql, [$rank])->fetch_all(MYSQLI_ASSOC);
// SQL query using a variable
$sql = "INSERT INTO structure_ranks (rank, whatever, foo, bar) VALUES (?,?,?,?)";
// A separate Insert method is indeed of no use
$conn->execute_query($sql, [$rank, $whatever, $foo, $bar]);

The only place for improvement I can see here is the syntax for getting all rows which is indeed rather verbose. So in your place I would focus this custom class on several fetch methods that return different types of array, such as

• fetchRows() just an alias for fetch_all(MYSQLI_ASSOC)
• fetchList() to get values of single columns
• fetchIndexed($index = 'id') that implements the fetching loop from "as opposed" section
• fetchDict() which combines the above two, taking the first selected column for index and the second for value
• fetchCell() just an alias for native mysqli's fetch_column()
• fetch() just an alias for fetch_assoc()

Given you showed some interest in the review, I will add some. A quite good example is

try
 {
 if (!isset($this->host, $this->username, $this->password, $this->dbname)
 || empty($this->host)
 || empty($this->username)
 || empty($this->password)
 || empty($this->dbname))
 {
 throw new \InvalidArgumentException("Connection string expects values for ['host', 'username', 'password', 'dbname']");
 }
 }
 catch (\InvalidArgumentException $e)
 {
 error_log("Missing Configuration Index: {$e}");
 throw $e;
 }
}

which, taken by itself, without any context, can be rewritten to

if (!$this->host || !$this->username || !$this->password || !$this->dbname) {
 $e = new \InvalidArgumentException("Connection string expects values for ['host', 'username', 'password', 'dbname']");
 error_log("Missing Configuration Index: {$e}");
 throw $e;
}

because

• isset() being useless for checking properties that deliberately exist, being defined in the class definition. Here is another live demonstration: even without isset() this code behaves exactly the same.
• empty() being twice useless as it is checking the existence of the existing variable that was already checked for existence by isset
• try being useless because it is used here not on purpose, but for the control flow, which is already done by if statement.

Yet, taking into account the actual context, this code block should be just removed altogether. Because, again, in case your credentials are incorrect, mysql will readily provide the error message telling you that. Besides, this pesky verification just gets in the way. Why can't I provide an empty parameter - or null, to use the default value?

Another vivid example is

 catch (ConnectionException $e)
 {
 error_log("Connection Error: " . $e->getMessage());
 throw $e;
 }
 catch (DatabaseException $e)
 {
 error_log("Database Error: " . $e->getMessage());
 throw $e;
 }
 catch (Exception $e)
 {
 error_log("Error: " . $e->getMessage());
 throw $e;
 }

As we can see, the code is basically duplicated here three times, only to add textual remarks that are rather superfluous. Why not to leave just the Exception case (or Throwable for that matter)?

Yet again, catching exception only to log it makes little sense. PHP already can log errors. Why not to just leave this exception alone and let PHP log it just like any other error?

I honestly, genuinely don't understand, why database errors should receive some special treatment or a supplementary textual remark. Like, never in my life I've seen anyone wrapping a require call in a try-catch that simply adds "Filesystem error:" and throws the exception again. But for some reason many people consider essential to add a try catch for the database call.

A rather small issue: here, the else clause is superfuluous

 {
 else
 {
 return $this->conn;
 }

and usually written as

 }
 return $this->conn;

Finally, like I said in the comments, the if statement here is superfluous too.

 if ($this->conn->connect_error)
 {
 throw new ConnectionException(
 $this->conn->connect_error
 );
 }

Like it is said in the article I linked above, you can always configure mysqli to throw exceptions by itself, which renders all such conditions also useless.

• Bellator: The Config class is designed so that a developer can easily switch between database sources. You call it "cargo cult" code, but my way makes it easy to compare data from two databases. It also aims to alert the developer if the source he provided is accurate, and why not. Not sure how you can state I wrote it without needing it when I wrote it because it was needed.
• Bellator: Your code also has no error handling. If we did a fetch_all on a bad SQL query, we get a fatal error. The functions account for error handling and other things. In the end, this 'simple' code isn't thinking about the future
• Your Common Sense: Not sure what you mean. What is "database source"? Got an example of such "easy switching"? You never mention it in your question and it is not clearly evident from the code. What is the actual use case that proved such a need?
• Your Common Sense: That's a bit peculiar way for asking but thank you anyway. Actually my code does have all the error handling (or, rather, reporting) it needs. And there is much more thinking about future than you can think of. Regarding error reporting, there is a simple configuration option that tells mysqli to report query errors (which is on by default in the current PHP version of 8.2). Which makes all the elaborate error reporting code in your class just useless. Another example of useless code I added to the answer.
• Bellator: I mean, reading the code would tell you what it does. But a simple example is switching between the development environment database and the production environment. As I said before, what if I simply want to use two databases without developing an API? This is a way to do that.