Update PHP function GetSQLValueString

Question

The old function was:

<?php
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "")
{
    $theValue = (!get_magic_quotes_gpc()) ? addslashes($theValue) : $theValue;

    switch ($theType) {
        case "text":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "long":
        case "int":
          $theValue = ($theValue != "") ? intval($theValue) : "NULL";
          break;
        case "double":
          $theValue = ($theValue != "") ? "'" . doubleval($theValue) . "'" : "NULL";
          break;
        case "date":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "defined":
          $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
          break;
    }
    return $theValue;
}
?>

I have changed in this way:

<?php
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "")
{
    $conn = new mysqli('localhost', 'username', 'password', 'dbname');

    $theValue = (!get_magic_quotes_gpc()) ? addslashes($theValue) : $theValue;

    $theValue = $conn->real_escape_string($theValue); 
    switch ($theType) {
        case "text":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "long":
        case "int":
          $theValue = ($theValue != "") ? intval($theValue) : "NULL";
          break;
        case "double":
          $theValue = ($theValue != "") ? "'" . doubleval($theValue) . "'" : "NULL";
          break;
        case "date":
          $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
          break;
        case "defined":
          $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
          break;
    }
    return $theValue;
}
?>

Is what I've made correct?

Can I remove get_magic_quites_gpc() if I use PHP version 5.3.2?

Answer 1

Your motivation is justified, but the revised code isn't quite right either.

Magic quotes was an ill-conceived idea that in fact has been deprecated in PHP 5.3 and will be removed altogether in 5.4. The only good reason for calling get_magic_quotes_gpc() would be to detect whether you are running on a server that has this unfortunate feature enabled, and undo its ill effects with stripslashes() if it is. (Include the stripslashes() routine early in your script so that your entire program benefits.)

Revising the function to use $conn->real_escape_string() protects you from SQL injection problems. Get rid of the conditional call to addslashes(), because it redundantly tries to do the same thing as real_escape_string(), but inaccurately.

You still have three problems, though:

• Performance: new mysqli() opens a new connection to the database. That's a very heavy price to pay just to escape a string.
• Connection leak: You open the connection without closing it when you're done. If you call this function dozens of times, you would have dozens of connections that remain open until your script terminates.
• Locale: The reason real_escape_string() requires a connection is that its behaviour depends on the character set of the connection. To be strictly correct, you should be using whatever existing connection you may have so that you apply the correct escaping for the relevant character set.

To solve all three issues, this function should use an existing mysqli connection object.

It's good that your application is using some kind of database abstraction layer, but I get the feeling that you are reinventing the wheel, poorly. If at all possible, I suggest that you consider replacing it all with a standard solution such as PDO.

Answer 2

PHP is not my area of expretise, so just some generic notes:

1. The function create a new MySQL connection on every call which could be a bottleneck in your application. You could use persistent connections with a p: prefix but others usually suggest PDO as a better alternative of database handling.

2. The $the prefix on most of the variables seem redundant (it doesn't add too much to them), I'd avoid it.

3. If you intended to return the original value when its not text, long, int etc. you should make it explicit with a default case inside the switch statement. If not, throw an exception in the default case and sign the error. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

4. case "text" and case "date" contains the same logic, you could use fall through there too.