DB insert function

[Derokorian]

Hey guys,
I made this insert function to inserting new data to the DB. I thought this was better than creating a function for each table and each amount of data to insert (IE sometimes I just use a lot of default values for the initial insert, sometimes I need to specify all fields). Anyway I was just wondering what you thought of it, and how you might improve it! $db is a mysqli connection object.

function dbInsert($table,$info) {
	GLOBAL $db;
	$query = "INSERT INTO `$table` SET ";
	foreach( $info as $k => $v) {
		if( is_numeric($v) ) {
			$query .= sprintf("`%s`=%f,",$k,$db->real_escape_string($v));
		} elseif ( $k == 'date_added' ) {
			$query .= sprintf("`%s`=%s,",$k,$db->real_escape_string($v));
		} else {
			$query .= sprintf("`%s`='%s',",$k,$db->real_escape_string($v));
		}
	}
	$query = substr($query,0,-1);
	if( !$db->query($query) ) return FALSE;
	return TRUE;
}

Thanks In Advance!

[bradgrafelman]

1. Seeing the use of [man]global[/man] sends up big huge red flags. Don't use global variables; pass in whatever parameters your function needs.

   Or, even better, switch to OOP and create your own DBAL class.

2. Floating point numbers are not exact (and thus are a different animal than decimal or integer numbers), so I would question why you're forcing all data that passes is_numeric() to be represented as a float.

3. Speaking of numeric data, you shouldn't be using real_escape_string() (and if you're wondering why, ask yourself what the last word of that method name is 😉).

4. Rather than let PHP decide what type of data it's inserting into the query based on what the data it has looks like, I would much rather be able to manually/explicitly specify the data type. Just because the user-supplied data you have looks like a string doesn't mean that column allows string data or that I ever wanted it to be represented as a string.

   In other words, you might as well just use prepared statements.

5. You could probably simplify this:

       if( !$db->query($query) ) return FALSE; 
       return TRUE; 

   down to this:

   return $db->query($query);

[Derokorian]

The reason I used float was because %d and %u were giving me problems with phone numbers =\

Also I type cast and verify correct data in the processing script before calling the function. (IE bool fields are 1 or 0, strings don't exceed the length, etc). And changed the $db to be passed as a parameter. However this function is for a site that's already live, and completely procedural, instead of trying to rewrite the whole site I'm just trying to clean up code (there were 27 different insert functions!) New function:

function dbInsert($db,$table,$info) {
    $query = "INSERT INTO `$table` SET ";
    foreach( $info as $k => $v) {
        if( is_numeric($v) ) {
            $query .= sprintf("`%s`=%f,",$k,$v);
        } elseif ( $k == 'date_added' ) {
            $query .= sprintf("`%s`=%s,",$k,$db->real_escape_string($v));
        } else {
            $query .= sprintf("`%s`='%s',",$k,$db->real_escape_string($v));
        }
    }
    $query = substr($query,0,-1);
    return $db->query($query);
} 

How would you handle the number thing? If they are too large they get changed. I know this is because of the way integers and unsigned integers are represented in memory (2147483647 and 4294967294 as maximums)

[bradgrafelman]

Derokorian wrote:

How would you handle the number thing?

Simple: don't treat phone numbers as numbers.

They really aren't "numeric" after all... do you ever add two phone numbers together? Multiply them? Find the square root of one? Increment/decrement them? Phone numbers really don't share any properties with other numeric data other than the fact that they are composed of digits. As such, I'd be storing them in a VARCHAR field and treating them as strings.

EDIT: Also, you say this:

Derokorian wrote:

And changed the $db to be passed as a parameter.

..and yet the code above doesn't reflect that change? Copy/paste error, perhaps?

[Derokorian]

..and yet the code above doesn't reflect that change? Copy/paste error, perhaps?

yeah lol copied it before changing all of it apparently, only selected XD I had fixed my post pretty quick tho!

[Derokorian]

function dbInsert($db,$table,$info) {
    $query = "INSERT INTO `$table` SET ";
    foreach( $info as $k => $v) {
        if( $k == 'phone' ) {
            $query .= sprintf("`%s`='%s',",$k,$v);
        } elseif ( is_numeric($v) ) {
            $query .= sprintf("`%s`=%d,",$k,$v);
        } elseif ( $k == 'date_added' ) {
            $query .= sprintf("`%s`=%s,",$k,$db->real_escape_string($v));
        } else {
            $query .= sprintf("`%s`='%s',",$k,$db->real_escape_string($v));
        }
    }
    $query = substr($query,0,-1);
    return $db->query($query);
} 

Hmm question, how do I make a mysql date column default to the current date? If I could do that I think I could get rid of the elseif ( $k == 'date_added' ) part all together. That's only there so mysql parses the NOW() instead of seeing a string and inserting 0000-00-00

[Derokorian]

Ok so it turns out you can't set a default to current date on a DATE column. So i tried to create a trigger, but apparently I can't do that with godaddy (the reasons to leave keep piling on). So I guess it will have to stay the way it is =\

[johanafm]

Derokorian;10985963 wrote:

date_added (...)

That's only there so mysql parses the NOW() instead of seeing a string and inserting 0000-00-00

To me, it sounds as if date_added is supposed to never be set to anything other than NOW() at the time of the insert. As such, I'd change that part to

elseif ( $k == 'date_added' ) {
            $query .= 'date_added=NOW(),'
        }