Solved

Form validation using functions

Posted on 2016-09-10
28
34 Views
Last Modified: 2016-09-10
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
Comment
Question by:Black Sulfur
  • 12
  • 6
  • 4
  • +2
28 Comments
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41792532
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
 
LVL 82

Expert Comment

by:Dave Baldwin
ID: 41792533
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
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 41792535
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
 
LVL 30

Accepted Solution

by:
Marco Gasi earned 300 total points
ID: 41792540
$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
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41792543
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
 

Author Comment

by:Black Sulfur
ID: 41792560
Wow, so many good answers. Sometimes giving points is harder than trying to understand the solutions!
0
 

Author Comment

by:Black Sulfur
ID: 41792562
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
 
LVL 51

Assisted Solution

by:Julian Hansen
Julian Hansen earned 200 total points
ID: 41792568
@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
 

Author Comment

by:Black Sulfur
ID: 41792569
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
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41792570
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
 
LVL 108

Expert Comment

by:Ray Paseur
ID: 41792571
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
 

Author Comment

by:Black Sulfur
ID: 41792572
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
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 41792574
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
IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

 

Author Comment

by:Black Sulfur
ID: 41792575
Oh, never mind. I forgot to do this:

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

Open in new window

0
 
LVL 51

Expert Comment

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

Open in new window

0
 

Author Comment

by:Black Sulfur
ID: 41792578
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
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 41792588
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
 

Author Comment

by:Black Sulfur
ID: 41792602
Thanks Julian. I tried your code and the error message displays instantly before even submitting the form.
0
 

Author Comment

by:Black Sulfur
ID: 41792603
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
 
LVL 30

Expert Comment

by:Marco Gasi
ID: 41792606
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
 

Author Comment

by:Black Sulfur
ID: 41792608
Sorry Marco, you are correct. I forgot to allocate some points to you. How can I change that?
0
 
LVL 51

Expert Comment

by:Julian Hansen
ID: 41792610
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
 
LVL 30

Expert Comment

by:Marco Gasi
ID: 41792612
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
 

Author Comment

by:Black Sulfur
ID: 41792613
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
 

Author Comment

by:Black Sulfur
ID: 41792616
@ 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
 

Author Closing Comment

by:Black Sulfur
ID: 41792733
I hope that's fair. I found both very helpful for different reasons.
0
 
LVL 30

Expert Comment

by:Marco Gasi
ID: 41792837
Thank you Black Sulfur. It wan't necessary (as I said) but thank you again.
0

Featured Post

How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

Join & Write a Comment

This article discusses four methods for overlaying images in a container on a web page
This article discusses how to create an extensible mechanism for linked drop downs.
The viewer will learn how to dynamically set the form action using jQuery.
The viewer will learn how to count occurrences of each item in an array.

747 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

15 Experts available now in Live!

Get 1:1 Help Now