4
\$\begingroup\$

I wrote a list of functions in PHP that I want to use in my pet project as an autoloaded file for all parts of the app. The purpose of these function is to shorten the code you write for MySQL queries in PHP using MySQLi. It also uses prepared statements to make it secure. Is this code alright? What improvements can I do to it?

<?php

// Insert function
function insert_db($table, $columns, $values, $return_id = false)
{
  global $db;

  $question_marks = "";
  $type_strings = "";

  for ($i = 0; $i < count($columns); $i++) {
    if ($i == count($columns) - 1) {
      $question_marks .= "?";
    } else {
      $question_marks .= "?,";
    }
    $type_strings .= "s";
  }

  $columns_string = implode(",", $columns);

  $sql = "INSERT INTO `$table` ($columns_string) VALUES ($question_marks)";
  $stmt = $db->prepare($sql);
  $stmt->bind_param("$type_strings", ...$values);
  $stmt->execute();

  if ($return_id === true) {
    return $db->insert_id;
    $stmt->close();
  } else {
    $stmt->close();
  }
}

// Read function
function read_db($table, $columns, $where, $type_strings, $values)
{
  global $db;
  $data = [];
  $complete_data = [];

  $columns_string = implode(",", $columns);
  $sql = "SELECT $columns_string FROM `$table` $where";
  $stmt = $db->prepare($sql);
  if (is_array($values)) {
    $stmt->bind_param("$type_strings", ...$values);
  }
  $stmt->execute();
  $result = $stmt->get_result();

  while ($row = $result->fetch_assoc()) {
    array_push($data,
      $row
    );
  }

  $complete_data = [$data, $result->num_rows, $stmt->affected_rows];

  return $complete_data;
}

// Update function
function update_db($table, $columns, $values, $where, $where_values, $type_strings)
{
  global $db;
  $sql = "UPDATE `$table` SET ";

  $question_marks = "";
  $type_length = strlen($type_strings);

  for ($i = 0; $i < $type_length; $i++) {
    if ($i == ($type_length - 1)) {
      $question_marks .= "?";
    } else {
      $question_marks .= "?,";
    }
  }

  for ($i = 0; $i < count($columns); $i++) {
    if ($i == (count($columns) - 1)) {
      $sql .= "$columns[$i]=? $where;";
    } else {
      $sql .= "$columns[$i]=?,";
    }
  }

  $data = array_merge($values, $where_values);

  $stmt = $db->prepare($sql);
  $stmt->bind_param("$type_strings", ...$data);
  $stmt->execute();
}

// Delete function
function delete_db($table, $where, $values, $type_strings) {
  global $db;
  $sql = "DELETE FROM `$table` $where";
  $stmt = $db->prepare($sql);
  $stmt->bind_param("$type_strings", ...$values);
  $stmt->execute();
}

// It technically became IRUD instead of CRUD, but for function naming create_db would confuse developers so I chose this :P

?>

Edit: I am also including some test cases for the functions,

$admissions_data = read_db("admission_records", ["*"], "WHERE organization_id = ? AND branch_id = ? AND admission_id = ?", "sss", [$organization_id, $branch_id, $record_id]);

insert_db("products", ['product_name', 'product_url', 'product_proof', 'company_id'], [$product_name, $product_url, $product_proof, $products_data[0][0]['company_id']]);

You get the idea now.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ stackoverflow.com/q/17226762/2943403 \$\endgroup\$ Aug 2, 2022 at 12:07
  • \$\begingroup\$ @mickmackusa The PHP 8.1 feature seems great, I have PHP 7.4 installed right now, need to upgrade. Thank you :) \$\endgroup\$ Aug 2, 2022 at 12:27
  • 1
    \$\begingroup\$ In insert_db, if $return_id === true, the $stmt->close() doesn't get executed after the return, unless php does some magic I don't know about. This is probably ok, as $stmt should be cleaned up when it goes out of scope anyway, but putting the close after the return is still not consistent. \$\endgroup\$ Aug 2, 2022 at 18:55
  • \$\begingroup\$ @GuntramBlohm Thank you, yes, one of the answer mentions about the $stmt->close statement as well, I'll get rid of it in my original code. \$\endgroup\$ Aug 2, 2022 at 18:56

