UPDATE

[jaykappy]

Trying to update a record...I dont get any errors but no update happening...Can anyone see anything standing out?  I can post more code if need be.

 

echo 'successful update';
echo $user_email;
echo $password;

$email = mysql_real_escape_string($user_email);
$password = mysql_real_escape_string($password);
mysql_query("UPDATE users SET email='$email' password='.md5($password)' WHERE `user_id`=".$_SESSION['user_id']." ");

$_SESSION['user']['email'] = $_POST['email']; 
echo $_SESSION['user']['email'];

 

[AbraCadaver]

$result = mysql_query("UPDATE .....");

if(!$result) { echo mysql_error(); }

 

Edited July 29, 2013 by AbraCadaver

[Psycho]

Always create your queries as a variable first so you can echo them to the page for debugging purposes. In this case I think one of two things is happening. Either the query is failing because it has an error - and you are not checking for errors. In fact, why are you stating "successful update" before you even make the update?! Or, there is no record that matches the value of $_SESSION['user_id'] - it may not even be set. Also, do NOT use mysql_real_escape_string() on a value that is going to be hashed! The hashing will prevent any sql injection and it will lead to problems later.

 

Looking closer at your code it is obvious it is failing. You are trying to include the hashed password in the query, but the md5()  is not being run because it is inside a string. Plus, you need a comma between each field.

 

Try this

<?php
$email = mysql_real_escape_string($user_email);
$password = md5($password);  ## Note: md5() is not suitable for password hashing

$query = "UPDATE users
          SET email = '{$email}', password = '{$password}'
          WHERE `user_id` = {$_SESSION['user_id']}";
$result = mysql_query($query);

if(!$result)
{
    //Query failed
    echo "Query failed!<br>Query: {$query}<br>Error: " . mysql_error();
}
elseif(!mysql_affected_rows())
{
    //No record was updated
    echo "No records updated!<br>Query: {$query}";
}
else
{
    //Record was updated
    echo 'successful update';
    echo $user_email;
    echo $password;
    $_SESSION['user']['email'] = $_POST['email'];
    echo $_SESSION['user']['email'];
}
?>

Edit: Note that the two error conditions above are only for debugging purposes. You should use generic messages for users in real use and not provide any actual errors from the system.

Edited July 29, 2013 by Psycho

[jaykappy]

Thanks everyone...I am trying this and seem to be hitting snags...syntax I assume...

Doing this so I can test if the password has been modified?  If not then dotn include in Query string

 

 

$query = " 

UPDATE users 

SET 

email = '{$email}' 

"; 

 

// If the user is changing their password, then we extend the SQL query to include the password and salt columns and parameter tokens too. 

if($password !== null) 

{ 

$query .= " 

, password = '{$password}'

"; 

} 

 

// Finally we finish the update query by specifying that we only wish to update the one record with for the current user. 

$query .= " 

WHERE 

user_id = {$_SESSION['user_id']} 

"; 

[Psycho]

That is nothing like what you originally posted. Are you hashing the password before you do this?

 

if($password !== null)

 

If so, that will always return true since the hash of an empty string is NOT an empty string. But, I already provided updated code that will check for errors. Are you using that? Are you getting errors? We can't tell you what is wrong if you don't provide the information needed.

[jaykappy]

This is all my code I am using...

 

1. For some reason I cannot get the Session User email with this?  $_SESSION['user']['email'];

  I had to run a query to grab the user info with this function...

 

I start with getting the SESSION user id, name etc.

Then use the code below to do a few validations etc... then while building the Query I test if the Password is blank, if so dont include that in the query as the user did not specify they wanted it changed...

I think things are working....although a bit chaotic ...

 

Anyone see anything that Could be doing better or security issues?  besides the md5...will get to that later

