Database output function

Question

Please review the following code and list all the coding errors and poor coding practices that you can see in this code.

function output()
{
    // Check authorization
    if(is_authorized())
    {
        $authorized = true;
        include('/path/to/' . $_REQUEST['module'] . '.php');
    }
    echo "<ul>"
    $conn = mysql_connect( "dbserver.com:6789", "dbyuser", "dbpassWd" );
    mysql_select_db( "kum", $conn ); // selects a database
    $q = " SELECT * FROM main WHERE id > " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        echo "<li>".$row['description']."</li>";
    }
    echo "</ul><br><ul>";
    $q = " SELECT * FROM main WHERE id < " . $_GET["id"]. ";";
    $res = mysql_query( $q, $conn);
    while( $row = mysql_fetch_assoc( $res ) )
    {
        // Display the status if it is authorized, otherwise display N/A
        echo "<li>".$row['description']. "(" .
        $authorized ? $row['status'] : "N/A" . ")</li>";
    }
    echo "</ul>";
}

Answer 1

showerhead and haylem covered this question quite thoroughly, but there's a few more things I think should be added or expounded upon.

Be extremely careful when including based on user input

This was mentioned a few times by both people mentioned, but considering how big of security hole it can be, I feel that it deserves a lot more attention than anyone gave it.

You should NEVER include a file blindly based on user input.

What if I go to: page.php?module=../some/secret

Suddenly /path/some/secret.php just got included. The .php extension would limit a lot of the damage this could do (page.php?module=../path/to/some/config.ini), but the hole is still there.

I tend to be extremely strict with file name handling. I typically only allow a-z, A-Z, 0-9, -, and _. That way you know that it will be a semi-logical file name, and you know that it will not be able to traverse directories.

For example:

$module = (isset($_GET['module']) && is_string($_GET['module'])) ? $_GET['module'] : null;
if (!preg_match('/^[a-zA-Z0-9_-]$/', $module)) {
    $module = false;
}

You should also check if the file is readable before including it. Otherwise, a user may see an ugly error message, or, best case, you open up the possibility of a huge log file of meaningless errors. (Production servers should never have display_errors = On, but they should have error_reporting = E_ALL.)

Don't assume (user provided) array keys are set

showerhead touched on this:

Also, I would not immediately use any value from any user provided array directly in my program. It should be sanitized and validated first.

But I think one of the reasons behind this should be explained a bit more.

When you access an array, if the key does not exist, then a notice is triggered about the array key not existing. As such, you should always make sure you are 100% certain that an array key exists. The only way to do this on user controlled inputs is explicity checked. The filter_input method is a favorite of many, but if you wanted to it "raw", you can use isset.

$val = (isset($_GET['val'])) ? $_GET['val'] : null;

It's guaranteed that every value of a $_GET/$_POST/etc array will be either a string or an array, thus it's a good idea to check type too. Using an array as a string will trigger quite a few errors in most scripts.

$val = (isset($_GET['val']) && is_string($_GET['val'])) ? $_GET['val'] : null;

array_key_exists is worth mentioning here too. It does basically the same thing, except it can be used when a value in an array may be false.

$a = array('foo' => null);

if (isset($a)) { echo 'isset'; }
if (array_key_exists('foo', $a)) { echo 'exists'; }

exists will echo here, but isset will not since isset(null) === false.

Separation of concerns

showerhead explained the concept well, but I disagree with his execution a bit.

