Form validation and prepared statements

[Steven]

I went through my application and implemented prepared statements where needed, and all is well now. What I'm up to now is implementing form validation. I'm doing a little something like this:

// Check for form submissionif ($_SERVER['REQUEST_METHOD'] == 'POST') {  // Initialize an error array  $errors = array ();  // Check for a date entry  if (empty($_POST['datein'])) {    $errors[] = 'You forgot to enter a date';  } else {    $datein = trim($_POST['datein']);  }// rest of the stuff...}

After a stressful morning I've come to understand what's going on with that code example a bit (I'm getting this method from the Larry Ullman book I got recently).

 

My question is, with this validation method, where each of the posts (at least the ones that are validated) are turned into variables, does that get rid of the need of prepared statements when it comes time to execute the SQL query? Do I need prepared statements in this case?

[Steven]

Okay, here is a bigger question...

 

I got everything wrote out, sent a test submission with a few empty fields, and it still sent it to the database without error. Not what I want happening.

 

I can't see anything wrong with my code -- there certainly IS something wrong, but my newby eyes can't see it. What's going on with this code:

<div id="container">	<!-- Include Navigation -->	<?php include ('includes/header.php'); ?>	<!-- Include Navigation -->	<?php include ('includes/navigation.php'); ?>	<h2>Add a new job (Validation)</h2><?php// Check for form submissionif ($_SERVER['REQUEST_METHOD'] == 'POST') {	// Initialize an error array	$errors = array();	// Check for a client entry	if (empty($_POST['clientid'])) {		$errors[] = 'You forgot to enter a client';	} else {		$clientid = trim($_POST['clientid']);	}	// Check for a date entry	if (empty($_POST['datein'])) {		$errors[] = 'You forgot to enter the date';	} else {		$datein = trim($_POST['datein']);	}	// Check for a description entry	if (empty($_POST['description'])) {		$errors[] = 'You forgot to enter a description';	} else {		$description = trim($_POST['description']);	}	// If it's all good...	if (empty($errors)) {		// ...send form to the database		$q = "INSERT INTO jobs (clientid, datein, description) VALUES ($clientid, $datein, $description)";		$r = @mysqli_query ($con, $q); // run the query		if ($r) { // if $r ran with no errors			echo "<h2>Thank you!</h2>";			echo "<p>The job has been successfully recorded.</p>";		} else { // if $r ran with errors			echo "<h2>Oh, bother...</h2>";			echo "<p>Something goofed. Sorry about that.</p>";			// Debugging message			echo "<p>".mysqli_error($con)."</p>";			echo "<p>Query: ".$q."</p>";		} // end of $r IF		mysqli_close($con); // close the db connection	} else { // Report the errors		echo "<h2>Error!</h2>		<p>The following error(s) occurred:<br>";		foreach ($errors as $msg) { // Print each error			echo " - $msg<br> ";		}		echo "</p><p>Please try again.</p><p><br></p>";	} // end of (empty($errors)) IF} // end of the main Submit conditional?><!-- FORM!! --><div class="formContainer">	<form action="insertJobs.php" method="post">	<p>		<label>Client:</label>		<?php			// prepare statement			if ($stmt = mysqli_prepare($con, "SELECT * FROM clients")) {				mysqli_stmt_execute($stmt);				// bind variables to prepared statement				// list all* columns in order of tables selected!				mysqli_stmt_bind_result($stmt, $id, $name);				// fetch values				echo "<select name='clientid'>";					while (mysqli_stmt_fetch($stmt)) {						echo "<option value='".$id."'>".$name."</option>";					}				echo "</select>";			}    ?>    <br>		<label>Date Received:</label> 		<input type="date" name="datein" class="focus-glow datepicker job--dateinInput" size="23">    <span class="job--dateinHint">Date placed</span><br>				<label>Description:</label> 		<textarea type="text" maxlength="600" name="description" class="focus-glow"  size="28"></textarea>		<p>				<input type="submit" label="Submit">				<p>	</form>

[davej]

No, you always want to use prepared statements. Also you aren't really doing anything to sanitize this untrusted input. If you parse an input to an integer then you have sanitized it, but for strings it is a more complex design decision.

 

http://www.w3schools.com/php/php_filter.asp

 

I would be testing the string lengths of those inputs after trim to see if they are reasonable.

[Steven]

Thanks, I'll chew on that link for a while, and likely come back with more questions.

[thescientist]

what about adding some debug lines? why don't you see what exactly is in error's if you try and submit a form with no inputs? It's good to know what your code is doing line by line. This might enlighten you on what the values of things are as the code executes.

 

also, don't supress errors

 

$r = @mysqli_query ($con, $q);

 

You are already checking for errors which is good. A better option is turn on displaying errors when developing (make sure to turn it off on production though)

[justsomeguy]

My question is, with this validation method, where each of the posts (at least the ones that are validated) are turned into variables, does that get rid of the need of prepared statements when it comes time to execute the SQL query? Do I need prepared statements in this case?

Yes, you do. You're just copying the value from one variable to another one. That doesn't do anything to protect the database.

[Steven]

what about adding some debug lines? why don't you see what exactly is in error's if you try and submit a form with no inputs? It's good to know what your code is doing line by line. This might enlighten you on what the values of things are as the code executes.

 

I tried sending a job with "date" and "description" left empty. There is only one other field, "client". This client field is a dropdown, and can't very-well be left blank. Is my code written in such a way that causes it to send the query to the database even if just one field is filled in? It isn't even showing me the error reports for the two fields left empty.

 

Also, I'm pretty sure I have error reporting turned on.

Edited March 26, 2014 by Steven

[Steven]

I changed the form's action from insertJob.php to the current page the form is on (I overlooked that in the example from the book). Well, it's sort of working now.

 

I can see the error reporting now, which is nice. It's successfully telling me when and where errors are happening. But, it's having trouble with the sql query. It's throwing this error at me:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '?,?,?)' at line 1Query: INSERT INTO jobs (clientid, datein, description) VALUES (?,?,?)