Function getuser($getuser){
$getuser = mysql_real_escape_string($getuser);

$user = array(); 
$user_query = mysql_query("SELECT `user_id`, `name`, `email` FROM `users` WHERE `user_id`=".$_SESSION['user_id'].""); 

While ($user_row = mysql_fetch_assoc($user_query)){
$user[] = array(
'userid' => $user_row['user_id'],
'user_name' => $user_row['name'],
'user_email' => $user_row['email']
);
}
return $user;
}

 

	$user = getuser($getuser);

	//// loop through array and return values
	foreach ($user as $individual){
	}	

	$email = $individual['user_email'];
	$user_id = $individual['userid'];

    if(!empty($_POST)) 
    { 	         
		$user_email = $_POST['email'];
		$errors = array();
		// VALIDATE EMAIL FORMAT
		if (filter_var($user_email, FILTER_VALIDATE_EMAIL) === false){
			$errors[] = 'Email not valid';
		}
		// MAKE SURE VALUE EXISTS
		if (empty($user_email)){
			$errors[] = 'you need an email address';
		}
		if($_POST['email'] != $email) {
			if (user_exists($user_email) === true){
				$errors[] = 'user already exists';
			}		
		}

		//// SET PASSWORD VARIABLE IF PASSWORD CHANGING
		if(!empty($_POST['password'])){ 
			$password = $_POST['password'];
        } 
        else { 
            // If the user did not enter a new password we will not update their old one. 
            $password = null; 
        } 
		
		// display errors if exist, else modify the user
		if (!empty($errors)){
			foreach ($errors as $error){
				echo $error, '<br />';
			}
		}else{
		
			$email = mysql_real_escape_string($user_email);
			$password = md5($password);
					
			$query = " 
				UPDATE users 
				SET 
					email = '{$email}' 
			"; 
			 
			//// If the user is changing their password, then we extend the SQL query to include the password. 
			if(empty($_POST['password'])){
				echo 'EMPTY PASSWORD';
			} else {
				$query .= " 
					, password = '{$password}'
				"; 				
			}
			
			// Finally we finish the update query by specifying that we only wish to update the one record with for the current user. 
			$query .= " 
				WHERE 
					`user_id` = {$_SESSION['user_id']}
			"; 

			$result = mysql_query($query);

				if(!$result){
					//Query failed
					echo "Query failed!<br>Query: {$query}<br>Error: " . mysql_error();
				}
				elseif(!mysql_affected_rows()){
					//No record was updated
					echo "No records updated!<br>Query: {$query}";
				}else{
					//Record was updated
					echo 'successful update';
					echo $user_email;
					echo $password;
					$_SESSION['user']['email'] = $_POST['email'];
					echo $_SESSION['user']['email'];
				}
		}			  
	}	

Edited July 29, 2013 by jaykappy

[Psycho]

I already gave you some tips and you didn't want to implement those (e.g. creating your query as a string variable so you can echo to the page for debugging). But, regardless, here are some other tips.

 

1. Give your variables meaningful names. Your function getuser has the variable $getUser. That doesn't give you any indication what the value really is. It should be $userID (or something like that).

 

2. Don't assume your queries will always work. Add error handling as shown previously. You will save yourself hours of hair pulling when you do hit a snag.

 

3. In your function getuser() you have a while() loop to process the DB result. First, you don't verify if any records were even returned. Plus, do you expect to have more than 1 record returned? No need for a while loop if there will only be one record. This is a waste of code:

 

While ($user_row = mysql_fetch_assoc($user_query)){
$user[] = array(
'userid' => $user_row['user_id'],
'user_name' => $user_row['name'],
'user_email' => $user_row['email']
);
}
return $user;

 

Just do this (after implementing error handling for failed query and no records returned):

 

$user = mysql_fetch_assoc($user_query);
return $user;

 

4. This seems to have no purpose since use should only ever be one person. Doesn't do anything anyway.

 

    //// loop through array and return values
    foreach ($user as $individual){
    }

 

5. OK, now I see why you have the above

 

    $email = $individual['user_email'];
    $user_id = $individual['userid'];

 . . . and what is the problem with just referencing $user['email'] and $user['user_id']? All you are doing is creating a lot of duplicative data in different variables.

 

