1

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...

Sven
  • 12,997
  • 27
  • 90
  • 148
  • `PHP only gives a notice, not an error` What about fixing the notice?? – PeeHaa Jan 13 '12 at 20:59
  • Thats why I am asking how to do it better. – Sven Jan 13 '12 at 21:01
  • You can make it better by fixing the notice ofc. Since you failed to add the notice I am just guessing. But you are doing: `validate_name`. What is this??? – PeeHaa Jan 13 '12 at 21:02
  • `Notice: Use of undefined constant validate_name - assumed 'validate_name' in...` That is the notice for validate name. validate_name is a function I've written in the code above. All information I got about this notice is from http://stackoverflow.com/questions/2941169/what-does-the-php-error-message-notice-use-of-undefined-constant-mean – Sven Jan 13 '12 at 21:05
  • are you getting the correct variables in each case? I mean, if you echo each variable right before each if() are you getting what you expect or a valid string? – Claudio Jan 13 '12 at 21:06
  • This is obviously not gonna work. – Vultour Jan 13 '12 at 21:09
  • @Sven please see my updated answer which should solve all you problems. – PeeHaa Jan 13 '12 at 21:54

7 Answers7

3

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.
}
Damien Pirsy
  • 25,319
  • 8
  • 70
  • 77
  • One gotcha to watch out for with evaluating a string as a boolean, is to keep in mind what translates to `true` and what translates to `false`. While unlikely, perhaps a valid message is the string "0". This will evaluate as `false` when converted to a boolean. If you were going to do it this way, I would suggest changing the if-statement to `if ($name !== false && $email !== false && $message !== false)`. – Travesty3 Jan 13 '12 at 21:22
  • @Damien I would advise against making a generic `validate()` function. What if you would want to add some more conditions for the message for example (e.g. check for minimum / maximum characters). A better to make it more generic would be to create a function like: `isMinimumLength($string, $length = 0)`. Now you could just use that function in the `validate_*` functions. This way you could easily add extra checks to the validation functions. – PeeHaa Jan 13 '12 at 22:06
  • @Damien +1 for the comment about the active connection though. – PeeHaa Jan 13 '12 at 22:07
  • @PeeHaa I'm not saying to make a generic function (and, especially, an all-purpose function), I'm sorry if I gave that impression (I might have picked the wrong name). I was just saying that, in this case, since both are doing the same exact operation, they could be reduced down to a single function with a different parameter. Buth your osservation is more than correct, and I agree with you. – Damien Pirsy Jan 13 '12 at 22:13
  • @Downvoter May I know the reason for a downvote? I'm also here to learn – Damien Pirsy Jan 13 '12 at 22:15
  • Thank you Damien Pirsy, your post also really helped me to understand it. It's always nice to know more than one way of doing it! – Sven Jan 14 '12 at 11:18
3

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.

Jeremy Giberson
  • 1,063
  • 8
  • 15
  • While nice and all, is a whole class like that necessary to validate 3 input fields? OP didn't specify the width of his project, maybe it's just a simple contact form. In a bigger picture, a validator class it's a good feature, though – Damien Pirsy Jan 13 '12 at 21:28
  • Although the class is indeed a bit oversized for that project, that post really helped me to understand how it would work. Thanks! – Sven Jan 14 '12 at 11:15
2

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!";
    }
}
PeeHaa
  • 71,436
  • 58
  • 190
  • 262
  • 1
    Just one minor correction: Instead of `filter_var($email, FILTER_VALIDATE_EMAIL)` it has to be `filter_var($mail, FILTER_VALIDATE_EMAIL)`. – Sven Jan 14 '12 at 11:49
2
  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!"; 
    } 
}
Travesty3
  • 14,351
  • 6
  • 61
  • 98
1

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.

TheDude
  • 11
  • 1
1

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.
}
MrGlass
  • 9,094
  • 17
  • 64
  • 89
1

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!";
    }
}
prdatur
  • 1,010
  • 1
  • 14
  • 20