Here is the section with my prepared stmt:

	// If it's all good...	if (empty($errors)) {		// ...send form to the database		// Make the query		$q = 'INSERT INTO jobs (clientid, datein, description) VALUES (?,?,?)';		// Prepare the statement		$stmt = mysqli_prepare($con, $q);		// Bind the variables		mysqli_stmt_bind_param($stmt, 'iis', $clientid, $datein, $description);		// Asign the values to variables		$clientid = $_POST['clientid'];		$datein = $_POST['datein'];		$description = $_POST['description'];		// Execute		mysqli_stmt_execute($stmt);		// Print a message based on the result		if (mysqli_stmt_affected_rows($stmt) == 1) {			echo '<p>Great! A new job has been added.</p>';		} else {			echo '<p>Sadly, the query could not be exeucted.</p>';			echo '<p>'.mysqli_stmt_error($stmt).'</p>';		}		// Close the statement		mysqli_stmt_close($stmt);		$r = @mysqli_query ($con, $q); // run the query		if ($r) { // if $r ran with no errors			echo "<h2>Thank you!</h2>";			echo "<p>The job has been successfully recorded.</p>";		} else { // if $r ran with errors			echo "<h2>Oh, bother...</h2>";			echo "<p>Something goofed. Sorry about that.</p>";			// Debugging message			echo "<p>".mysqli_error($con)."</p>";			echo "<p>Query: ".$q."</p>";		} // end of $r IF		mysqli_close($con); // close the db connection	} else { // Report the errors		echo "<h2>Error!</h2>		<p>The following error(s) occurred:<br>";		foreach ($errors as $msg) { // Print each error			echo " - $msg<br> ";		}		echo "</p><p>Please try again.</p><p><br></p>";	} // end of (empty($errors)) IF

There is another funny error showing up. When the page shows these errors after having tried to submit an entry, my clientid dropbox disappears and shows this error in its place:

Warning: mysqli_prepare(): Couldn't fetch mysqli in C:xampphtdocsmos-logformJobsValidate.php on line 165

And here's the relevant PHP for that:

		<?php			// prepare statement			if ($stmt = mysqli_prepare($con, "SELECT * FROM clients")) {				mysqli_stmt_execute($stmt);				// bind variables to prepared statement				// list all* columns in order of tables selected!				mysqli_stmt_bind_result($stmt, $id, $name);				// fetch values				echo "<select name='clientid'>";					while (mysqli_stmt_fetch($stmt)) {						echo "<option value='".$id."'>".$name."</option>";					}				echo "</select>";			}    ?>

And... another weird thing. This submission actually made its way to the database, despite the error message. However, once it got there, the date is empty, even though I picked a date with the datepicker.

 

---- EDIT -------

 

Okay, well I fixed the issue where the dropbox disappeared. Earlier in the code, where I pasted in the $stmt blocks from another page, there was a call to close the database connection. I got rid of that, and now it is working.

 

I'm still completely stumped as to why I'm getting query errors when the query is still being added, and why the date field is being read as "0000-00-00" when I enter a date using the jquery datepicker plugin.

Edited March 26, 2014 by Steven

[thescientist]

you are doing things out of order here

mysqli_stmt_bind_param($stmt, 'iis', $clientid, $datein, $description); // Asign the values to variables$clientid = $_POST['clientid'];$datein = $_POST['datein'];$description = $_POST['description'];

