0

I'm trying to make a database for users to publish scripts for a game. Every single time I've had an issue using query and got it working one way or another. This time, I decided to make it a little easier to format the string for the query. Its not working. Heres my code:

function getSQLOperation($Data){
    $returning = "INSERT INTO `ScriptDatabase`";
    $keys = "(";
    $values = ") VALUES (";
    foreach ($Data as $Key => $Value){
        $keys = $keys."`".$Key."`, ";
        $values = $values."'".$Value."',";
    }
    return $returning.$keys.$values.")";
}
$values = array();
$values['Visibility'] = "Public";
$values['Name'] = "NameOfScript";
$values['Publisher'] = "UserID";
$values['Genres'] = "";
$values['PublishDate'] = Date('m-d-Y');
$values['Updated'] = $values['PublishDate'];
$values['Version'] = "1.0";
$values['Description'] = "None for now";
$values['Likes'] = "0";
$values['Dislikes'] = "0";
$values['Downloads'] = "0";
$operation = getSQLOperation($values);

mySQL table structure: mySQL table structure

Anything I'm doing wrong here?

Pneuma Official
  • 51
  • 6
  • 1
    **Warning:** You are wide open to [SQL Injections](https://stackoverflow.com/a/60496/1839439) and should use parameterized **prepared statements** instead of manually building your queries. They are provided by [PDO](https://php.net/manual/pdo.prepared-statements.php) or by [MySQLi](https://php.net/manual/mysqli.quickstart.prepared-statements.php). Never trust any kind of input! Even when your queries are executed only by trusted users, [you are still in risk of corrupting your data](http://bobby-tables.com/). [Escaping is not enough!](https://stackoverflow.com/q/5741187) – Dharman Apr 24 '20 at 21:12
  • 1
    Why would you ever need to create such a function? It is not only unsafe, but also unreadable. – Dharman Apr 24 '20 at 21:13

1 Answers1

0

MySQL doesn't allow trailing commas in queries, but your query is generated as:

INSERT INTO `foo`(`bar`, `baz`,) VALUES ('x','y',)

Of note, there are much better ways to work with MySQL. Check out PDO, especially prepared statements.

I mean it. Your query is vulnerable to SQL Injection attack and that's serious.

Robo Robok
  • 21,132
  • 17
  • 68
  • 126
  • given the trailing comma issue (outside of injection issues) try the implode function to solve that part. Will reduce a lot of the code you've written.https://www.php.net/manual/en/function.implode.php – nickL Apr 24 '20 at 21:17
  • so you could use implode(',', $keys) and manipulate it with implode(',' , $values) – nickL Apr 24 '20 at 21:19
  • @nickL I wouldn't encourage *fixing* OP's code, as it's fundamentally wrong approach to the problem. I answered what was wrong for educational purpose. – Robo Robok Apr 24 '20 at 21:19
  • Thanks for now just downvoting the question lmao. I'm not concerned about injection atm, just getting this set up. I have another POST handling that. This actually was the answer :) – Pneuma Official Apr 24 '20 at 21:21
  • I upvoted you to make it `0`, I sometimes don't understand voting policy on Stack Overflow lol. – Robo Robok Apr 24 '20 at 21:23
  • Prepared statemens are useless for the keys – Your Common Sense Apr 25 '20 at 04:51
  • OP's code uses column names and values. Your point? He should do what he does, because there are fields? – Robo Robok Apr 25 '20 at 08:13