2
\$\begingroup\$

Here is a class that I made to validate forms data. I will really appreciate any criticism and hints.

  <?php

class formvalidator {

    public $filtered, $errors,$db,
    $fields_type = Array(), $error_msgs = Array();

    public function validate($form, $fields, $error_msgs) {

        $this->fields_type = $fields;
        $this->error_msgs = $error_msgs;

        foreach ($form as $field => $data) {

            if (isset($this->fields_type[$field])) {
                if (method_exists($this, $this->fields_type[$field])) {
                    $_POST[$field] = call_user_func_array(array($this, $this->fields_type[$field]), array($data));

                    if ($_POST[$field] === false) {
                        $this->errors[$field] = sprintf($this->error_msgs[$field], $data);
                     }
                }
            } else {//Else ? unset it Security ?
                unset($_POST[$field]);
            }
        }

        //Manual validation for password and password confirmation 
        if($_POST['form'] == 'signup' or $_POST['form'] == 'login')
        if (!isset($_POST['password'])) {
            $this->errors[$field] = sprintf($this->error_msgs['password'], '');
        }
        if (isset($_POST['repassword']) and $_POST['password'] != $_POST['repassword']) {
            $this->errors[$field] = sprintf($this->error_msgs['repassword'], '');
        }
        //End of password confirmation and password check.

        //
        $_POST = $this->clean($_POST);    

        if (is_null($this->errors)) {
            return true;
        } else {

            return false;
        }
    }

    public function value_calss($input) {
        if (isset($_POST[$input])) {
            if (isset($this->errors[$input])) {
                echo ' error ';
            } else {
                echo '" value = "' . $_POST[$input];
            }
        }
    }

    public function errors() {

        if (!is_null($this->errors)) {
            echo '<div class=" messagebox errorbox"><br><h4>Please Fix these:</h4><br><ul>';
            foreach ($this->errors as $error)
                echo "<li>$error</li>";
            echo '</ul><br></div><br>';
        }
    }

    public function setError($error) {
        $this->errors[] = $error;
    }


    //Validations

    public function IsEmail($email) {
        return filter_var($email, FILTER_VALIDATE_EMAIL);
    }

    public function IsAlphaNum($str) {
        if (ctype_alnum($str))
            return trim($str);
        else
            return false;
    }

    public function IsString($str) {
        if (preg_match("/^[A-Za-z0-9\s]+[^ ]$/", $str))
            return trim($str);
        else
            return false;
    }

//Explite will return false on empty values which will cause raise error flag.
    public function explite($input) {
        return trim($input);
    }


    public function text($input){
        return trim(htmlentities($input));
    }

    public function IsDate($is_date){ 
        if(strtotime($is_date))
            return date("Y-m-d",strtotime($is_date));
        else
            return false;
    }

    public function file($input)
    {
        return $input;
    }

    public function boolen($input)
    {
        if($input)
            return true;
        else false;
    }


    public function clean($input) {
        $clean = Array();
        foreach ($input as $field => $data) {
            $clean[$field] = mysql_real_escape_string($data);
        }
        return $clean;
    }




}

?>

Update: New Version on gist

\$\endgroup\$
1
  • \$\begingroup\$ Comment to self: Rename valure_class() method with attributes() as it either returns class or value. \$\endgroup\$
    – omeid
    Commented Oct 12, 2011 at 1:36

1 Answer 1

2
\$\begingroup\$

One thing that I see right off the bat, is the clean() method. Why are you escaping things arbitrarily?

public function clean($input) {
    $clean = Array();
    foreach ($input as $field => $data) {
        $clean[$field] = mysql_real_escape_string($data);
    }
    return $clean;
}

All you're doing is making it harder to trace code, and harder to prove if your code is secure or not. Not to mention that's a major breach of the Single Responsibility Principle.

It's a huge side-effect and should be removed.

Always escape or quote as close as possible to where you're putting it.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.