Generalized PHP function for editing data in a table

Question

I've got some legacy code which I need to maintain and its got this function which works perfectly fine, but I'm trying to understand if it is working using acceptable coding practices or not... I am trying to understand if it is safe and secure to operate the site with the function as-it-is.

The function would be called as:

$this->editData('users',"activation_key",$_GET['key']);

The actual function (unedited):

function editData($table_name,$fldname,$fldval,$other='',$lang_flag=0)
{

    $link = $this->my_connect();
    $this->my_select_db($this->DATABASE_NAME,$link);
$arr_types =  array("TR_", "TN_", "TREF_", "PHR_", "PHN_", "IR_", "IN_", "MR_", "MN_", "TNEF_", "TRC_", "TNC_", "TRFN_", "TNFN_","TNURL_","TRURL_");
    if (!empty($GLOBALS["HTTP_POST_VARS"])) {
        reset($GLOBALS["HTTP_POST_VARS"]);
        while (list($k,$v)=each($GLOBALS["HTTP_POST_VARS"])) 
        {

            for($p=0;$p<count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "<br> k =$k -- v = $v";
        }   
    }
    if (!empty($_POST)) {
        reset($_POST);
        while (list($k,$v)=each($_POST)) 
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "<br> k =$k -- v = $v";
        }   
    }
    if (!empty($GLOBALS["HTTP_GET_VARS"])) {
        reset($GLOBALS["HTTP_GET_VARS"]);
        while (list($k,$v)=each($GLOBALS["HTTP_GET_VARS"])) 
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "<br> k =$k -- v = $v";
        }   
    }
    if (!empty($_GET)) {
        reset($_GET);
        while (list($k,$v)=each($_GET)) 
        {

            for($p=0;$p < count($arr_types);$p++)
            {
                if(stristr($k,$arr_types[$p])!="")
                {
                    $k = str_replace($arr_types[$p],"",$k);
                }
            }
            ${strtolower($k)}=$v;
            //echo "<br> k =$k -- v = $v";
        }   
    }
     $result=$this->my_query("show fields from $table_name",$link);
    $query="update $table_name SET   ";
     while ($a_row = $this->my_fetch_array($result)) {
        $field="$a_row[Field]";


            if($field!=$fldname)
            {
                if(isset($$field))
                {
                       $query.="`".$field."`=";

                       $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
                }
                else
                {
                    if(isset($GLOBALS["$field"]))
                    {
                        //echo "<br> var ".$GLOBALS["$field"];
                        $query.="`".$field."`=";
                        $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";

                    }
                }
            }
     }
     $query = substr($query,0,-2);
    $query.=" where `$fldname`='$fldval' $other"; 
    $query  ;
    //echo $query;
     $result=$this->my_query($query,$link);
     $this->my_free_result($result);
     $this->my_close($link);
     return $result;
}

Answer 1

Simply put, this code fails a lot best practices also contains massive security issues. Suggestion to improve for best practices:

$GLOBALS["HTTP_POST_VARS"] is deprecated in favor of $_POST

So you looping over $GLOBALS["HTTP_POST_VARS"] and $_POST is redundant and bad mojo. Just looping over $_POST will accomplish the same thing. Same goes for $GLOBALS["HTTP_GET_VARS"] and $_GET.

You should be passing in the $link object as parameter.

Since there is no code intended to change the database connection conditionally (the connect parameters are always the same). This prevents your scripts creating multiple database connection resources and destroying them (more memory and time consuming). Also passing in the database resource as parameter to the function is a type of dependency injection which makes your code testable should you want to in the future. Since this also appears to be in a class, consider creating the database connection in the __construct and use that single instance.

Using sessions in favor of $GLOBALS

Pretty much any use of $GLOBALS is considered bad practice. I'm sure there are a few exceptions to the rule. But there you should definitely not be using is for storing data in a scopeless fashion. This is what sessions are for.

removeQuotes() is never called on $fldname or $fldval

As you can imagine this a serious security issue leaving your script open to SQL Injection attacks.

Use mysql_real_escape_string in favor of home made sanitising functions i.e. removeQuotes()

There is a lot more to safe escaping query parameters that remove slashes. And in my experience home made sanitising functions often quietly fail when you have mixed string and number parameters.

As a matter of readability and style I would make to following suggestions, I know these are subject matters but there are common ways of doing things.

Substitute the use of string concatenation

$query.="`".$field."`=";
$query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
... more concatenation ...
$query = substr($query,0,-2);

for array joining

$queryParams[] = "`".$field."`='".$this->removeQuotes($$field,$lang_flag)."'";
... more array filling ...
$query = join(',', $queryParams);

General WTFs

This is doing nothing at all $query ;

And code that does nothing is very bad mojo.

Call count() once.

This is not really an issue, doing something like this

for($p=0;$p

In actual fact calls count() on every iteration of the loop, even though the return value will never change. It causes the interpreter to do more work than necessary, although I suspect during compilation to byte code this would be optimised automatically. This could be substituted for:

// only call count() once
$count = count($arr_types);
for($p=0;$p<$count;$p++)

Don't nest unnecessarily.

Don't nest conditionals (if/else/switch) and loop(while/for) unnecessarily. It makes for messier code, code that's harder to read and follow mentally, and importantly difficult to test.

if(isset($$field))
{
     $query.="`".$field."`=";

     $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
 }
 else
 {
     if(isset($GLOBALS["$field"]))
     {
         $query.="`".$field."`=";
         $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";
     }
 }

The above code is exactly equal to the following, notice less nesting.

if(isset($$field))
{
     $query.="`".$field."`=";

     $query.="'".$this->removeQuotes($$field,$lang_flag)."' , ";
 }
 else if(isset($GLOBALS["$field"]))
 {
         $query.="`".$field."`=";
         $query.="'".$this->removeQuotes($GLOBALS["$field"],$lang_flag)."' , ";
 }

Personal preference

This is completely subjective, but something to think about.

if(stristr($k,$arr_types[$p])!="")

equal to

if(stristr($k,$arr_types[$p]) == true)

equal to

if(stristr($k,$arr_types[$p]))

Considering the above, this:

while (list($k,$v)=each($_GET)) 
{
    for($p=0;$p < count($arr_types);$p++)
    {
        if(stristr($k,$arr_types[$p])!="")
        {
            $k = str_replace($arr_types[$p],"",$k);
        }
    }
    ${strtolower($k)}=$v;
}

Is exactly the same as:

while (list($k,$v)=each($_GET)) 
{
    $count = count($arr_types);
    for($p=0; $p<$count; $p++);
    {
        $k = str_replace($arr_types[$p],"",$k);
    }
    ${strtolower($k)}=$v;
}  

Which would you rather test and/or work with?