INSERT failing on my query but not on PHPMyAdmin generated query

[Karaethon]

I am trying to centralize my database communicatin into a class (im getting tired of typing and retyping the code to send queries) Its not done but I cant get INSERT to work. 

Heres my Class:

<?php
######################################################
# Class Name: DBConnect
# Description: Base Class to handle DB inquiries
# Created: 06/27/19
# Updated: 06/27/19
# Author: James 'Karæthon' Cantando
# Contact: TheKaraethon@gmail.com
######################################################
class DBConnect{
######################################################
# PROPERTIES
######################################################
# PRIVATE
	private const HOST = 'localhost';
	private const USER = 'root';
	private const PASS = '';
	private const DB = 'crackthecode';
	private $link;
	private $table;
# PUBLIC

######################################################
# METHODS
######################################################
# CONSTRUCTOR
	function __construct($target){
		$this -> table = $target;
		$this -> link = mysqli_connect(self::HOST,self::USER,self::PASS,self::DB);
		if(!$this->link){
		die("An error occured while attempting to connect to the table \"{$this -> table}\" on the ".self::DB." database.<br />Error#: ".mysqli_connect_errno()."<br />Description: ".mysqli_connect_error());
		}
	}
# PRIVATE
	private function checkValue($val){
		switch(strtoupper($val)){
			case 'NULL':
				return 'NULL, ';
				break;
			case 'CURRENT_TIMESTAMP':
				return 'CURRENT_TIMESTAMP, ';
				break;
			default:
				return "`".$val."`, ";
		}
	}

# PUBLIC
	public function getData($criteria){
		$query = "SELECT * FROM {$this->table} WHERE ";
		$crit = "";
		foreach($criteria as $column => $value){
			if(($column === 'bind')){
				$crit .= "{$value} ";
			}else{
				$crit .= "{$column} = {$value} ";
			}
		}
		$result = mysqli_query($this->link, $query.$crit);
		if(mysqli_num_rows($result) !== 0){
			return $result;
		}else{
			return false;
		}
	}

	public function newData($data){
		$query = "INSERT INTO `{$this->table}` ";
		$cols = "(";
		$vals = "(";
		foreach($data as $col => $val){
			$cols .= "`".$col."`, ";
			$vals .= $this->checkValue($val);
		}
		$query = $query.substr($cols,0,-2).") VALUES ".substr($vals,0,-2).")";
		echo $query;
		echo "<br />INSERT INTO `players` (`PlayerID`, `CreatedDate`, `Username`, `Passcode`, `Email`, `FName`, `Lname`, `Addr1`, `Addr2`, `City`, `State`, `Country`, `PostalCode`, `Phone`, `PhoneType`, `TaxpayerID`, `DOB`, `TokenBalance`) VALUES (NULL, CURRENT_TIMESTAMP, 'test', 'ggb', 'hyh', 'yjj', 'hjj', 'ghu', 'ghj', 'tuo', 'dgi', 'fgu', 'iyh', 'ghk', 'Other', 'tujk', '2019-6-17', '0')";
		$insert = mysqli_query($this->link, $query);
		//$insert = mysqli_query($this->link, "INSERT INTO `players` (`PlayerID`, `CreatedDate`, `Username`, `Passcode`, `Email`, `FName`, `Lname`, `Addr1`, `Addr2`, `City`, `State`, `Country`, `PostalCode`, `Phone`, `PhoneType`, `TaxpayerID`, `DOB`, `TokenBalance`) VALUES (NULL, CURRENT_TIMESTAMP, 'test', 'ggb', 'hyh', 'yjj', 'hjj', 'ghu', 'ghj', 'tuo', 'dgi', 'fgu', 'iyh', 'ghk', 'Other', 'tujk', '2019-6-17', '0')");
		if(!$insert){
			echo "An error occured while attempting to INSERT into the table \"{$this -> table}\" on the ".self::DB." database.<br />Error#: ".mysqli_connect_errno()."<br />Description: ".mysqli_connect_error();
		}
		return $insert;
	}



######################################################
# End Class
######################################################
}
?>

The testing page has this:

<?php
	include '../includes/dbconn.class';
	$test = new DBConnect("players");
	$inserted = $test->newData([
		'playerID'=> 'NULL',
		'CreatedDate'=> 'CURRENT_TIMESTAMP',
		'Username'=>'test',
		'Passcode'=>'123456789',
		'Email'=>'test@test.test',
		'FName'=>'test',
		'Lname'=>'tested',
		'Addr1'=>'1234 msin st',
		'Addr2'=>'NULL',
		'City'=>'Anytown',
		'State'=>'Denial',
		'Country'=>'United States',
		'PostalCode'=>'12345-6789',
		'Phone'=>'19995551212',
		'PhoneType'=>'Other',
		'TaxpayerID'=>'123-45-6789',
		'DOB'=>'2019-6-17',
		'TokenBalance'=>'1000000000'
		]);
	if($inserted){echo 'Insert succeeded.';}else{echo 'Insert failed.';}