you're using your variable in the bind_params function before you've assigned them.

 

Also, I again urge you to remove the error suppression from this line

$r = @mysqli_query ($con, $q); // run the query

displaying errors (not in production though) is a good practice. this is good to have at the top of your script

ini_set('display_errors', 'on');

[Steven]

you are doing things out of order here

 

It looked out of order to me, but the manual lays it out this same way: http://us.php.net/manual/en/mysqli-stmt.bind-param.php

 

 

Also, I again urge you to remove the error suppression from this line

 

Could you explain how $r is suppressing errors?

 

As far as I know, it is what allows reporting these errors:

		if ($r) { // if $r ran with no errors			echo "<h2>Thank you!</h2>";			echo "<p>The job has been successfully recorded.</p>";		} else { // if $r ran with errors			echo "<h2>Oh, bother...</h2>";			echo "<p>Something goofed. Sorry about that.</p>";			// Debugging message			echo "<p>".mysqli_error($con)."</p>";			echo "<p>Query: ".$q."</p>";		} // end of $r IF

I'm not arguing it at all, I just don't really know any better and want to understand what you are saying better.

[Steven]

Alright, well I don't understand it, but I changed

		$r = @mysqli_query ($con, $q); // run the query		if ($r) { // if $r ran with no errors			echo "<h2>Thank you!</h2>";			echo "<p>The job has been successfully recorded.</p>";		} else { // if $r ran with errors			echo "<h2>Oh, bother...</h2>";			echo "<p>Something goofed. Sorry about that.</p>";			// Debugging message			echo "<p>".mysqli_error($con)."</p>";			echo "<p>Query: ".$q."</p>";		} // end of $r IF

to

		if ($q) { // if $r ran with no errors			echo "<h2>Thank you!</h2>";			echo "<p>The job has been successfully recorded.</p>";		} else { // if $r ran with errors			echo "<h2>Oh, bother...</h2>";			echo "<p>Something goofed. Sorry about that.</p>";			// Debugging message			echo "<p>".mysqli_error($con)."</p>";			echo "<p>Query: ".$q."</p>";		} // end of $r IF

And it's running fine, without those SQL query errors. Thanks!

 

However, it's still not registering the dates, and I still can't tell what's going on there.

[justsomeguy]

you're using your variable in the bind_params function before you've assigned them.

That actually doesn't matter, the variables are passed by reference. The reason for that is so that you can prepare a statement once, and run it in a loop after assigning new values to the parameters. That's the other use for prepared statements other than avoiding SQL injection - the prepared statement is faster when you run it in a loop versus sending the same query to the server over and over. The prepared statements are actually handled by MySQL as such. The mysqli API does not build that query by actually substituting the parameter values into the query string, instead it sends the prepared query and the parameters separately to MySQL. That's why SQL injection is not a problem - the parameter values are sent to the server separately from the query, and MySQL itself does the necessary escaping.

Warning: mysqli_prepare(): Couldn't fetch mysqli in C:xampphtdocsmos-logformJobsValidate.php on line 165

According to comments in the PHP manual, it sounds like that means you closed the database connection and then tried to use it again.

Could you explain how $r is suppressing errors?

It's not $r, it's the @ operator. That's the error suppression operator.http://www.php.net/manual/en/language.operators.errorcontrol.php

mysqli_stmt_bind_param($stmt, 'iis', $clientid, $datein, $description);

You're saying the date is an integer, is that correct? Is that an integer field in the database and the date value is a Unix timestamp?

[Steven]

It's not $r, it's the @ operator. That's the error suppression operator.http://www.php.net/manual/en/language.operators.errorcontrol.php

 

Gotchya, thanks!

 

 

You're saying the date is an integer, is that correct? Is that an integer field in the database and the date value is a Unix timestamp?

 

It's actually a date type in the database. Should that be an 's' then, and not an 'i'?

 

--- EDIT ------

 

Yes, it appears as though it does need to be a string. I was confused for a while, because I wasn't ever noticing in the php manual anywhere saying dates need to be strings.

Edited March 27, 2014 by Steven

[thescientist]

That actually doesn't matter, the variables are passed by reference. The reason for that is so that you can prepare a statement once, and run it in a loop after assigning new values to the parameters. That's the other use for prepared statements other than avoiding SQL injection - the prepared statement is faster when you run it in a loop versus sending the same query to the server over and over. The prepared statements are actually handled by MySQL as such. The mysqli API does not build that query by actually substituting the parameter values into the query string, instead it sends the prepared query and the parameters separately to MySQL. That's why SQL injection is not a problem - the parameter values are sent to the server separately from the query, and MySQL itself does the necessary escaping.

 

TIL!