Updating data in an SQL table

Question

I have created a webpage that gets the user to login, if they havent got an account then they can register and is creates a new user for them.

This is the table users in the database (user_registration)

    user_id    username   password    email                 wage
    1          johnsmith  jsmith99    jsmith@gmail.com      0
    2          davidscott dscott95    davidscott@gmail.com  0

When a new user registered, the default value for the wage is 0. I want them to be able to edit this through the use of a form - this is the HTML code for the form:-

<form method="post" action="<?php $_PHP_SELF ?>">
  <input name="new-wage" type="text" id="new-wage" class="textbox" placeholder="New Wage">
  <input name="update" type="submit" id="update" value="Update" class="btn">
</form>

PHP Code: (note- the php and the html form are all in the same file(index.php) )

 <?php
 if(isset($_POST['update']))
 {
$dbhost = 'localhost';
$dbuser = 'root';
$dbpass = '';
$conn = mysql_connect($dbhost, $dbuser, $dbpass);
if(! $conn )
{
   die('Could not connect: ' . mysql_error());
}

$user_id = $_SESSION['MM_Username'];;
$wage = $_POST['new-wage'];

$query = "UPDATE users ".
       "SET wage = '$wage' ".
       "WHERE user_id = '$user_id'" ;

mysql_select_db('user_registration');
$retval = mysql_query( $query, $conn );
if(! $retval )
{
die('Could not update data: ' . mysql_error());
}
echo "Updated data successfully\n";
mysql_close($conn);
}
else
{
?>

When i fill in this form and hit the update button, it reloads the webpage and displays Updated data successfully which is exactly what should happen. BUT - when I refresh the table in PHP My Admin, it keeps the wage as 0, and not what i entered in the form.

Has anyone got any ideas what might be wrong with my code?

Thanks in advance for any answers.

PS.- I know that I have used the functions mysql_* and not mysqli_, simply because i dont know how to convert it, can you also help me with this??

the session has already been started in another part of the file(for the login) — Sam, Apr 08 '14 at 18:36
Ah you should mention all that .. also I don't see any `include()` or `require()` — Shankar Narayana Damodaran, Apr 08 '14 at 18:37
its likely that you are setting $user_id = $_SESSION['MM_Username']; and not the users ID. So it is updating your table where the userid = john, which is never true, so it doesnt update anything. — David Stetler, Apr 08 '14 at 18:43
sorry i am relatively new php coding, so sorry if i come across stupid but how do i implement include() or require()? — Sam, Apr 08 '14 at 18:44
$SESSION['MM_USERNAME'] holds the username - i know this as it is displayed on the webpage — Sam, Apr 08 '14 at 18:46
Ahar that was the problem, the $user_id was equal to the username which means that the table was updating for userid = johnsmith which isnt true it should be userid = 1 — Sam, Apr 08 '14 at 18:52
possible duplicate of [How to update data in an SQL database](http://stackoverflow.com/questions/22940989/how-to-update-data-in-an-sql-database) — The Blue Dog, Apr 08 '14 at 18:58

score 2 · Answer 1 · answered Apr 08 '14 at 18:34

2

You are vulnerable to sql injection attacks.

Plus, you don't seem to call session_start() so $_SESSION is probably empty, leading to $user_id being empty, and your query not matching anything. A simple var_dump($query) will show you what's actually going on there.

You need to check mysql_affected_rows() as well, to see if anything actually got updated.

answered Apr 08 '14 at 18:34

Marc B

356,200
43
426
500

+1 for mentioning SQL injection attacks, and for a sound answer. OP, you should consider using PDO for mysql, as it has prepared statements and is object-oriented, allowing more flexibility. – Apr 08 '14 at 18:36
the session has been started at the start of the file for the login to work - how would i convert to PDO? would that prevent sql injection attacks? @Ken – Sam Apr 08 '14 at 18:40
1

@SAMTHEMAN999 let's not convert until your problem is solved. – Apr 08 '14 at 18:42
2

just switching to pdo doesn't help prevent injection attacks. you have to fundamentally alter how you write the queries and execute them. – Marc B Apr 08 '14 at 18:54

score 1 · Accepted Answer · edited Apr 10 '14 at 16:48

Based on what you said in the comments:

This:

$user_id = $_SESSION['MM_Username'];
$wage = $_POST['new-wage'];

$query = "UPDATE users ".
       "SET wage = '$wage' ".
       "WHERE user_id = '$user_id'" ;

Should be this:

$user = $_SESSION['MM_Username'];
$wage = $_POST['new-wage'];

$query = "UPDATE users ".
       "SET wage = '$wage' ".
       "WHERE username = '$user'";

As you wanted to update the records based on the user id but you passed in the username as the parameter!

However, I'm assuming you want to update by the user_id, as that is unique for each user, and thus a more robust design.

To do that you'd have to add another $_SESSION variable that stores the user id when they log in. Call it $_SESSION['MM_Id'] or whatever. The name is arbitrary.

You also have an unclosed else statement at the end of your code:

else
{
?>

Either close it or remove it.

As Mark said, your code is susceptible to SQL injection attacks, a commonly abused mistake. I'd recommend looking at PDO (my personal favorite). If you're new to object-oriented programming, it may be difficult, but it offers more power, flexibility, and security. Plus, object-oriented programming is an important concept anyway.

To protect your current code from injection, use mysql_real_escape_string()...

$user = mysql_real_escape_string($_SESSION['MM_Username']);
$wage = mysql_real_escape_string($_POST['new-wage']);

$query = "UPDATE users ".
       "SET wage = '$wage' ".
       "WHERE username = '$user'";

This prevents people form putting in special characters (such as quotes) to attack your queries. I gotta run, but if you read up on SQL injection, you'll understand!

yes, at the end of the html code there is an else statement - would you be able to convert the code yourself, or if not, can you guide me into the right direction to convert AND make it safe from SQL injection attacks? thanks for your help. — Sam, Apr 08 '14 at 18:56
@SAMTHEMAN999 No problem! If I were you, I'd just remove the `else` statement. To protect your code, check my updated answer. — , Apr 08 '14 at 18:58
would you be able to drop me an email to help me with converting my code into PDO? - just I have been trying to do it my self but without success so far, and you seem like the person to help, any help or guidance to accomplish this would be very much appreciated. @KEN — Sam, Apr 09 '14 at 11:59

Updating data in an SQL table

2 Answers2