How important is it really to check every array index in PHP?

Question

I'm working on quite a large project in which there are a lot of places where code like the following exists:

function foo($a, $b, $c, $d, $e, $f) {
    $clean = array();
    $mysql = array();

    $clean['a'] = htmlentities($a);
    $clean['b'] = htmlentities($b);
    $clean['c'] = htmlentities($c);
    $clean['d'] = htmlentities($d);
    //...

    $mysql['a'] = mysql_real_escape_string($clean['a']);
    $mysql['b'] = mysql_real_escape_string($clean['b']);
    $mysql['c'] = mysql_real_escape_string($clean['c']);
    $mysql['d'] = mysql_real_escape_string($clean['d']);
    //...

    //construct and execute an SQL query using the data in $mysql
    $query = "INSERT INTO a_table
              SET a='{$mysql['a']}',
                  b='{$mysql['b']}',
                  c='{$mysql['c']}',
                  d='{$mysql['d']}'";
}

Obviously this throws a lot of warnings in PHP for undefined indexes.

Is it really necessary to rewrite the code as follows?

function foo($a, $b, $c, $d, $e, $f) {
    $clean = array();
    $mysql = array();

    $clean['a'] = htmlentities($a);
    $clean['b'] = htmlentities($b);
    $clean['c'] = htmlentities($c);
    $clean['d'] = htmlentities($d);
    //...

    $mysql['a'] = (isset($clean['a'])) ? mysql_real_escape_string($clean['a']) : mysql_real_escape_string($a);
    $mysql['b'] = (isset($clean['b'])) ? mysql_real_escape_string($clean['b']) : mysql_real_escape_string($b);
    $mysql['c'] = (isset($clean['c'])) ? mysql_real_escape_string($clean['c']) : mysql_real_escape_string($c);
    $mysql['d'] = (isset($clean['d'])) ? mysql_real_escape_string($clean['d']) : mysql_real_escape_string($d);
    //...

    //construct and execute an SQL query using the data in $mysql
    if (isset($mysql['a']) and isset($mysql['b']) and isset($mysql['c']) and isset($mysql['d'])) {
        $query = "INSERT INTO a_table
                  SET a='{$mysql['a']}',
                      b='{$mysql['b']}',
                      c='{$mysql['c']}',
                      d='{$mysql['d']}'";
    }

}

Answer 1

You can simplify your function a lot if you use:

function foo($a, $b, $c, $d, $e, $f) {

    $args = func_get_args();   // or build an array() manually

    $args = array_map("htmlentities", $args);
    $args = array_map("mysql_real_escape_string", $args);

    list($a, $b, $c, $d, $e, $f) = $args;

The isset() check at the showed position seems completely useless. The variables were already defined.

Answer 2

Is it necessary to have such a hard-coded function at all?

I use this:

function insert_array($table, $data) {  
    $cols = '(';
    $values = '(';
    foreach ($data as $key=>$value) { 
        $value = mysql_real_escape_string($value);
        $cols .= "$key,";  
        $values .= "'$value',";  
    }
    $cols = rtrim($cols, ',').')';
    $values = rtrim($values, ',').')';  
    $sql = "INSERT INTO $table $cols VALUES $values";
    mysql_query($sql) or die(mysql_error());
}

Then to insert data regardless of its name and columns use:

$data = array('id' => 1, 'name' => 'Bob', 'url' => 'foo.com');
insert_array('users', $data);

Answer 3

Yes, if the array index or the variable is not exists php give warning/notice.

The right way is to check every variable before using them with isset() function.

It is a good practice to check them before using.

Answer 4

You need to check indexes which might or might not exist. However, your code is quite confusing and your real code looks probably totally different. In this example code, the keys obviously exist, you just created them yourself.

1. In your example, you can move the mysql_real_escape_string part inside the if where you check the variables, then you already know that they exist.

2. There is no reason to use arrays here, you can store them in the same variable.

3. XSS-Protection (htmltentities(), note that that is not enough) should be done before dispalying data, not before storing as in your example. Just one reason is that you will end up with stuff that is encoded/escaped multiple times. Malicious HTML/JS can do no harm in your database.

Answer 5

If your project is very large, then undefined indexes can become a nightmare down the line, especially if they hold data generated by user input, and ESPECIALLY in the absence of good error reporting with stack tracing. This is because as the data is handed between requests, there is no guarantee that it was set at it's original point of entry, and therefore you end up doing a lot of redundant checks for null or empty values.

You may want to examine if what you are trying to accomplish here might not be better achieved by making this functionality into an object. The values you are representing with $a $b and $c could easily be made into object properties and one save() method could save the state down to database.

Other than that, you can perform your check much quicker and much more coherently by using a foreach loop. Just iterate over the data by its keys and perform the real escape and htmlentities within the loop body.

http://php.net/manual/en/control-structures.foreach.php

I also suggest HTMLPurifier for your XSS filtering, as very often htmlentites is insufficent, especially for public forms that accept user content to be placed within the web application.

http://htmlpurifier.org/