As part of having only one responsibility, a function should have no inner depencies. What I mean by this is that a function that is not related to direct database interaction should not even have to know that a database exists. (Obviously at some point in your code, this will have to be broken, but it's more a general idea than a code-law.)

In more technically, slightly more vague terms, if a function has any dependencies, they should be provided to a function rather than a function finding those dependencies itself. ("dependency injection" is typically what this is called.)

I would consider rewriting your code like:

function output(array $recordsA, array $recordsB, $authorized, $module)
{

    if($authorized)
    {
        include('/path/to/' . $module . '.php');
    }

    echo "<ul>"

    foreach ($recordsA as $record) {
        echo "<li>" . htmlspecialchars($row['description']) . "</li>";
    }

    echo "</ul><br><ul>";

    foreach ($recordsB as $record) {
        $status = ($authorized) ? htmlspecialchars($record['status']) : 'N/A';
        $description = htmlspecialchars($record['description']);
        echo "<li>{$description} ({$status})</li>";
    }

    echo "</ul>";

}

Then the usage of the function would be:

$recordsAQuery = mysql_query("...");
$recordsA = array();
while ($row = mysql_fetch_assoc($recordsAQuery)) {
    $recordsA[] = $row;
}

$recordsBQuery = mysql_query("...");
...

$module = ...;

output($recordsA, $recordsB, is_authorized(), $module);

I would also be tempted to abstract yet more. Specifically, the database stuff could be abstracted away a bit more depending on how carried away one was.

function queryAndFetchAll($query, $conn = null)
{
    $res = mysql_query($query);
    if (!$res) {
        throw new Exception("mysql_query failed.  Query: [{$query}]. Error: [" . mysql_error($conn) . "]");
    }
    $records = array();
    while ($row = mysql_fetch_assoc($res)) {
        $records[] = $row;
    }
    return $records;
}

$recordsA = queryAndFetchAll("SELECT ...");
$recordsB = queryAndFetchAll("SELECT ...");

Note that I would name recordsA and recordsB something more descriptive. (I also might consider breaking this function into two functions.)

And, depending on what exactly the module file is doing, I might try to abstract that out of the function.

---

On a side note, the PDOStatement is Iterable, so you can pass it and the function can use it silently as an array.

If you're not familiar with PDO this probably won't have much meaning, but hopefully the gist is there:

$recAStmt = $db->prepare("SELECT * FROM main WHERE id > ?");
$recAStmt->execute(array($id));
$recBstmt = ...;
output($recAStmt, $recBStmt, ...);

output

What is being output? I would consider naming this something like renderStatuses. (Though if you were to rename it render*, I would expect it to return content instead of echoing.)

echo vs return

That brings me to my next point :).

I avoid outputting to the client except for in my main view code. What if for some reason you want to store this code to a variable instead of echoing it immediately? If you want to echo immediately, it's easy enough to do echo f();, but storing and then echoing is quite a pain. (ob_start(); f(); $f = ob_get_flush();) This does tend to make functions take a bit longer to write, but it's typically worth it.

SELECT *

This one is being pretty picky, however, I favor explicitly listing the column name. There's a tiny performance benefit (even if selecting all columns), but more so than that, I would rather my queries break in the future than my PHP code silently mess up.

Consider if you decided to rename your status column. Your code would silently break. You'd start getting undefined index notices, and nothing would be output correctly.

However, if you had selected all of the columns individually, the query would fail, and you'd likely notice the error a lot quicker.

Answer 2

• magic strings,
• DB connection not closed,
• DB errors not handled,
• no prepared statements with safe query parameters,
• injectable request and query parameters,
• mix of UI and logic code,
• no HTML encoding,
• 2 of the 3 comments are very redundant with the line they comment.

Answer 3

REQUEST / GET Array

Would suggest not using the REQUEST array. Because this data is so easy to hijack it is a poor choice. Better to simply use the right array (GET, POST, COOKIE, SESSION) and if it could be one of a number of them, then set up statement(s) to return the correct one. Also, I would not immediately use any value from any user provided array directly in my program. It should be sanitized and validated first. Check out PHP's filter_input() function. Here's an example of it.

$id = filter_input( INPUT_GET, 'id', INPUT_SANITIZE_NUMBER_INT );
//Equivalent to sanitizing and validating $id = $_GET['id'];

Variables

Make sure your variables are available for each instance a function might be run! Right now $authorized is only ever defined if the contents of that first if statement are processed. Because of this, if is_authorized() fails then your second mysql while loop would produce errors because $authorized was never defined. Also, that variable need not be separate from that function. is_authorized() obviously returns a boolean, or something that can be used as a boolean, should be the former rather than the later. Since $authorized needs to always be set, and its value is dependent upon that function, why not just set it to the return value of that function?