?>

In the class file you will see two queries, one code generated the other from a successful phpmyadmin query. The phpmyadmin one works everytime, but the generated doent. and I cant see any difference between them when echoed except the values being inserted. please what am I doing wrong?

Output from echoed queries:

Generated Query: INSERT INTO `players` (`playerID`, `CreatedDate`, `Username`, `Passcode`, `Email`, `FName`, `Lname`, `Addr1`, `Addr2`, `City`, `State`, `Country`, `PostalCode`, `Phone`, `PhoneType`, `TaxpayerID`, `DOB`, `TokenBalance`) VALUES (NULL, CURRENT_TIMESTAMP, `test`, `123456789`, `test@test.test`, `test`, `tested`, `1234 msin st`, NULL, `Anytown`, `Denial`, `United States`, `12345-6789`, `19995551212`, `Other`, `123-45-6789`, `2019-6-17`, `1000000000`)

PHPMyAdmin Query: INSERT INTO `players` (`PlayerID`, `CreatedDate`, `Username`, `Passcode`, `Email`, `FName`, `Lname`, `Addr1`, `Addr2`, `City`, `State`, `Country`, `PostalCode`, `Phone`, `PhoneType`, `TaxpayerID`, `DOB`, `TokenBalance`) VALUES (NULL, CURRENT_TIMESTAMP, 'test', 'ggb', 'hyh', 'yjj', 'hjj', 'ghu', 'ghj', 'tuo', 'dgi', 'fgu', 'iyh', 'ghk', 'Other', 'tujk', '2019-6-17', '0')

notes mysql version: 10.0.33-MariaDB (? is that correct?)

 

Edited June 28, 2019 by Karaethon
added query dumps and mysql version

[mac_gyver]

since you should be using a prepared query with external/unknown data values, your current code is not general purpose and will need to be redone to make it secure. you will want to switch to the much simpler php PDO extension, since it requires fewer statements to accomplish any task and the result from prepared and non-prepared queries can be treated in the same way. also, you should be using exceptions to handle connection, query, prepare, and execute errors and in most cases let php catch and handle the exception, where it will use its error related settings to control what happens with the actual error information (database errors will get displayed or logged the same as php errors.) by unconditionally outputting the raw error information on a live/public server, you will only be helping hackers who intentionally trigger errors, since connection errors contain the db username, and all the errors contain server path information.

next, to dynamically produce sql statements, use arrays to hold the column/value terms and implode() the results. this will eliminate all the extra commas and substr() statements.

lastly, you apparently didn't look very closely at the echoed sql query statement. you are putting back-ticks ` around the values. do you know what using back-ticks in an sql context means?

 

 

[Karaethon]

29 minutes ago, mac_gyver said:

since you should be using a prepared query with external/unknown data values, your current code is not general purpose and will need to be redone to make it secure. you will want to switch to the much simpler php PDO extension, since it requires fewer statements to accomplish any task and the result from prepared and non-prepared queries can be treated in the same way. also, you should be using exceptions to handle connection, query, prepare, and execute errors and in most cases let php catch and handle the exception, where it will use its error related settings to control what happens with the actual error information (database errors will get displayed or logged the same as php errors.) by unconditionally outputting the raw error information on a live/public server, you will only be helping hackers who intentionally trigger errors, since connection errors contain the db username, and all the errors contain server path information.

next, to dynamically produce sql statements, use arrays to hold the column/value terms and implode() the results. this will eliminate all the extra commas and substr() statements.

lastly, you apparently didn't look very closely at the echoed sql query statement. you are putting back-ticks ` around the values. do you know what using back-ticks in an sql context means?

 

 

Thanks for the info, I'm not sure if it matters but this class will only be called by other classes whose job is to sanitize user input so i dont think i have to worry there and the error handling is at the moment only for testing server when placed on live server that will all be gone.

And nope I'm not sure about back-ticks i just kinda copied the phpmyadmin output and tried to match it.

 

OHHH I SEE IT! the phpmyadmin isnt using back-ticks! theyre single quotes! i had to zoom in to catch that.

Edited June 28, 2019 by Karaethon
opened my eyes.

