CODEWITHSUNDEEP
phpvalidation
Sven
Sven

Reputation: 13296

PHP form validation not working - several issues

At the moment I am trying to validate a form using PHP. The problem is, that even after entering something wrong, PHP interprets it as right. Obviously I don't now why, although I have an assumption. This is the code:

if(isset($_GET['contact'])){

    // Validation functions

    // Name
    function validate_name(){
        $name       =   $_POST['customer'];
        if(strlen($name) > 0){
            trim(mysql_real_escape_string($name));
            return true;
        }else {
            return false;
        }
    } 

    // Mail
    function validate_mail(){
        $mail       =   $_POST['mail'];
        if(preg_match('/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/', $mail) && strlen($mail) > 0){
            return true;
        }else {
            return false;
        }
    }

    // Message
    function validate_message(){
        $message    =   $_POST['message'];
        if(strlen($message) > 0){
            trim(mysql_real_escape_string($message));
            return true;
        }else {
            return false;
        }
    }

    validate_name();
    validate_mail();
    validate_message();

    if(validate_name == true && validate_mail == true && validate_message == true){
        echo "Ok!";
    }else{
        echo "Error!";
    }
}

One thing I know is bad is this: if(validate_name == true && validate_mail == true && validate_message == true){}. But if I am not mistaken, this still works because PHP can handle something like this (PHP only gives a notice, not an error). But how to do it right, there must be a better way?

The second this I found out is, that PHP basically calls the functions correct, but inside the functions the if-else is not working. Why? I don't understand this...

Upvotes: 1

Views: 1484

Answers (7)

PeeHaa
PeeHaa

Reputation: 72729

OK. Since it is weekend and I have nothing better to do I'll fix your issues and explain what is wrong with your code.

To start by answering your original question:

PHP only gives a notice

As the notice states:

Notice: Use of undefined constant validate_name - assumed 'validate_name' in...

This is because of the line where you do:

