Common Style Mistakes, Part 1

by John Coggeshall
05/29/2003

Introduction

With some PHP basics under our belt, it's time to introduce one of the most important things when writing web applications: security. I don't mean just protecting credit cards or other personal information. Rather, I am talking about writing secure, solid code from the very first line to the very last. With this topic, which I call "PHP Paranoia", I intend to teach you not only the function calls necessary to accomplish a task, but also the thought processes and practices to do so safely.

I recommend that you look at the columns in this series not as simple examples of using whatever functionality has been chosen but, rather, as general examples that can be applied to a number of different situations. I'll start my discussion by pointing out a few common stylistic mistakes made by PHP newcomers.

Errors in Style

Although they can't be considered fatal errors, bad stylistic habits can lead to a number of problems in a large-scale development project. Often these practices don't become apparent until you upgrade to a newer version of PHP. But they can also add hours to your development time.

To help you clearly see the differences between what is considered bad and good style let's take a look at some contrasting examples.

Tip 1: Avoid optional configuration directives

Consider the following code:

<?
    $var = 0;
    function myfunc($myvar) {
        $myvar = 1;
    }
    echo "The value of the variable is: $var<br>";
    myfunc(&$var);
    echo "The value of the variable is: $var<br>";
?>

Although this code will work, it relies on PHP configuration directives to function properly. For instance, what would happen if the PHP configuration directive short_tags were disabled? Since this code does not use <?php to begin a code block, the entire script would be output directly to the browser! Although not a serious error for this script, for a script containing more sensitive data it would be a real security risk.

There is also a problem here with the way our variable $var is being passed to the function. Although it's not a problem to pass a variable by reference, notice that the function declaration itself does not indicate that references are allowed. There is a configuration directive allow_call_time_pass_reference which will allow you to ignore this error. However, in future versions of PHP, the chances are good that your code will break. It is important to make sure that when passing variables by reference that you declare the function as accepting references:

function myfunc(&$myvar) {
    $myvar = 1;
}

Tip 2: Avoid using function return values directly

Another common stylistic error which occurs among both seasoned and new PHP developers alike is the tendency to use return values from functions directly as parameters for other functions. Consider the following code:

<?php
    $mystring    = "Test String";
    $my_variable = (strcmp(strtoupper(substr($mystring,
        (strlen($mystring) > 10) ? strlen($mystring) - 10 : 0)),
        strtoupper(substr($mystring,
            rand(0,strlen($mystring))))) == 0) ? true : false;
?>

Writing code like this works, but can you tell me what it does? Furthermore, would anyone looking at this code who didn't actually write it have any idea? These factors should both influence the way you write your scripts. Here's a much easier to read version of the above code:

<?php
    $str_len     = strlen($mystring);
    $start_pos   = ($str_len > 10) ? $str_len - 10 : 0;
    $start_rand  = rand(0, $str_len);
    $substr_rand = substr($mystring, $start_rand);
    $substr_base = substr($mystring, $start_pos);
    $substr_rand = strtoupper($substr_rand);
    $substr_base = strtoupper($substr_base);

    if(strcmp($substr_base, $substr_rand) == 0) {
        $my_variable = true;
    } else {
        $my_variable = false;
    }
?>

Now can you tell what this code does? The point is to not be afraid to write a few extra lines of code for the sake of readability. In the long run it will most definitely pay off. If you really feel the urge to use return values directly, as a general rule of thumb, if the combined number of parameters between all of the functions is three or less, it is acceptable in most circumstances:

<?php
    /* "acceptable", but not recommended */
    $mystr = substr(strtoupper($myvar), 5);
?>

In this case, we are calling the substr() function with only two parameters, one of which is the strtoupper() function which accepts one parameter. Since there are a total of three parameters in this statement, it's probably acceptable to combine them into a single line if so desired.

Although there are times when using a return value directly in another function may be acceptable, please note that one should never use a conditional assignment statement as function call parameter:

<?php
    /* Don't ever do this */
    $mystr = substr(strtoupper($myvar), (strlen($myvar) > 10) ?
                                         strlen($myvar) - 10 : 0);
?>

Tip 3: Avoid using improper array syntax

One common syntax mistake is found when working with string-indexed arrays. Due to the nature of PHP, a piece of code such as the following:

<?php
    $myarray = array('foo'=>'Your index was foo', 'bar'=>'Your index was bar');
    echo $myarray[bar];
?>

will, as you might expect, print Your index was bar to the browser. However, if the following nearly identical script is executed, things will break:

<?php
    define('bar', 'foo');
    $myarray = array('foo'=>'Your index was foo', 'bar'=>'Your index was bar');
    echo $myarray[bar];   
?>

Instead of the expected output, the script will now display Your index was foo to the browser. Since in this case I have defined a constant bar whose value is foo, PHP has replaced each instance of bar with the string foo in my script. Since my array index is not represented explicitly as a string (being encapsulated in single or double quotes), it is automatically replaced with the value foo. My echo statement actually displays the value stored in the array index defined by the constant (not the string) bar. In order to prevent this confusing mess, it is important that all non-integer array indexes be clearly defined as strings as shown:

echo $myarray['bar'];

Even if you have no constants defined in your scripts, there is no assurance that from version to version of PHP a new constant won't be introduced which will cause undesired behavior. Note that there is one exception to this rule, namely, when referencing an array index from within a string. For instance, you are already familiar with the following syntax:

echo "My Value {$myarray['bar']}";

where the value associated with the key bar in the array $myarray will be displayed. Since this array is in a string, this syntax can be simplified without consequence as shown:

echo "My Value $myarray[bar]";

For consistency's sake, I recommend the former of these two examples be used (when working with objects and their member variables only the former will work). If nothing else, you should be aware of the differences.

Coming soon

That concludes the first of many columns in my PHP Paranoia series. You are already well on your way to writing quality PHP code, but we're not finished yet. There are still many more common mistakes made by PHP developers to discuss, so check back again soon for my next article.

John Coggeshall is a a PHP consultant and author who started losing sleep over PHP around five years ago.

---

Read more PHP Foundations columns.

Return to the PHP DevCenter.