6. This is not a good validation

 

if(!empty($_POST['password'])){

A value of a space will

 

OK, I'm not going to go through any more as it will be too time consuming. So, I will leave you with this. Take your time when determining the process flow in your logic. Get out a piece of paper and determine what data you need and when. Figure out any branching logic (e.g. if/else) and where it makes sense to do it - and ensuring you don't have any dead ends. Then, write your code. In your validation you do some checking of the password from the POST data and put a value into a variable. But then later you do another check on the POST value - you should instead check the value you set earlier.

[jaykappy]

First off Thank you very much for your help and patience...it is greatly appreciated...green here and it shows but catching on...

 

As for creating the Query as a string...I thought I was still doing that BUT in three parts. ( using the $query .= " I am able to bring all three parts together )..in the second I am testing to see if there was a password modifications (ie something in the textbox) if so include it in the Query string...if not then don't reference it.  I dont know of any other way to do this....BUT in the end I concatenate all 3 pieces and create one String Query.

 

Not using the While loop with the Array makes perfect sense seeing that I am only looking for one record and not even testing for more than one anyways...pointless...

Removing the While loop and For Loop makes perfect sense and referencing the return values with $user['user_id'] works great...

 

I thank you...was able to clean my code up quite a bit....really starting to see the bigger picture here...slow moving as I dont get a ton of time to devote to this....just minutes here and there....

 

Last question....if the below is NOT GOOD practice....then what should I test for....assuming that my textbox on the form is empty to start with...I need to test to see if they placed a value in there or not...Maybe test for "Length > x", maybe adding htmlentities as well

 

if(!empty($_POST['password'])){

 

Thank you very much for your help and guidance...very appreciated.

[jaykappy]

I tried to edit my post above but could not....I switched to this for testing the password

$posted_password = $_POST['password'];
if (isset($posted_password) AND $posted_password != ''){

instead of 

if(!empty($posted_password)){
 

Thoughts?  Thanks

Edited July 30, 2013 by jaykappy

[Psycho]

As for creating the Query as a string...I thought I was still doing that BUT in three parts. ( using the $query .= " I am able to bring all three parts together )..in the second I am testing to see if there was a password modifications (ie something in the textbox) if so include it in the Query string...if not then don't reference it.

 

I was referring to this in the last code you submitted:

 

$user_query = mysql_query("SELECT `user_id`, `name`, `email` FROM `users` WHERE `user_id`=".$_SESSION['user_id'].""); 

The query is written directly in the mysql_query() call, so if there were errors you can't echo the query to verify it. There isn't even any error handling on the query execution anyway. And, even though session values should be fairly safe to use w/o validation, you should always validate/escape values that could cause errors. In this case, you could be using prepared statements (MySQLi or PDO) or you can format the value to be in integer before running the query. You shoudl definitely look at using MySQLi or PDO for database handling, but there will be a learning curve.

 

 

Last question....if the below is NOT GOOD practice....then what should I test for....assuming that my textbox on the form is empty to start with...I need to test to see if they placed a value in there or not...Maybe test for "Length > x", maybe adding htmlentities as well

 

 

if(!empty($_POST['password'])){

 

I missed removing that comment in my response. I really isn't good practice, but passwords are unique. What I was getting at is you shouldn't typically do a empty() check on POST values directly to verify if the user made an entry into a field. The reason is because spaces are not considered "empty". So, if you have a name field that is required, you would not want the user to use spaces for a name. Therefore, you should first trim() the values and then test them. However, passwords are different. In fact, you should almost always trim() user entered data. But, passwords are a little different. Unlike trimming the value for a user's name, if you were to trim a password, it is really changing what the password is. Now, it could be argued that as long as you are always trimming the password upon creation and when the user logs in that it would always work - and that is true. But, removing anything from a password could decrease the complexity of the password. For example, let's say the user made their password "__password__", with two leading and trailing spaces. If you were to trim the value, then their password would just be "password" making them much more open to being  compromised.