Preparing and executing MySQL SELECT query from parameters

I am trying to make a function for Select statements. It will give result based from the parameters passed. I tried using it and it gives the results I am expecting.

I know that it is still unpolished but I would like to know what are the critical things in it that should be fixed or improved so that it would be ready for usage.

Below is the parameter passed:

$parameters = array(
    'select' => 'user_id, username, password',
    'type' => 'i',
    'from' => 'users',
    'where_column' => 'user_id',
    'where_row' => '4'
);

And here is the function:

```
function select_query($parameter){
require('database.php');

$select = $parameter['select'];
$type = $parameter['type'];
$from = $parameter['from'];

$columns = '';
$column_array = explode(', ', $select);
foreach ($column_array as $value) {
$columns .= '$'. $value .', ';
}

//prepares the WHERE section of the SQL statement if the WHERE parameter exist
$where = '';
$wc = '';
$wr = '';

if(isset($parameter['where_column']) && isset($parameter['where_row'])){
$wc = $parameter['where_column'];
$wr = $parameter['where_row'];

$where = " WHERE $wc = ?";
}

/ create a prepared statement /
if ($stmt = $conn->prepare("SELECT $select FROM $from". $where)) {

if($where != ''){
/ bind parameters for markers /
$stmt->bind_param($type, $wr);
}

/ execute query Returns TRUE on success or FALSE on failure. /
$stmt->execute();

/ bind result variables Returns a resultset or FALSE on failure. /
$res = $stmt->get_result();

/ fetch value /
if($res->num_rows > 0){
while($row = $res->fetch_assoc()){ // fetch_assoc returns NULL if there are no more rows in resultset.
$results[] = $row;
}
return $results;
}else{
return false;
}

Right off the bat, there are a couple of things about your code that make me feel uneasy. I'll start by running through your code, almost line by line, explaining what I feel could be done to improve on what you have right now:

function select_query($parameter){
    require('database.php');

    $select = $parameter['select'];
    $type = $parameter['type'];
    $from = $parameter['from'];

For starters, you declare a function, that takes just one single argument. What type the function expects is an unknown, unless you actually bother to check the code of the function itself. $parameter (not the best name for the argument, IMHO), is expected to be an array, so your function signature should reflect that:

function selectQuery(array $parameter)
{
}

That would tell anyone who wants to use your function that they need to pass an array, not a string or object. If they pass anything else, they'll get a fatal error.

Next up, your function starts by require-ing a file. Not require_once, or checking if the file has already been processed by PHP, it just requires it. The result will probably be a lot of disk IO (which is bad for performance), and a function with an external dependency. Suppose someone opened the database.php file, and decided to change a thing or two. The variables declared within that file might change names, and you're left with a function that doesn't work anymore, for no apparent reason. To me, it seems like all you really need from that database.php file, is a mysqli connection object (or PDO, but since you've tagged your question mysqli, I'm assuming that that's what $conn is).

A far more common (and all-round better) way to obtain a DB connection would be to have the caller pass the connection of their choice to your function, again, as an argument. Here, you can use type-hints, too, to ensure the argument is indeed a mysqli instance/resource:

function selectQuery(mysqli $conn, array $parameter)
{}

That way, the caller can choose what DB should be used to query, what user, and -crucially- the caller can decide to re-use the connection for other tasks (other queries), and your function is no longer responsable for closing the connection, that falls to the caller.

The last thing about this piece of code that I'd change is this:

$select = $parameter['select'];
    $type = $parameter['type'];
    $from = $parameter['from'];

Yes, using type-hints, you can ensure that the caller can only pass an array to your function, but an array is actually a rather vague thing, especially in PHP. Consider this:

function needsArray(array $argument)
{
    return $argument['total'] - $argument['costs'];
}

Now, these attempts at calling this function will fail:

needsArray(123);
needsArray(null);
needsArray('string');
needsArray(new stdClass);

But these are, going by the signature, correct invocations of the function:

needsArray([]);
needsArray((array) new stdClass);
needsArray(['total' => 'some string', 'costs']);
needsArray(['total' => 123456, 'costs' => 60]);

Yet, only the last call is actually valid, the others will emit notices (undefined index), or won't do anything even remotely meaningful.

At the very least, you'll have to check for every expected key in the $parameter array using isset or array_key_exists, and then you'll still have to ensure that the value of each key is what you expect it to be. That's going to add a lot of silly-looking if's and else's to the code. What's more: if one of the keys doesn't exist, or doesn't contain the type of data you expect it to contain, what do you do? You'll probably have to throw an exception. Let's take our dummy needsArray function, and re-write it in a safe way. Note that we're only expecting a 1D array, with just 2 keys, but just look at the amount of added code we need to add to ensure the array looks the part:

function needsArray(array $argument)
{
    if (!isset($argument['total']) || !is_numeric($argument['total'])) 
    {
        throw new InvalidArgumentException(
            sprintf(
                '%s expects array argument with key "total" set to a numeric value'
                __FUNCTION__
            )
        );
    }
    if (!isset($argument['costs']) || !is_numeric($argument['costs'])) 
    {
        throw new InvalidArgumentException(
            sprintf(
                '%s expects array argument with key "costs" set to a numeric value'
                __FUNCTION__
            )
        );
    }
    return $argument['total'] - $argument['costs'];
}

That's just an awful lot of clutter. Especially if you consider the fact that it's more than likely that the array you're passing to this function will be passed to several other functions, each of which might perform the same checks over and over.

TL;TR

If your function requires something (like a DB connection), don't have your function literally require a file to resolve its dependencies:

StackExchange Code Review Q#91555, answer score: 4