$authorized = is_authorized();
if( $authorized ) { }

Ternary Operations

Ternary operations are good for simple things, such as setting a variable. What they are not good for is long statements, string concatenation, and other such more complicated tasks. They can do it, but they lead to illegible code and mistakes. I mainly point this out because your ternary operation is flawed. Currently it will not end the parenthesis or end the list item because that is considered part of the "else" statement in the ternary operation. A better way to perform your ternary operation would be to remove it from the string and declare it beforehand. This will prevent all such mishaps in the future and is easier to read.

$status = $authorized ? $row['status'] : 'N/A';

PHP Strings

Once this is done then you can insert it into the string. Let me also point out that there are two types of PHP strings. Strings that are processed for variables and entities and strings that aren't. The first uses double quotes ("") to enclose a string and add a little bit of overhead to PHP, unnoticeable really, so that it knows to search that string for items to escape. The later uses single quotes ('') and is only ever used for holding static strings. There is no overhead to these strings because PHP ignores them. That being said, you are telling PHP that all of your string have variables and entities in them to be escaped, so PHP has to process them. As I said, the overhead isn't really noticeable, but it is there and unnecessary. So I would write that echo statement you had your ternary operation in like so:

echo "<li>{$row['description']}($status)</li>";
//OR
echo '<li>' . $row['description'] . "($status)</li>";
//OR EVEN
echo '<li>' . $row['description'] . '(' . $status . ')</li>';

Out of all of those I prefer the first implementation, but it is a matter of preference. Honestly this bit on strings might be considered preference as well, but I believe this optimization is standard and that my examples are also easier to read, minus the third which is more or less the same.

Haylem and Corbin

I mostly agree with them, but I believe an explanation is in order instead of just all these terms you might not have heard of before, or don't understand.

"no prepared statements with safe query parameters" Not strictly necessary, but yes, PDO or MySQLi is much better than mysql_*.

From my understanding mysql is also getting deprecated. But maybe I've heard wrong. Don't do much with MySQL to know, so keep your eye out for this. It is good for learning the basics on though. Continue what you are doing, just be aware that there are better, and safer (security wise), solutions out there that you should eventually look into. And that eventually you may have to change any mysql code.

DB connection not closed" -- scripting languages are used for a reason

I still agree that it should be closed. This will prevent the connection from being used after you are done with it. Not sure how it is done, but I still think it is a valid concern. Sure PHP is very forgiving and should close it for you, but I believe writing the extra line won't hurt and can only help.

some random output method has no business making a DB connection in the first place

What they are talking about here is called Separation of Concerns. In my opinion its the first step to learning OOP. Don't be concerned with OOP quite yet, but it is a good idea to start learning this concept. Basically it states that functions should only be concerned with one thing. A well written function can be described using its name only and should not do anything its name does not imply. From your output function I would expect it to only "output" information, never to connect to a database and retrieve it. I would instead expect to see a connectToDB() function, or something similar. And maybe a fetch() function for those mysql queries. So your output function would more closely resemble this (all suggestions implemented here):

connectToDB();

$id = filter_input( INPUT_GET, 'id', INPUT_SANITIZE_NUMBER_INT );
$res1 = fetch(" SELECT * FROM main WHERE id > $id;");
$res2 = fetch(" SELECT * FROM main WHERE id < $id;");

function output( $res1 , $res2 )
{
    // Check authorization
    $authorized = is_authorized();
    if( $authorized )
    {
        include('/path/to/' . $_REQUEST['module'] . '.php');
    }
    echo "<ul>"

    while( $row = $res1 )
    {
        echo "<li>".$row['description']."</li>";
    }

    echo "</ul><br><ul>";

    while( $row = $res2 )
    {
        // Display the status if it is authorized, othewise display N/A
        $status = $authorized ? $row['status'] : "N/A";
        echo "<li>{$row['description']}($status)</li>";
    }

    echo "</ul>";
}

Even this is not fully optimized, but it will give you an idea and a place to start. Further optimization might see those while statements turned into a single function so that it could be reused. Hope this helps!