4 Answers 4

1
\$\begingroup\$

The code is decent - simple to read, though the comments above the functions don't add much. It is a good idea to follow the PHP Standards Recommendations - and there is currently a draft for the PHPDoc Standard. Instead of a single line comment above functions it would be more informative to have a Multiline DocComment listing parameters with types, return type, etc. Instead of simply

// Insert function
function insert_db($table, $columns, $values, $return_id = false)

It would be more helpful for anyone reading the code to see something more descriptive:

/**
 * Insert data into the database
 *
 * @param string $table
 * @param array $columns
 * @param array $values
 * @param bool $return_id
 * 
 * @return integer|void
 */
function insert_db($table, $columns, $values, $return_id = false)

One major advantage is that many IDEs will index those docblocks and provide suggestions based on them during code development.


The closing PHP tag can be omitted when the file only contains PHP - e.g. no HTML exists outside the PHP code. This is supported by PSR-12.


As I explained near the beginning of this review the loops used to create the placeholders can typically be simplified using the built-in PHP functions implode() and array_fill(). Then there is no need to have conditional logic to append separators like commas.

\$\endgroup\$
0
4
\$\begingroup\$

Just from a pure useability stand, I don't find these types of wrappers very useful. They work fine for the simplest test cases, but as soon as you want to do a basic join you're back to writing the query by hand. You end up spending a lot of time expanding and customizing the wrapper, but as you said, this is for a pet project so knock yourself out; it is a good learning exercise.

Overall the code looks pretty good. Just some minor nitpicks.


global $db;

Global variables are generally seen as a no-no. You should pass the database connection in.


$question_marks

Most "professional" codebases are camelCased instead of snake_case, but that is a personal preference.


$question_marks = "";

I normally stick with single quotes for string literals. Double quotes are reserved when you want to transpose variables inside the string. Ex. echo "Hi $username."


if ($return_id === true) {
    return $db->insert_id;
    $stmt->close();
}

The close statement will never fire because the return statement will return control to the calling code.


$complete_data = [$data, $result->num_rows, $stmt->affected_rows];

return $complete_data;

You can just return the array without assigning it to a variable. Ex. return [...];


There is no error handling in the functions. What happens if you pass a malformed where statement? Will the calling code have to check and log it each everywhere?


If you're using a modern(ish) version of PHP you should type-hint the return of the function. Ex. function read_db(...): array

\$\endgroup\$
2
  • \$\begingroup\$ and no, it doesn't do any input validation at the moment which is a good point. \$\endgroup\$ Aug 2, 2022 at 12:30
  • 2
    \$\begingroup\$ What is modern-ish PHP? Do you mean PHP 7+? \$\endgroup\$
    – Dharman
    Aug 3, 2022 at 12:35
3
\$\begingroup\$

It's good that you are using prepared statements. That's something that should be a no-brainer and yet it's something people often get wrong.

However, there are a lot of weird things in this code. Let's enumerate:

  • What does this function return if $return_id is false? Why can it return two different results? A function accepting a boolean parameter is usually a sign that it should be two separate functions. Please rething the design.
  • Why do you put a variable inside a string like this "$type_strings"? If it's a string already then this just becomes a useless string interpolation. If it's not a string then you will get an error, which makes this also useless. Just remove the quotes around the variable.
  • Do not call $stmt->close() unless you have a very good reason to do so. Calling it at the end of the method is especially useless. You created the object in the current scope and it never left that scope. Once PHP finishes execution of this function, the local variables are destroyed if they are not referenced outside the scope. Closing the statement becomes completely unnecessary as when the object is destroyed everything related to the statement is cleaned up anyway.
  • Please declare types for your parameters and return values. It not only prevents errors but also makes it easier to read and understand the code.
  • If you know that a variable is boolean (which you would if you declared parameter type) then there's no need to compare it to true like you did here $return_id === true.

If we consider the function from a design point of view, we realize that this function should not exist. It doesn't follow SOLID principles. It does more than one thing. It has no clear return type - does it only cause side effects or does it produce results? It relies on global values. If you wanted to execute an UPDATE statement, you would need to copy part of the code. If you wanted to execute INSERT SELECT FROM ... then you cannot use it anymore.

