mysqli Help

[ironmangary]

I have a game engine that desperately needs upgraded from PHP 5 to PHP 7. I’m trying to tackle database functions for starters, but running into a lot of issues, and Google hasn’t been helpful, either.

The original code I need to convert is:

	if (!isset($settings[$settingname]) && $value){
			$sql = "INSERT INTO " . db_prefix("settings") . " (setting,value) VALUES (\"".addslashes($settingname)."\",\"".addslashes($value)."\")";
	}else if (isset($settings[$settingname])) {
			$sql = "UPDATE " . db_prefix("settings") . " SET value=\"".addslashes($value)."\" WHERE setting=\"".addslashes($settingname)."\"";
	} else {
		return false;
	}
	db_query($sql);

Here’s what I currently have:

$conn = mysqli_connect('localhost', 'username', 'password');
	if (!isset($settings[$settingname]) && $value){
			$sql = "INSERT INTO " . db_prefix("settings") . " (setting,value) VALUES (\"".addslashes($settingname)."\",\"".addslashes($value)."\")";
	}else if (isset($settings[$settingname])) {
			$sql = "UPDATE " . db_prefix("settings") . " SET value=\"".addslashes($value)."\" WHERE setting=\"".addslashes($settingname)."\"";
	} else {
		return false;
	}
	mysqli_query($conn, $sql);

The resulting error is:

Warning: mysqli_query() expects parameter 1 to be mysqli, string given 

So, I tried switching the values around, like this:

	mysqli_query($conn, $sql);

And I get this error:

Warning: mysqli_query() expects parameter 1 to be mysqli, string given

So I obviously have no idea what the mysqli_query() function wants. I appreciate any advice.

[phdr]

Updating old code requires more than just getting it to work with a new database extension. You must make sure that the code is also secure.

The existing use of addslashes() in the code was never secure and even the use of the various _escape_string() functions is not secure if the character set that php is using is not the same as your database table(s).

The foolproof way of making queries secure for all data types is to use a prepared query. This also simplifies the code/query, provided you use the much simpler PDO database extension, since you can remove a lot of existing things rather than to convert them.

Your use of a return statement in the code implies that this code is inside of a function definition. You should NOT make a new connection inside of each function that needs it. A database connection is one of the slowest things you can do. Making a new connection inside of each function will also consume multiple connections for every instance of your script. To get the database connection into the function you SHOULD supply it as a call-time parameter. However, if you have a lot of code (100’s of function calls and nested function calls), as a work-around, you can use the global keyword to bring the connection into the function.

You also need to eliminate all the escaped double-quotes. Each of these could have been just a single-quote. Keep It Simple (KISS.)

If you switch to use the PDO extension and use prepared queries, the posted code would look like -

if (!isset($settings[$settingname]) && $value){
	// note: this has swapped the two columns to match the same parameter order in the update query
	$sql = "INSERT INTO " . db_prefix("settings") . " (value, setting) VALUE (?,?)";
}else if (isset($settings[$settingname])) {
	$sql = "UPDATE " . db_prefix("settings") . " SET value=? WHERE setting=?";
} else {
	return false;
}
// note: $pdo contains an instance of a PDO connection
$stmt = $pdo->prepare($sql);
$stmt->execute([$value,$settingname]);

Note: an INSERT … ON DUPLICATE KEY UPDATE … query, with an appropriate index set, can probably replace all of that logic and two queries with just a one query.

[Ojoshiro]

Hello.
If you look into $conn, what do you see?
Right now you try to make a database handle and regardless of the outcome fire off a query. It’d be good practise to check whether you succeeded in making the handle first. (The $conn variable should hold false if it failed).

By the way, that is the boolean false. Not a string. So I’m really curious what’s actually in $conn.

[Ojoshiro]

Rather beside the point, don’t you think? Starting about all kinds of security and good practise when the query doesn’t even fire yet.