if(validate_name == true ...

validate_name is nothing. If it were a variable it would have been $validate_name if it were a function call it would have been validate_name(). So PHP assumes it is a constant. Without ranting about whether this is a smart move of PHP trying to 'help' you it does what it does. So basically PHP will handle it as a constant with a value of validate_name.

So what PHP does is the following:

if ('somestring' == true) // this will always be truthy.

Now to further fix / improve your code:

I'm also not sure about your use of superglobals. You use $_GET['contact'] and also $_POST['customer']. So you are mixing _POST and _GET. This could be correct not sure though.

If you have a form with an action of '/file.php?contact=something' and the form method is set to post this is perfectly fine.

Also it would be better to add params to the functions. So change:

function validate_name(){

To

function validate_name($name){

In stead of relying on the _POST values. This way you can test your code without needing any post data.

In your validate_name and validate_message function you are doing this:

trim(mysql_real_escape_string($name));
trim(mysql_real_escape_string($message));

There are two things wrong with this:

  1. It is only available in the scope of the function and you are not returning it (so why would you do it)
  2. The function names states that it validates, not that it does other stuff.

In your validate email function you do the following:

if(preg_match('/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/', $mail) && strlen($mail) > 0){

Besides the fact that I'm sure that that regex isn't getting all valid emailaddresses it would have been better to first check if the string is filled in.

Now to correct all your issues in the following code:

if(isset($_GET['contact'])){

    function validate_name($name)
    {        
        if(strlen($name) === 0) {
            return false;
        }

        return true;
    } 

    function validate_mail($mail)
    {
        if (strlen($mail) === 0 || filter_var($mail, FILTER_VALIDATE_EMAIL)) {
            return false;
        }

        return true;
    }

    function validate_message($message)
    {
        if(strlen($message) === 0) {
            return false;
        }

        return true;
    }

    if(validate_name($_POST['customer']) && validate_mail($_POST['mail']) && validate_message($_POST['message'])) {
        echo "Ok!";
    } else {
        echo "Error!";
    }
}

Upvotes: 2

Jeremy Giberson
Jeremy Giberson

Reputation: 1143

The first problem, is your functions return a boolean result they don't persistently represent a value. You're if conditional is basically just checking if the constants "validate_name", "validate_mail", and "validate_message" are defined. PHP when encountering an undefined constant will set the constant equal to itself. Then type coercion is comparing the string values "validate_name", "validate_mail" and "validate_message" to true which is does by converting both the boolean true value and the string values to a 1 value. They are then considered equal.

What you should be doing is:

$isNameOk = validate_name();
$isMessageOk = validate_message();
$isMailOk = validate_mail();

if($isNameOk && $isMessageOk && $isMailOk)
    echo "Ok!";
else
    echo "Error!";

The second unrelated errors are what you are doing the the functions. You are setting variables like $name, $mail and $message in the scope of the function (which is fine) but then you also issue trim and mysql_escape on them with out actually setting the value back to the variable. Ie it should be:

$name = trim(mysql_escape_string($name));

On top of that, you're applying these value sanitizers to your variables but your functions don't actually do anything with those variables. I'm assuming you want to later use the sanitized $name, $mail and $message variables in your script. However because they are defined in the local function scope that will never work.

The quick fix is to define your variables in the global scope and reference them in the function with the global key word but that is ugly. A better solution would be to objectify your code and make those values and functions members of a class object.

class Validator {
    protected $name;
    protected $isNameOk;
    protected $mail;
    protected $isMailOk;
    protected $message;
    protected $isMessageOk;

    public function validate($data) {
         $this->isNameOk = $this->checkAndSanitizeName($data['name']);
         $this->isMessageOk = $this->checkAndSanitizeMessage($data['message']);
         $this->isMailOk = $this->checkAndSanitizeMail($data['mail']);
         if($this->isNameOk && $this->isMessageOk && $this->isMailOk)
             return true;
         return false;
    }

    protected function checkAndSanitizeName($name) {
        $this->name = trim(mysql_real_escape_string($name));
        if(strlen($name) > 0)
            return true;
        return false;
    }

    protected function checkAndSanitizeMail($mail) {
        $this->mail = trim(mysql_real_escape_string($mail));
        if(preg_match('/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/', $this->mail) && strlen($this->mail) > 0)
            return true;
        return false;
    }

    protected function checkAndSanitizeMessage($message) {
        $this->message = trim(mysql_real_escape($mail));
        if(strlen($this->message) > 0)
            return true;
        return false;
    }

    public function getName() { return $this->name; }
    public function getMail() { return $this->mail; }
    public function getMessage() { return $this->message; }
    public function isMailOk() { return $this->isMailOk; }
    public function isNameOk() { return $this->isNameOk; }
    public function isMessageOk(){ return $this->isMessageOk; }
}

Usage would be:

$validator = new Validator();
if($validator->validate($_POST))
{
    echo "Ok!";
    echo "Thanks " . $validator->getName() . " for posting your message.";
}
else
{
    echo "Error!";
    if(!$validator->isMailOk())
        echo "Mail was invalid";
    // etc.
}

I banged this code out in the reply box so take it with a grain of salt. May need some typos fixed.

Upvotes: 3

prdatur
prdatur

Reputation: 1007

You can do it for example:
Its not the best solution but displays no notices or errors. happy coding.

if(isset($_GET['contact'])){
    // Validation functions
    // Name
    function validate_name(){
        if(!isset($_POST['customer'])) {
            return false;
        }
        if(strlen($_POST['customer']) > 0){
            $_POST['customer'] = trim($_POST['customer']);
            return true;
        }else {
            return false;
        }
    } 

    // Mail
    function validate_mail(){
        if(!isset($_POST['mail'])) {
            return false;
        }
        return preg_match('/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/', $_POST['mail']);

    }

    // Message
    function validate_message(){
        if(!isset($_POST['message'])) {
            return false;
        }
        if(strlen($_POST['message']) > 0){
            $_POST['message'] = trim($_POST['message']);
            return true;
        } else {
            return false;
        }
    }

    if(validate_name() == true && validate_mail() == true && validate_message() == true){
        echo "Ok!";
    }else{
        echo "Error!";
    }
}

Upvotes: 1

Travesty3
Travesty3

Reputation: 14479

  1. Change validate_name == true to just validate_name(). This will call the function and return the boolean. No need to compare something that's already a boolean to another boolean in order to produce a boolean...just use the returned value. Do the same thing for the other two functions.
  2. Inside your functions, you are calling the functions trim() and mysql_real_escape_string(), but you aren't saving those changes. These aren't passed by reference, so you need to save the change back to your $_POST variables.

Here's the updated code:

if(isset($_GET['contact'])){ 

    // Validation functions 

    // Name 
    function validate_name(){ 
        $name       =   $_POST['customer']; 
        if(strlen($name) > 0){ 
            $_POST["customer"] = trim(mysql_real_escape_string($name)); 
            return true; 
        }else { 
            return false; 
        } 
    }  

    // Mail 
    function validate_mail(){ 
        $mail       =   $_POST['mail']; 
        if(preg_match('/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/', $mail) && strlen($mail) > 0){ 
            return true; 
        }else { 
            return false; 
        } 
    } 

    // Message 
    function validate_message(){ 
        $message    =   $_POST['message']; 
        if(strlen($message) > 0){ 
            $_POST["message"] = trim(mysql_real_escape_string($message)); 
            return true; 
        }else { 
            return false; 
        } 
    }

    if(validate_name() && validate_mail() && validate_message()){ 
        echo "Ok!"; 
    }else{ 
        echo "Error!"; 
    } 
}

Upvotes: 2

Damien Pirsy
Damien Pirsy

Reputation: 25445

I see 3 problems so far:

  1. your functions are just returning true or false, not the modified supplied variable. Also, variables are subject to function scope, so are not available outside the function, if you just modify them inside and don't return them, or use global variables.
    If you don't return the values, then, why trimming and escaping them?
  2. mysql_real_escape_string() works if a connection is active, otherwise returns FALSE.
  3. in your if() condition, you're using "validate_name" but it's syntax is wrong, both if you intended it as a variable or a function (missing parenthesis)

Taking your first function, for ex., you could do (provided you've an active connection):

function validate_name($customer){
        if(strlen($customer) > 0){
            $customer = trim(mysql_real_escape_string($customer));
            return $customer;
        }else {
            return false;
        }
    }

and later on:

if(validate_name($_POST['customer'] AND ....)

Another thing I see, since validate_name() and validate_message() are doing the same operations, why not making it a single function (e.g., validate($postIndex)), passing the $_POST index as a parameter? It's a tiny bit more DRY. Like:

$name = validate($_POST['customer']);
$message = validate($_POST['message']);
$email = validate_email($_POST['email']);
if($name AND $email AND $message){

// now here you also have the trimmed/escaped values to work with.
}

Upvotes: 3

MrGlass
MrGlass

Reputation: 9262

You need to change the last bit of code a little. like this:

if(validate_name() && validate_mail() && validate_message()){
    //do stuff with the info here
}else{
    echo "Error!";
    //display error information and show them the form again.
}

Upvotes: 1

TheDude
TheDude

Reputation: 11

Looks as if you're not calling the functions you've written in the if-block. Also you could shorthand that statement.

if( validate_name() && validate_mail() && validate_message() ) {echo 'Ok!';}

Because all of these are returning boolean results, the if statement doesn't require an explicit == true check. Also, you'll want to be careful to sanitize any GET/POST variables.

Upvotes: 1

Related Questions