On the surface, it looks like you need this function. But that's only because there's a lot of unnecessary code. If we try to simplify it, we quickly realize that the function should either be split into other functions or shouldn't exist at all. Here's a simplified mysqli version (PHP<8.1):

function insert_db(mysqli $db, string $table, array $mapOfColsToVals): void
{
    $question_marks = str_repeat('?,', count($mapOfColsToVals) - 1) . '?';
    $columns_string = implode(",", array_keys($mapOfColsToVals));
    $stmt = $db->prepare("INSERT INTO `$table` ($columns_string) VALUES ($question_marks)");
    $stmt->bind_param(str_repeat('s', count($mapOfColsToVals)), ...array_values($mapOfColsToVals));
    $stmt->execute();
}

insert_db($db, 'table', ['col1' => 'val1']);
$id = $db->insert_id;

However, if we use PDO or if you upgrade to PHP 8.1, we can make it shorter even more.

function insert_db(mysqli $db, string $table, array $mapOfColsToVals): void
{
    $question_marks = str_repeat('?,', count($mapOfColsToVals) - 1) . '?';
    $columns_string = implode(",", array_keys($mapOfColsToVals));
    $db->prepare("INSERT INTO `$table` ($columns_string) VALUES ($question_marks)")
        ->execute(array_values($mapOfColsToVals));
}

As you can see, the function does two things: it prepares INSERT SQL and then executes it. Given that the SQL preparation should ensure that the values are properly filtered out to avoid SQL injection, we should consider making it a function of its own.

const COLUMN_MAP = ['table' => ['col1', 'col2', 'col3']];

function prepare_insert_SQL(string $table, array $mapOfColsToVals): string
{
    $allowedColumns = COLUMN_MAP[$table] ?? throw new Exception("Unknown table");
    foreach (array_keys($mapOfColsToVals) as $col) {
        if (!in_array($col, $allowedColumns, true)) {
            throw new Exception("Column '$col' is not allowed list of column names");
        }
    }
    $question_marks = str_repeat('?,', count($mapOfColsToVals) - 1) . '?';
    $columns_string = implode(",", array_keys($mapOfColsToVals));
    return "INSERT INTO `$table` ($columns_string) VALUES ($question_marks)";
}

$sql = prepare_insert_SQL('table', ['col1' => 'val1']);
// Execute SQL using the usual prepare-execute functions
\$\endgroup\$
4
  • \$\begingroup\$ Great answer, thanks. Referring to your comment, "What does this function return if $return_id is false? Why can it return two different results? A function accepting a boolean parameter is usually a sign that it should be two separate functions. Please rething the design." $return_id is a variable that contains the id (primary key) of the inserted record in the table. Sometimes, you would only want to do an insert and carry on, other times, you would want to insert a record in the database and get its primary key, $return_id is for those cases. \$\endgroup\$ Aug 3, 2022 at 14:39
  • \$\begingroup\$ Brilliant answers from everyone though, I'll surely consider all these tips and make use of them in improving my code. \$\endgroup\$ Aug 3, 2022 at 14:40
  • \$\begingroup\$ The function can return the ID every time, but you can ignore it when you don't need it. However, as the ID is actually exposed by mysqli object, you don't need to return it from your function. \$\endgroup\$
    – Dharman
    Aug 3, 2022 at 14:42
  • \$\begingroup\$ Hmm, you do have a point actually, now that I think of it, that block of code seems useless. \$\endgroup\$ Aug 3, 2022 at 14:44
1
\$\begingroup\$

I totally agree with the other answer, this kind of functions is anything but usable. This code is simply unreadable.

During my ten years on Stack Overflow I've seen hundreds of functions like this, and still, for the life of me, I am unable to understand, why someone would voluntarily ditch that concise, eloquent, ubiquitous, universally understood SQL like this

SELECT * FROM admission_records 
WHERE organization_id = ? AND branch_id = ? AND admission_id = ?

and exchange it for gibberish

"admission_records", ["*"], "WHERE organization_id = ? AND branch_id = ? AND admission_id = ?

