[Last Call] Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 56
  • Last Modified:

Form validation using functions

I am trying to learn how to use functions to make my code neater by not having loads of php in amongst the html. It seems to be working okay so far for making queries and inserting/editing/delete data in the database. But I am having an issue with validation.

Before the function I had:

 
       $error = "";
	$success ="";

		if(isset($_POST['submit'])) {
		if (empty($_POST['cat_title'])){
			
			$error .= "Category cannot be empty";
			
		} else {

		$cat_title = $link->real_escape_string($_POST['cat_title']);
		
			
			$sql = "INSERT INTO `categories`(cat_title) VALUES('$cat_title')";
			if ($result = $link->query($sql) == TRUE) {

			$success .="record added";
				

Open in new window


As soon as I put that into a function and call it on the page I want it on, I get a notice:

Undefined variable: error in /Applications/MAMP/htdocs/cms/admin/categories.php on line 54

Line 54 is:

<div><?php echo $error.$success;?></div>

Open in new window


This used to work just fine before using the function to call this code.

I defined the variable in the function and am calling the function before the $error.$success code so why is it saying the variable is undefined?

Here is the function:

function insert_categories(){
	
	global $link;

	$error = "";
	$success ="";


		if(isset($_POST['submit'])) {
		if (empty($_POST['cat_title'])){
			
			$error .= "Category cannot be empty";
			
		} else {

		$cat_title = $link->real_escape_string($_POST['cat_title']);
		
			
			$sql = "INSERT INTO `categories`(cat_title) VALUES('$cat_title')";
			if ($result = $link->query($sql) == TRUE) {

			$success .="record added";

Open in new window


Perhaps I should rather exclude the validation from the function, perform it on the page and if it passes, then only call the function?
0
Black Sulfur
Asked:
Black Sulfur
  • 12
  • 6
  • 4
  • +2
2 Solutions
 
Ray PaseurCommented:
The issue is "variable scope."  Let me look for some references.  While I'm doing that, take a look at this article:
https://www.experts-exchange.com/articles/11769/And-by-the-way-I-am-New-to-PHP.html
1
 
Dave BaldwinFixer of ProblemsCommented:
Functions work by passing and returning data.  Since '$success' is defined inside the function and is not 'global', it is not available to the code outside the function.  The only reason that $_POST works is because it is declared a 'super-global' in PHP.

This page http://php.net/manual/en/functions.arguments.php shows how to pass and return arguments in PHP functions.  Normally the last line in your function would be 'return $success;' which passes it back to the line that called the function.
2
 
Julian HansenCommented:
Can you show us your full function - you have only posted some of it - which does not contain the error line. The error is saying that $error is not defined - the compiler is always right so you need to show us where this line is
<div><?php echo $error.$success;?></div>

Open in new window

because I am willing to bet it is not inside your function.


Aside: Don't use global - for too many reasons to mention. Either put your function inside a class and make the $link a property or pass the $link value into the function as a parameter.
1
Technology Partners: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
Marco GasiFreelancerCommented:
$error and $success are variable created within the function, so they are not visible outside the function itself: this is what we call variable scope

If you want print messages originated by a function you have to write the function in order it return thos messages:
function insert_categories() {

	global $link;

	$message = "";


	if (isset($_POST['submit'])) {
		if (empty($_POST['cat_title'])) {

			$message .= "Category cannot be empty";
		} else {

			$cat_title = $link->real_escape_string($_POST['cat_title']);


			$sql = "INSERT INTO `categories`(cat_title) VALUES('$cat_title')";
			if ($result = $link->query($sql) == TRUE) {

				$message .="record added";
			}
		}
	}
	return $message;
}

Open in new window

So you can write:
<div><?php echo insert_categories();  ?></div>

Open in new window

without any previous call to the function.
But you might want inprove function code to make it more concise and reusable: instead to leave the function use $_POST array, you could pass it the values it has to manage. After all, the function has to insert categories in the database so it needs only to get categories not the whole array. The advantage is that you can modify your code and retrieve categories not through $_POST but through $_GET and the function will work anyway withou any modification:
function insert_categories($category) {

	global $link;

	$message = "";

	if (empty($category)) {

		$message .= "Category cannot be empty";
	} else {

		$cat_title = $link->real_escape_string($category);

		$sql = "INSERT INTO `categories`(cat_title) VALUES('$category')";
		if ($result = $link->query($sql) == TRUE) {

			$message .="record added";
		}
	}
	return $message;
}
//usage with $_POST
if(isset($_POST['submit'])) {
	$category = $_POST['cat_title'];
	echo insert_categories($category);
}
//usage with $_GET
if(isset($_GET['submit'])) {
	$category = $_GET['cat_title'];
	echo insert_categories($category);
}

Open in new window

1
 
Ray PaseurCommented:
Check this, and please ask questions about it.  It's one of the most important things you can learn:
http://php.net/manual/en/language.variables.scope.php

See also:
http://php.net/language.functions

In this particular question-and-answer dialog, please learn that global is an antipractice.  PHP is a very old language, and a lot of the examples you will find online are old.  They are outdated and create security holes.  We did things differently a decade ago, and through experience we have come to learn that we were doing it wrong.  Unfortunately, all those old PHP examples are still hanging around on the internet, just waiting to poison the minds of new developers.  They do not rot away.

The opposite of the evil and stupid  global  is dependency injection.  It's a little hard to explain, but I've made an effort here:
https://www.experts-exchange.com/articles/19999/PHP-Design-Avoiding-Globals-with-Dependency-Injection.html
https://www.experts-exchange.com/articles/18210/Software-Design-Dependencies.html
1
 
Black SulfurAuthor Commented:
Wow, so many good answers. Sometimes giving points is harder than trying to understand the solutions!
0
 
Black SulfurAuthor Commented:
I am going to try work my way down the answers.

@Julian, that was the whole function but after what you said I did change it to include the $error.$success echo in the function itself like this:

<?php

function insert_categories(){
	
	global $link;

	$error = "";
	$success ="";
	
		if(isset($_POST['submit'])) {
		if (empty($_POST['cat_title'])){
			
			$error .= "Category cannot be empty";
			
		} else {

		$cat_title = $link->real_escape_string($_POST['cat_title']);
		
			
			$sql = "INSERT INTO `categories`(cat_title) VALUES('$cat_title')";
			if ($result = $link->query($sql) == TRUE) {

			$success .="record added";
				
		   
			}
		}
			echo $error.$success;
	}
}
?>

Open in new window


And then I just placed the call to the function in the html where I wanted the message to display.

                     
 <div class="form-group">
                   		<input type="submit" class="btn btn-primary "name="submit" value="Add Category">
                   		<div><?php insert_categories();?></div>
                   	</div>

Open in new window


The tutorial I was following used global. But it sounds like that is a bad idea!
0
 
Julian HansenCommented:
@Julian, that was the whole function
No if you go back you will see it cuts off after the if - there are 4 missing closing }

Yes as I mentioned global is a big no.

Your function is quite verbose - consider this alternative
<?php
function insert_categories($link)
{
  $response = "";

  $cat_title = empty($_POST['cat_title']) 
    ? $link->real_escape_string($_POST['cat_title']) 
    : false;
  if ($cat_title) {
    $sql = "INSERT INTO `categories`(cat_title) VALUES('{$cat_title}')";
    $result = $link->query($sql);
    if ($result) {
      $response = 'Record added';
    }
    //EDIT
    else {
      $response = 'Insert failed with error ' . $link->error;
    }
  }
  else {
    $response = 'Category  cannot be empty';
  }
  
  return $response;
}
// This gets called at the start of your script
$result = $insert_categories($link);
?>
...
<div class="form-group">
  <input type="submit" class="btn btn-primary "name="submit" value="Add Category">
  <div><?php echo $result?></div>
</div>

Open in new window


In the above
1. We are passing the $link to the function as a parameter instead of declaring it as global in the function
2. We are returning a response from the function (the message) which we assign to $response which is in the same scope as your HTML output
3. We are only using one response variable in the function - no need for an error and a success - in your logic it is not possible for success and error to have a value so keeping them separate makes no sense.

You may have to massage this to fit into your code as we have not seen the full picture but hopefully this will give you some insight into how to go about this.
0
 
Black SulfurAuthor Commented:
Sorry, you are right Julian. The reason I had 2 i.e.: $error and $success is that I wanted to have different classes for them so that the error had a red background and the success had a green background. That was my thinking behind having the 2 messages.
0
 
Ray PaseurCommented:
Yes, global is a bad idea.  There are many old, bad ideas on the internet.  The most pernicious ones frustrate automated testing.  Global declarations have been shown too be one of the big mistakes in software design.  If you want another view of how bad this problem can be, look at the way JavaScript handles undeclared variables!
0
 
Ray PaseurCommented:
Sidebar note:
$cat_title = empty($_POST['cat_title']) 
    ? $link->real_escape_string($_POST['cat_title']) 
    : false;

Open in new window

Should probably be:
$cat_title = !empty($_POST['cat_title']) 
    ? $link->real_escape_string($_POST['cat_title']) 
    : false;

Open in new window

0
 
Black SulfurAuthor Commented:
Thanks for everyones patience.

I am trying to avoid the usage of global as suggested but doing this gives me an error:

function insert_categories($link){

Open in new window


Warning: Missing argument 1 for insert_categories(), called in /Applications/MAMP/htdocs/cms/admin/categories.php on line 53 and defined in /Applications/MAMP/htdocs/CMS/admin/functions.php on line 3

line 53 is :

<div><?php echo insert_categories();?></div>

Open in new window


Line 3 is:

function insert_categories($link){

Open in new window

0
 
Julian HansenCommented:
Yup, or
$cat_title = empty($_POST['cat_title']) 
    ? false;
    : $link->real_escape_string($_POST['cat_title']) 

Open in new window

Thanks Ray, side effects of not testing code before posting.
0
 
Black SulfurAuthor Commented:
Oh, never mind. I forgot to do this:

<div><?php echo insert_categories($link);?></div>

Open in new window

0
 
Julian HansenCommented:
You need to pass $link to the function when you call it
<div><?php echo insert_categories($link);?></div>

Open in new window

0
 
Black SulfurAuthor Commented:
Was my reasoning for having both $error and $success okay or am I still wrong?

$error .= '<div class="alert alert-danger" role="alert">Category cannot be empty</div>';

$success = '<div class="alert alert-success" role="alert">Record added</div>';

Open in new window

0
 
Julian HansenCommented:
If you are going that route you need two bits of information
The result of the query and the message. A function can only return one value so you either need to return an object / array or you need to change your logic.

The quick fix is to just change the function  to create the entire <div> complete with alert classes - not really good design but it will work in the context of what you are trying to do.

Something like this
<?php
function insert_categories($link)
{
  $response = "";

  $cat_title = empty($_POST['cat_title']) 
    ? $link->real_escape_string($_POST['cat_title']) 
    : false;
  if ($cat_title) {
    $sql = "INSERT INTO `categories`(cat_title) VALUES('{$cat_title}')";
    $result = $link->query($sql);
    if ($result) {
      $response = '<div class="alert alert-success">Record added</div>';
    }
    //EDIT
    else {
      $response = '<div class="alert alert-danger">Insert failed with error ' . $link->error . "</div>";
    }
  }
  else {
    $response = '<div class="alert alert-danger">Category  cannot be empty</div>';
  }
  
  return $response;
}
// This gets called at the start of your script
$result = $insert_categories($link);
?>
...
<div class="form-group">
  <input type="submit" class="btn btn-primary "name="submit" value="Add Category">
  <?php echo $result?>
</div>

Open in new window

Sidebar: .= is a contactination operation - it means append to the variable the string on the right. Your $error value is clean so you don't need to do that.
0
 
Black SulfurAuthor Commented:
Thanks Julian. I tried your code and the error message displays instantly before even submitting the form.
0
 
Black SulfurAuthor Commented:
Sorry, one last thing. Why is it not good to echo out the errors like I had them with the div tags i.e.:

 $response = '<div class="alert alert-danger">Category  cannot be empty</div>';

Open in new window

0
 
Marco GasiFreelancerCommented:
It looks like you totally ignored my comment https://www.experts-exchange.com/questions/28968945/Form-validation-using-functions.html#a41792540
Or am I missing something?
0
 
Black SulfurAuthor Commented:
Sorry Marco, you are correct. I forgot to allocate some points to you. How can I change that?
0
 
Julian HansenCommented:
Because you are mixing your status messages with the presentation. If you want to change the styling of the message you have to go into the function and change it there.

Ideally you want your error message to be just the text.

One option is to assume that an empty message means success and have a default message so you could do this
$error= insert_category($link);
// Assume the worst
$class = 'alert-danger';

if (empty($error)) {
   $class = 'alert-success';
   $error = 'Record Added';
}
...
<div class="alert <?php echo $class;?>"><?php echo $error;?></div>

Open in new window

As for the message showing immediately - that is because there is no check for a post
$class = 'hidden';
if ($_POST) {
   // above code with call to insert_category here
}
// If no POST we drop through and class hidden kicks in and hides the status <div>

Open in new window

0
 
Marco GasiFreelancerCommented:
Ok, don't worry. I just wanted to understand what happened since the accepted solution presented a code substantially identical to mine.
On to the next :)
0
 
Black SulfurAuthor Commented:
I chose Julian's as the best because he showed me how to not use global. Apologies, I will try allocate better next time, it's not that easy! ;)
0
 
Black SulfurAuthor Commented:
@ Julian. That makes sense in your example but I don't know how to implement into existing function?

function insert_categories($link){


	$response = "";
	$class = 'alert-success';
	
	if (isset($_POST['cat_title'])) {
	
		if (!empty($_POST['cat_title'])){
			

		$cat_title = $link->real_escape_string($_POST['cat_title']);
		
			
			$sql = "INSERT INTO `categories`(cat_title) VALUES('$cat_title')";
			if ($result = $link->query($sql) == TRUE) {
				//$response = '<div class="alert alert-success" role="alert">Record added</div>';
				$response = "Record added";
				
			} 
			
		} else {
		
				$class = 'alert-danger';
			//$response = '<div class="alert alert-danger" role="alert">Category cannot be empty</div>';
			$response = "Category cannot be empty";
				
			}
			
		
			
		
			return $response;
		
	}

}

Open in new window



<div><?php echo insert_categories($link);?></div>

Open in new window

0
 
Black SulfurAuthor Commented:
I hope that's fair. I found both very helpful for different reasons.
0
 
Marco GasiFreelancerCommented:
Thank you Black Sulfur. It wan't necessary (as I said) but thank you again.
0

Featured Post

Concerto's Cloud Advisory Services

Want to avoid the missteps to gaining all the benefits of the cloud? Learn more about the different assessment options from our Cloud Advisory team.

  • 12
  • 6
  • 4
  • +2
Tackle projects and never again get stuck behind a technical roadblock.
Join Now