-6

I am updating a table in phpmyadmin like this. My question is basically: Why do I need a comma after '$username', i.e this $query.="username = '$username' , ";Not sure why this comma is necessary.

function UpdateTable()
{
    global $connection;
    $username = $_POST['username'];
    $password = $_POST['password'];
    $id = $_POST['id'];
    $query = "UPDATE users SET "; // username is the name of the column in the database, you need the space in  between
    $query.="username = '$username' , ";
    $query.= "password = '$password' ";
    $query.= "WHERE id = $id ";
    $result = mysqli_query($connection,$query);
    if(!$result)
    {
        die("Query failed". mysqli_error($connection));
    }
}
Jason Genova
  • 127
  • 7
  • 3
    PHPMyAdmin *is not* a database. It is a web interface for your MySQL database. – Jay Blanchard May 31 '16 at 18:28
  • 6
    The comma is necessary because that is proper SQL syntax for an `UPDATE` statement. – Jay Blanchard May 31 '16 at 18:28
  • 6
    **Never store plain text passwords!** Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). Make sure that you [don't escape passwords](http://stackoverflow.com/q/36628418/1011527) or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard May 31 '16 at 18:28
  • `echo $query;` before `$result = mysqli_query...` i think you will see – splash58 May 31 '16 at 18:28
  • 6
    [Little Bobby](http://bobby-tables.com/) says [your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php). Even [escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! – Jay Blanchard May 31 '16 at 18:29
  • Sorry.. I am new to PHP I just started the topic of mysql injection. Thank you – Jason Genova May 31 '16 at 18:31
  • 2
    I'm voting to close this question as off-topic because had the OP read the fine user's manual he would've seen the proper SQL syntax. – Jay Blanchard May 31 '16 at 18:31
  • @JayBlanchard [we do not have a RTFM close reason, and that is intentional](http://meta.stackoverflow.com/a/253484/781965). – Jeff Jun 01 '16 at 04:04

2 Answers2

1

Because that is MySQL syntax. Check MySQL doc: http://dev.mysql.com/doc/refman/5.7/en/update.html

UPDATE [LOW_PRIORITY] [IGNORE] table_reference
    SET col_name1={expr1|DEFAULT} [, col_name2={expr2|DEFAULT}] ...
    [WHERE where_condition]
    [ORDER BY ...]
    [LIMIT row_count]
Igor R
  • 418
  • 2
  • 7
  • thank you :), didnt know that I thought it had to do with some spacing issues which I previously messed up alot before ( like forgetting to put a space after SET) – Jason Genova May 31 '16 at 18:32
0

It's not necessary following '$user_name'.

It's necessary preceding the next assignment, on the following line.

Just like the space following '$password' isn't strictly necessary.

The space is necessary before the WHERE on the following line.

Think of the statement as being formatted like this:

 UPDATE users
    SET username = ?
      , password = ?
  WHERE id = ? 

The comma is required, and essentially reads as "and also SET"

A better pattern is to prepend the required space and/or commas on the line that requires it. Rather than remembering to do it on the line where it's not required. e.g.

$sql = "UPDATE users";
$sql.= " SET username = ?";
$sql.= ", password = ?";
$sql.= " WHERE id = ?"; 

Doesn't make much difference here. But when you actually have a need to dynamically generate SQL, this pattern is much easier.


Some additional thoughts.

Do NOT store plaintext passwords in the database. Store hash values instead.

Code appears to be vulnerable to SQL Injection.

Use prepared statements with bind placeholders... static SQL text, not dynamically generated SQL.

spencer7593
  • 106,611
  • 15
  • 112
  • 140