When I start reading it, a lot of questions get on the way. What is admission_records? Is it a column name? Probably not. Why ["*"] is array? Do column names get wrapped in backticks? If not, I cannot use a keyword as a column name. But what if I am bound to work with a legacy database that does? Can I use a function in the column list, like, DATE(datetime) as date? Does this function support ORDER BY? If so - what's the syntax? LIMIT? GROUP BY? JOIN?

Above all, I don't understand - why? What gives? I really, genuinely don't understand the profit. Is to spare yourself from typing a few SQL keywords? Seriously?

Why not let read_db() to accept just SQL string and an array with parameters, making it 1000 times more flexible and readable?

The same goes for insert_db(). Although a helper function for the INSERT query is much more sensible, in its current form the only benefit is saving yourself typing a few placeholders and SQL keywords.


Among other things, there is a lot of duplicated code, that repeats in the every function, like

$stmt = $db->prepare($sql);
$stmt->bind_param("$type_strings", ...$values);
$stmt->execute();

or that placeholder string creation code, that although must be changed for implode, but just for sake of example - if you have a block of code that is used in many places, then create a helper function and call it instead.


Last but not least. All these functions are essentially prone to SQL injection. I know, you intend to write all table and column names by hand. This is not what I mean either. I didn't say that "the way you intend to use these functions is unsafe". I am talking of these functions. When you project grows big, you will hire someone to add a nice SPA interface, and this someone would think: "great, I already got a JSON array from JS" and then do something like

insert_db($json['table'], array_keys($json['data']), array_values($json['data']));

And get SQL injection. Note that they would follow your own principle - less typing justifies everything. Leaving these functions as is will put you in a constant danger.


Let me suggest you another approach, which is similar to yours but safer and requires less typing.

It will require some OOP but no more than native mysqli does. But it will make your code less verbose and SQL injection proof.

Compare yours

update_db('products', ['company_id'], [$cid], 'id=?', $id, "s");
delete_db('products', 'id=?', $id, "s");

with

$productGw->update(['company_id' => $cid], $id);
$productGw->delete($id);

This I would call less boilerplate code.

And you can even supply the array for update directly from the client - there will be no SQL injection.

If you're interested, here is the prototype to learn from.


A bit if a critical review for the other answers: those type hinting dock blocks are totally obsoleted now. PHP got native type declarations, no need for the boring dockblocks

Avoid returning multiple data like this [$data, $result->num_rows, $stmt->affected_rows]. Not only it makes the code dirty but also just useless. It's impossible to have a function that can return either $data and $stmt->affected_rows. While num_rows is just useless, you can always simply have count($data) for this.

function insert_db(string $table, array $columns, array $values, bool $return_id = false): ?int

does exactly the same without much typing and understood not only by your IDE but by PHP itself.

Considering PHP language a professional codebase. snake_case is used for the mosr internal PHP variables. What is more important to have a consistent style for the entire code.

\$\endgroup\$
8
  • \$\begingroup\$ The existing syntax (besides the updates in PHP 8) is anything but eloquent and concise, I am sorry, have to disagree with that ::P it should be a single liner. \$\endgroup\$ Aug 14, 2022 at 16:39
  • \$\begingroup\$ Though I agree my code is still dumb in many aspects that you mentioned. \$\endgroup\$ Aug 14, 2022 at 16:39
  • \$\begingroup\$ "DATE(datetime) as date? Does this function support ORDER BY? If so - what's the syntax? LIMIT? GROUP BY? JOIN?" I guess you didn't go through the code properly or the code is unreadable? (properly the latter case) but yeah, it can do those things, but it is a bit dirty, you have to insert the order by and joins in the WHERE parameter. \$\endgroup\$ Aug 14, 2022 at 16:40
  • \$\begingroup\$ You seem to miss a lot of points from my questions, I already mentioned that this was a pet project :/ \$\endgroup\$ Aug 14, 2022 at 16:42
  • \$\begingroup\$ "What is admission_records? Is it a column name? Probably not" IT IS! it was a test example case as I mentioned in the comment above that. Do you even read questions fully or just give an overview? \$\endgroup\$ Aug 14, 2022 at 16:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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