change_password function

[Stooney]

This is the change_password() function of my User class I'm working on.  I'm just hoping someone can see if I'm headed in the right direction.  My main question is if the form has not been submitted, should I show the form from inside the class?  (This question is marked near the bottom of the function) Here's the function:

 

Note:  I haven't tested this, so there may be minor typos, ignore those.  I will fix them later.

 

<?php
        public function change_password(){
	if(isset($_POST['old_password'])){
		global $dbc;
		global $user;
		//Form was submitted, sanitaze user input
		$data=$dbc->sanitize(array($_POST['old_password'], $_POST['new_password1'], $_POST['new_password2']));
		//Check if old password matches
		$check=$dbc-query("SELECT id FROM ".$CONFIG['db_prefix']."_users WHERE id='$user->userid' AND password=PASSWORD('$data[0]')");
		if($dbc->numrows($check)==1){
			//Old password was a match, validate and set the new password.
			if(strlen($data[1])>5 && $data[1]==$data[2]){
				//Password is greater than 6 characters and both match
				$update=$dbc->query("UPDATE ".$CONFIG['db_prefix']."_users SET password=PASSWORD('$data[1]') WHERE id='$user->userid'");
				if($dbc->affected()==1){
					//Password was updated successfully
					return true;
				}
			}
			else{
				//New password wasn't valid
			}
		}
		else{
			//Old password didn't match
		}
	}
	else{
		//Form has not been submitted
		//*****************Should I show the form here?  Or should that be done outside of the class?
	}
}
?>

[Naez]

In my opinion, put all the contents of the form into "form.html" (or whatever), then just include it... to make it easier for maintainers and such

[able]

I think you should check if the form is posted outside of the function and do the include of the form outside.

 

I don't understand why the function loads from the $_POST array directly. What if you wish to reuse this function to change passwords for another reason? You would have to send them to the $_POST array first.

[Stooney]

Yea I've already changed the post thing, I caught that pretty quick.  Not sure why I did it in the first place.  And I'll just go ahead and include forms from external files.  That seems to be the general consensus.

[aschk]

Try to avoid the use of $_POST/$_GET/_$SESSION inside your class functions.

Instead consider the following (i'll leave out code specifics):

 

<?php

public function change_password($old,$new){
  // Look up old password
  if($old == $db_old){
    // update password to new
    return true;
  } else {
    return false;
  }
}
?>

 

then

$user = new user();
if($user->change_password($_POST['old'], $_POST['new'])){
  echo "yay you changed it";
} else {
  echo "your old password was wrong";
}