Creating new user keeps failing

Hello All,
I am working on a PHP website with MySQL DB. Initially the code was working, and but as I carried on working on it, the create new user function stopped working, I am not sure if I changed something in it to cause this. The message on the website pulls through the successfully created message, but the DB is not updated. I can return the SQL code and enter it manually to PHPmyAdmin, and see it fails to create because the ID (A-I PK), is being returned as a ‘’ due to the rest of the values going through a “cleaning” function before inserting into DB. I am sure this was always the case, but clearly something is wrong now as values I am not wanting to insert it is trying to insert as the fields are being pulled from the table.

Here is the code
the webpage uses the save function, update works as it should, so dont think there are issues behind this:

 }
    	private function has_attribute($attribute){
    		// get_object_vars returns an associative array with all attributes
    		//(incl. private ones!!) as the keys and their current values as the value
    		
    		$object_vars = $this->attributes();
    		
    		return array_key_exists($attribute, $object_vars);
    	}
    	
    	protected function attributes(){
    		// return an array of attribute keys and their values
    		$attributes = array();
    		foreach(static::$db_fields as $field){
    			if(property_exists($this, $field)){
    				$attibutes[$field]  = $this->$field;
    			}
    		}
    		return get_object_vars($this);
    	}
    	
    	protected function sanitized_attributes(){
    		global $database;
    		$clean_attributes = array();
    		// Clean values before submitting
    		foreach($this->attributes() as $key => $value){
    			$clean_attributes[$key] = $database->escape_value($value);
    		}
    		return $clean_attributes;
    	}
    public function save(){
    		// A new record won't have a valid id
    		return isset($this->id) ? $this->update() : $this->create();
    	}
    	
    	protected function create(){
    		global $database;
    		
    		$attributes = $this->sanitized_attributes();
    		$sql = "INSERT INTO ".static::$table_name." (";
    		$sql .= join(", ", array_keys($attributes));
    		$sql .= ") VALUES ('";
    		$sql .= join("', '", array_values($attributes));
    		$sql .= "')";
    		var_dump($sql);
    		return($sql);
    		if($database->query($sql)){
    			$this->id = $database->insert_id($sql);
    			$message = "Success.";
    			return $message;
    		} else {
    			return false;
    		}
    	}
    	
    	protected function update(){
    		global $database;
    		
    		$attributes = $this->sanitized_attributes();
    		$attribute_pairs = array();
    		foreach($attributes as $key => $value){
    			$attribute_pairs[] = "{$key}='{$value}'";
    		}
    		$sql  = "UPDATE ".static::$table_name." SET ";
    		$sql .= join(', ', $attribute_pairs);
    		$sql .= " WHERE id='". $database->escape_value($this->id) . "'";
    		$database->query($sql);
    		return($database->affected_rows($database) == 1) ? True : False;
    	}

    the webpage form: 
    $reg_errors = array();
    $user = new User();
    if(isset($_POST['submit'])){
    	include('forms/validate/reg_validate.php');		
    	if(empty($reg_errors)){	
    	var_dump($user);
    		$user->first_name = $_POST['first_name'];
    		$user->last_name = $_POST['last_name'];
    		$user->email_address = $_POST['email_address'];
    		$password = $user->pass_enc($_POST['password']);
    		$user->password = $password;
    		$user->receive_emails = $_POST['receive_emails'];
    		$user->contact_number_1 = $_POST['contact_number_1'];
    		$user->contact_number_2 = $_POST['contact_number_2'];
    		$user->save();
    		$session->message("USER CREATED!");
    		//redirect_to('index2.php');
    	}
   
DB functions
	public function query($sql){
		$result = mysqli_query($this->connection, $sql);
		$this->confirm_query($result);
		return $result;
	}
	
	public function escape_value($string){		
		$escaped_string = mysqli_real_escape_string($this->connection, $string);
		return $escaped_string;
	}

if i var dump user on this page before it is passed to the functions id = null
after attributes are cleaned, id = ‘’

let me know if there is more code needed

the user is not created as ‘’ cannot be inserted as a primary key

This doesn’t give us the full picture; Please post all the code involved, so we can see what’s going on.

posted a bit more, if there are any sections you need more detail on please feel free to point it out

For the posted code to return the ‘Success.’ message, the query is (apparently) being executed without error (depending on how well the $database->query() method is written.) How about posting the code for that method?

Normally (in non-strict mode), an empty string ‘’ supplied as the value for an auto-increment column will work. When you copy/pasted the sql into phpmyadmin, did it in fact produce an error and if so post the sql statement and the error you got from it?

Based on the limited information you have shown, the most likely problem is you have multiple databases and the one that you are looking in isn’t the one where the data is actually being inserted into.

Please use either bbocde [code][/code] tags or markdown (three back-ticks ```), on lines of their own before/after the code, around your code when posting it. Edit: Which I see you have now done.

1 Like

The statement that is being sent to insert_id:

INSERT INTO users (id, first_name, last_name, email_address, password, profile_pic, profile_pic_path, profile_visible, receive_emails, contact_number_1, contact_number_2, show_contact) VALUES ('', 'test2', 'Test2', '[email protected]', '$2y$10$NGRlYzBjNjZiY2JmOTdiNeaOszvh.wPAY8nLA9NDsDjBlYz2FTezi', '', '', '', '1', '1111111111', '2222222222', '')

the error:
#1366 - Incorrect integer value: '' for column 'id' at row 1

the db config file has never changed… and with it pulling the correct fields i assume it is looking in the right db…

I’m going to guess that the above is the success message you are citing? The code itself is not using the returned value from calling the $user->save(); method, so it will always report that the user was created. The database’s confirm_query() method is probably doing something with the fact that the query failed, but the code calling the save() method isn’t.

Most query errors are fatal problems, due to programming mistakes. Code execution should stop in these cases. The only query errors that are recoverable, and code execution should continue, are when inserting/updating duplicate or out of range user data. In these cases, you should setup a message telling the user exactly what was wrong with the data that they submitted.

Sorry for how this sounds, but this is a lot of code and typing for nothing. It looks like someone took some main procedural style code and added class definition(s) around it. This is not OOP. Within all that extra clutter of defining and calling syntax, the code missed the point that it is not testing if the user row was inserted before reporting that the user was successfully created. The code is also missing the point that if the user already exists (it appears that the email address is the unique user identifying value, and that column in the database table should be defined as a unique index), that this is a recoverable sql error, that the user should be told about, and allowed to correct.

So, back to the main issue. The sql error you are getting is because the database server mode is set to strict mode (invalid values result in an error, rather than being converted to the nearest valid value.) If this mode hasn’t been changed, then if this ‘worked’ before, it was because the id column was completely left out of the query (the preferred way of doing this) or the escape_value() method was testing for and respecting null values. What’s the code for the escape_value() method? (You can see why all this extra code isn’t adding any value and why it is making debugging harder.)

Hi, I added it to the end of the code section… but feel I am just starting to waste time now, the code is defiantly a huge mess, when I started it, in 2014, it was making sense, but once it broke, i keep coming back to it, messing around, getting more confused and giving up all together for a few more months. I’m starting to feel that this will never work.

All the validations are done on the actual webpage, so no duplicates or data that is being request is pulled incorrect, but those are such chaos I don’t want to even get into those… but yes none of the values but what is posted should “cleaned”

Client-side validation is only a nicety for legitimate visitors. Data submitted to the server can come from anywhere, can be anything, and cannot be trusted. You must validate data on the server before using it.

As to duplicate values, your database must enforce uniqueness. From the time you check if a value does/doesn’t exist, to the time you actually execute the insert/update query, thousands of queries can be executed on the database server that could result in duplicate data.

Your code is ‘dynamically’ building the sql query statements (though you should be using a prepared query, because the escape_string function is open to sql injection if the character set that php is using is not the same as your database table(s), and you cannot threat the data as a string for some values), so, you have a definition (array?) of what columns to use. This definition currently contains the id column. Has that always been the case? Removing the id column from the definition, for both the insert and the update query won’t affect the query code and will eliminate the current insert error.

1 Like

The validation makes sense, as I said, was done long ago… I think how i did it is validate submitted data against what exists in the DB, do a basic clean up of data, then feed it to the database but first run it through that escape value and tuning into an array to be used… my concern with using the email as PK I assume was I allow the email address to be changed (my knowledge is very limited as you picking up, and assume you don’t want your PK to vary).
I am maybe thinking from your comment I learnt how to make the dynamic queries (which is perfect for the updating statement), maybe I tried to carry that across the logic to the create function and never noticed as the webpage feeds the success message regardless of the sql outcome as you point out…
Do I then take the solution as changing create to a predefined function only… or is it better to recreate the whole web interface, and then any advise on where to look to better learn set this up?
Thank you for your your time with looking through all that

I’m know I’m late to the thread, but here’s how I do this using PDO.

public function create():bool
{

    /* Initialize an array */
    $attribute_pairs = [];

    /*
     * Setup the query using prepared states with static:$params being
     * the columns and the array keys being the prepared named placeholders.
     */
    $sql = 'INSERT INTO ' . static::$table . '(' . implode(", ", array_keys(static::$params)) . ')';
    $sql .= ' VALUES ( :' . implode(', :', array_keys(static::$params)) . ')';

    /*
     * Prepare the Database Table:
     */
    $stmt = Database::pdo()->prepare($sql);

    /*
     * Grab the corresponding values in order to
     * insert them into the table when the script
     * is executed.
     */
    foreach (static::$params as $key => $value)
    {
        if($key === 'id') { continue; } // Don't include the id:
        $attribute_pairs[] = $value; // Assign it to an array:
    }

    return $stmt->execute($attribute_pairs); // Execute and send boolean true:

}

/*
 * This is the update that method that I came up with and
 * it does use named place holders. I have always found
 * updating is easier than creating/adding a record for
 * some strange reason?
 */
public function update(): bool
{
    /* Initialize an array */
    $attribute_pairs = [];
    
    /* Create the prepared statement string */
    foreach (static::$params as $key => $value)
    {
        if($key === 'id') { continue; } // Don't include the id:
        $attribute_pairs[] = "{$key}=:{$key}"; // Assign it to an array:
    }

    /*
     * The sql implodes the prepared statement array in the proper format
     * and updates the correct record by id.
     */
    $sql  = 'UPDATE ' . static::$table . ' SET ';
    $sql .= implode(", ", $attribute_pairs) . ' WHERE id =:id';

    /* Normally in two lines, but you can daisy chain pdo method calls */
    Database::pdo()->prepare($sql)->execute(static::$params);
    
    return true;

}

Note: I haven’t done data validation yet on it, but I just use the code for my own website.

1 Like

Thank you, this does make sense how the id key will always be skipped, I have managed to get around it now in a similar way (yours is better) but as there are more fields I don’t need for creating, so I unset the values in the function, so its quite messy, I can continue doing it across the tables and it should work, but is adding to more unnecessary code, and needing to relook at the PK fields as pointed out earlier.
So think the biggest lesson is importance of upfront DB design with this in mind would have kept things neater. I have added the updated code that is working for the create function if it helps anyone, but will prob take suggestions from your post.
lastly I was trying to add a proper error report with mysqli_error but not having much luck at with getting it to work with any above statement to make troubleshooting a bit easier, if you got any examples, Im happy to see in this environment.

new code:

	protected function create(){
		global $database;
		
		$attributes = $this->sanitized_attributes();
		unset($attributes['id']);
		unset($attributes['profile_pic']);
		unset($attributes['profile_pic_path']);
		unset($attributes['profile_visible']);
		unset($attributes['show_contact']);
		unset($attributes['area_province']);
		unset($attributes['company_id']);
		unset($attributes['company_name']);
		$sql = "INSERT INTO ".static::$table_name." (";
		$sql .= join(", ", array_keys($attributes));
		$sql .= ") VALUES ('";
		$sql .= join("', '", array_values($attributes));
		$sql .= "')";
		if($database->query($sql)){
			$this->id = $database->insert_id($sql);
			$message = $_SESSION['message'] = "Please log in.";
			return $message;
		} else {
			$message = $_SESSION['message'] = "Error: ";
			return $message;
			
		}
	}

New, and hopefully final version of the create DB entry function now looks like this, which should be friendlier to use across all tables, will just be error report to be updated.

	protected function create(){
		global $database;
		
		$attributes = $this->sanitized_attributes();
		foreach($attributes as $key => $value){
			if($value == ''){
				unset($attributes[$key]);
			} else {continue;}
		}
		$sql = "INSERT INTO ".static::$table_name." (";
		$sql .= join(", ", array_keys($attributes));
		$sql .= ") VALUES ('";
		$sql .= join("', '", array_values($attributes));
		$sql .= "')";
		if($database->query($sql)){
			$this->id = $database->insert_id($sql);
			$message = $_SESSION['message'] = "Please log in.";
			return $message;
		} else {
			$message = $_SESSION['message'] = "Error: ";
			return $message;
		}
	}
Sponsor our Newsletter | Privacy Policy | Terms of Service