[Karaethon]

And with that it's fixed... so hard to see the diference at my screen size and resolution...

[mac_gyver]

10 minutes ago, Karaethon said:

sanitize user input

nope. you should validate input only, not alter it. for any sanitation you can come up with, there are libraries of things hackers use that can get past it.

the only fool-proof way of preventing anything in data from being treated as sql syntax is to use a true prepared query (PDO has emulated prepared queries that should be avoided.) prepared queries also eliminate all the single-quotes around values and all the concatenation, extra quotes for concentration, and any {} that people have used to get values into the sql query statement.

 

10 minutes ago, Karaethon said:

that will all be gone.

don't waste your time writing and then changing code just because the context changes. write code once and leave it alone.

 

Edited June 28, 2019 by mac_gyver

[Karaethon]

2 minutes ago, mac_gyver said:

nope. you should validate input only, not alter it. for any sanitation you can come up with, there are libraries of things hackers use that can get past it.

the only fool-proof way of preventing anything in data from being treated as sql syntax is to use a true prepared query (PDO has emulated prepared queries that should be avoided.) prepared queries also eliminate all the single-quotes around values and all the concatenation, extra quotes for concentration, and any {} that people have used to get values into the sql query statement.

 

don't waste your time writing and then changing code just because the context changes. write code once and leave it alone.

 

hmm, ok i need to learn about these prepared queries i think...

and i guess i thought sanitize = validate, but i guess there is a difference

i know i SHOULD write once, but I tend to be a bit thick-headed so i put in exra verbose error code so i know whats wrong at code time... 

Ieally need to break my bad habits and develop beter ones, thanks for the input.

[mac_gyver]

the code you are currently producing is where we were back when using the mysql_ extension. it took a lot of code to securely handle each different type of data being put into the sql query statement and a lot of code to provide error handling for all the statements that can fail. by using prepared queries, the simple and consistent PDO extension, and exceptions to handle errors, most of the implementation detail code disappears. you only have to validate that data meets the needs of the application, which in most cases is just to make sure it is not an empty value or that is has an expected format, form the sql query statement, with place-holders for any external/unknown data values and array of input data values, then either call the prepare/execute methods or the query() method (which you can combine by extending the PDO class with a general purpose query method that accepts an optional 2nd array parameter of input values) to run the query.

letting php handle the exception/error will give you all the file name, line number, and sql error information, that will either get displayed or logged based on the php error settings, without requiring any conditional logic in your code or ever touching the code after it is written. this just takes one line of code to set the error mode to exceptions when you make the connection. (lol i just noticed that you are using the connection error statements in your insert query error handling.)

[gizmola]

The first issue with SQL is what happens when your input contains characters that are special to SQL.  Quotes are the primary issue.  In the old days you had to "escape" any strings, because they could have quotes that cause the SQL statement to become invalid.  Consider:

Quote

So I said to them 'escape your strings' and they laughed!

If the SQL is:

INSERT INTO my_table (comment) VALUES ('$input')

Right away you should see the issue.  It quickly gets out of hand when you add in character sets and different combinations of quotes.  The old way was to "escape" all the quotes before you stored them, which went through the input and tried to find all the characters that could mess up your flavor of database and add "escape" characters.  The oldest version of this was addslashes which would inject backslashes into your input at each character.

Even when this works, it's annoying because now you have screwed up your original input by adding a bunch of slashes that have to be removed when you read the value back out of the database.

For this reason alone, using prepared statements is fantastic, as the entire issue of escaping, as well as the possibility of executing a SQL injection goes away.  There is really no excuse not to do it as it's easier, less complicated and secures your code from SQL injection exploits.

Everything else that Mac advised I 100% agree with.

[Karaethon]

45 minutes ago, mac_gyver said:

the code you are currently producing is where we were back when using the mysql_ extension. it took a lot of code to securely handle each different type of data being put into the sql query statement and a lot of code to provide error handling for all the statements that can fail. by using prepared queries, the simple and consistent PDO extension, and exceptions to handle errors, most of the implementation detail code disappears. you only have to validate that data meets the needs of the application, which in most cases is just to make sure it is not an empty value or that is has an expected format, form the sql query statement, with place-holders for any external/unknown data values and array of input data values, then either call the prepare/execute methods or the query() method (which you can combine by extending the PDO class with a general purpose query method that accepts an optional 2nd array parameter of input values) to run the query.

letting php handle the exception/error will give you all the file name, line number, and sql error information, that will either get displayed or logged based on the php error settings, without requiring any conditional logic in your code or ever touching the code after it is written. this just takes one line of code to set the error mode to exceptions when you make the connection. (lol i just noticed that you are using the connection error statements in your insert query error handling.)

Hmm, ok... well I started playing with php/mysql back in 2003, then had a minor 15 year vacation... so i know im behind the times... where can i go to read/learn this new PDO stuff? I'm currently using my old 'PHP, MySQL, Javascript, & HTML5 all-in-one for dummies' as a basic reference, I got it on that "vacation" to try to stay in the loop but i know its old now itself. (copyrighted 2013).

Edited June 28, 2019 by Karaethon

[Karaethon]

Ok... so I googled PDO and found a phpdelusions.net about it, will this be a good introduction for me?

[mac_gyver]

13 minutes ago, Karaethon said:

Ok... so I googled PDO and found a phpdelusions.net about it, will this be a good introduction for me?

that's a good introduction that can be boiled down to the following for select, insert, update, and delete queries -

1) when you make the connection, set the character set to match your database tables, set the error mode to exceptions, set emulated prepared queries to false, and set the default fetch mode to assoc.

2) if there are no external/unknown values being put into the sql query statement, just use the PDO query() method. this returns a PDOStatement object for accessing the result from the query.

3) if there are external/unknown values being put into the sql query statement, use a ? place-holder for each value in the sql query syntax (without single-quotes around it/them) and add each value to an array. call the PDO prepare() method, which returns a PDOStatement object, same as for the above item, and then call the PDOStatement execute() method, with the array of input values as a parameter.

Items #2 and #3 can be combined into a single user written method, so that you can have a common single-point call to use throughout your code.

[Karaethon]

Alright, I've been reading the article and I have a question you might know the answer to: When using a PDO prepared statement to INSERT via an assosiative array do I need to put the inserted columns in the array in the same order they are in the table?

[mac_gyver]

if you mean named place-holders, no, the order doesn't matter. they are matched via their names.

[Karaethon]

26 minutes ago, mac_gyver said:

if you mean named place-holders, no, the order doesn't matter. they are matched via their names.

No, I meant if the table has col1, col2, col3... do I have to make the PDO query in the same order? i understand that it will bind my values regardless of where they are in the array, I just want to know do I have to put the INSERT order to match the table order.

i.e: INSERT INTO table (col1, col2, col3) VALUES ( :arg1, :arg2, :arg3) where the data was [arg1=>'test1', arg2=>'test2', arg3=>'test3']

OR could I use

INSERT INTO table (col3, col1, col2) VALUES ( :arg3, :arg1, :arg2) where the data was [arg1=>'test1', arg2=>'test2', arg3=>'test3']

[Karaethon]

ok, been playing with pdo... i get a good connection (i believe, because no errors are being thrown) but my attempt to query is not working...

<?php
	include '../includes/dbconn.inc';
	$player = $link->prepare("SELECT * FROM players WHERE PlayerID=?")->execute(['1']);
	echo $player;
	echo $player->fetch();
?>

is failing,  at first echo $player is: 1

at fetch error is thrown: 'Call to a member function fetch() on boolean

whats going on?

Edited June 29, 2019 by Karaethon
typo

[kicken]

You can't chain prepare and exec like that for a query where you want results.

execute() returns a boolean, true if it succeeded or false if it failed.  To get the results you have to call fetch on the statement object that is returned by prepare.  As such, you have to assign the result of prepare, then call the execute method separately.

<?php

include '../includes/dbconn.inc';
$player = $link->prepare("SELECT * FROM players WHERE PlayerID=?");
$player->execute(['1']);

var_dump($player->fetch());

 

[kicken]

3 hours ago, Karaethon said:

No, I meant if the table has col1, col2, col3... do I have to make the PDO query in the same order?

You don't have to specify the columns in your insert in the same order they exist in the table if that's what you mean.  You can even leave columns out entirely if they have a default value or can be NULL.

All that matters is that the values match up with the appropriate columns in the two lists.

As far as placeholders go, if you're using :name style then order doesn't mater.  If you're just using ? then order maters.

[Karaethon]

This thread has evolved into a PDO lesson (for me), should I start  a new topic specifically for my PDO questions?

[gizmola]

40 minutes ago, Karaethon said:

This thread has evolved into a PDO lesson (for me), should I start  a new topic specifically for my PDO questions?

 Well, if you have a specific PDO question, then it would be better for future readers to do a new topic.  If we are just cleaning up what you've been working on and it's dwindling down, then no worries, and continuing the topic has the advantage that those who have already been helping you will get notifications.