form validation - extreme critique needed

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

form validation - extreme critique needed

Post by s.dot »

So I'm developing a new site and I'm trying to get in as much OO practices as I can, in the best way I can. The following code posted is not intended to be a snippet or even really usable. This site will be closed source, and what I have works, so I could easily leave it as it is and call it a day. However, I'm eager to learn how things are done and the best way to do them.. it's a curse sometimes. :x

Right now, I'm doing error handling within the form validation class. This feels very ugly to me because "objects are supposed to do one thing, and do it well". Well, if it's validating, why should it be handling errors? Unfortunately I don't know of any other way to do it.

So here's my form validator base class

Code: Select all

<?php
 
/**
 * This is a form validator class to validate forms.
 *
 * @author Scott Martin <smp_info[at]yahoo[dot]com>
 */
 
//load in our rules
require_once 'class/formvalidator/rule/length.php';
require_once 'class/formvalidator/rule/required.php';
require_once 'class/formvalidator/rule/regex.php';
require_once 'class/formvalidator/rule/match.php';
 
 
class formValidator
{
    //container for rules
    protected $_rules = array();
    
    //container for potential errors
    protected $_potentialErrors = array();
    
    //container for errors
    protected $_errors = array();
    
    /**
     * Method to add rules
     */
    public function addRule(formValidator $rule, $errorMessage)
    {
        $this->_rules = array_merge_recursive($this->_rules, $rule->_add());
        $this->_potentialErrors = array_merge_recursive($this->_potentialErrors, $rule->_setError($errorMessage));
    }
    
    public function getErrors()
    {
        return $this->_errors;
    }
    
    //runs through different validating methods
    public function validate()
    {
        $this->_validateRequired();
        $this->_validateLength();
        $this->_validateRegex();
        $this->_validateMatch();
        
        return empty($this->_errors);
    }
    
    //checks required fields are filled in
    private function _validateRequired()
    {
        if (!empty($this->_rules['required']))
        {
            foreach ($this->_rules['required'] AS $required)
            {
                if (empty($_POST[$required]) || (trim($_POST[$required]) == ''))
                {
                    $this->_errors[] = $this->_potentialErrors['required'];
                    return;
                }
            }
        }
    }
    
    //checks that fields have the appropriate lengths
    private function _validateLength()
    {
        if (!empty($this->_rules['length']))
        {
            $it = 0;
            foreach ($this->_rules['length'] AS $field => $length)
            {
                $strlen = strlen($_POST[$field]);
                if (($strlen < $length['minLength']))
                {
                    $this->_errors[] = is_array($this->_potentialErrors['length']) ?
                        $this->_potentialErrors['length'][$it] : $this->_potentialErrors['length'];
                }
                
                if ($length['maxLength'] && ($strlen > $length['maxLength']))
                {
                    $this->_errors[] = is_array($this->_potentialErrors['length']) ?
                        $this->_potentialErrors['length'][$it] : $this->_potentialErrors['length'];
                }
                $it++;
            }
        }
    }
    
    //checks that fields have the appropriate characters
    private function _validateRegex()
    {   
        //regex rules
        if (!empty($this->_rules['regex']))
        {
            $it = 0;
            foreach ($this->_rules['regex'] AS $field => $pattern)
            {
                if (!preg_match($pattern, $_POST[$field]))
                {
                    $this->_errors[] = is_array($this->_potentialErrors['regex']) ?
                        $this->_potentialErrors['regex'][$it] : $this->_potentialErrors['regex'];
                }
                $it++;
            }
        }
    }
    
    //checks that field1 matches field2 in value
    private function _validateMatch()
    {
        //match rules
        if (!empty($this->_rules['match']))
        {
            $it = 0;
            foreach ($this->_rules['match'] AS $match)
            {
                if ($_POST[$match[0]] != $_POST[$match[1]])
                {
                    $this->_errors[] = is_array($this->_potentialErrors['match']) ? 
                        $this->_potentialErrors['match'][$it] : $this->_potentialErrors['match'];
                }
                $it++;
            }
        }
    }
}
And here are the rule classes

Code: Select all

<?php
 
class formValidator_Rule_Required extends formValidator
{
    private $_field;
    
    public function __construct($field)
    {
        $this->_field = $field;
    }
    
    protected function _add()
    {
        $ret = array();
        if (is_array($this->_field))
        {
            foreach ($this->_field AS $field)
            {
                $ret['required'][] = $field;
            }
        } else
        {
            $ret['required'] = $this->_field;
        }
        return $ret;
    }
    
    protected function _setError($errorMessage)
    {
        return array('required' => $errorMessage);
    }
}

Code: Select all

<?php
 
class formValidator_Rule_Length extends formValidator
{
    private $_field;
    private $_minLength;
    private $_maxLength;
    
    public function __construct($field, $minLength=false, $maxLength=false)
    {
        $this->_field = $field;
        $this->_minLength = $minLength;
        $this->_maxLength = $maxLength;
    }
    
    protected function _add()
    {
        $ret = array();
        $ret['length'] = array(
            $this->_field => array(
                'minLength' => $this->_minLength,
                'maxLength' => $this->_maxLength
            )
        );
        
        return $ret;
    }
    
    protected function _setError($errorMessage)
    {
        return array('length' => $errorMessage);
    }
}

Code: Select all

<?php
 
class formValidator_Rule_Regex extends formValidator
{
    private $_field;
    private $_regex;
    
    public function __construct($field, $regex)
    {
        $this->_field = $field;
        $this->_regex = $regex;
    }
    
    protected function _add()
    {
        $ret = array();
        $ret['regex'] = array($this->_field => $this->_regex);
        
        return $ret;
    }
    
    protected function _setError($errorMessage)
    {
        return array('regex' => $errorMessage);
    }
}

Code: Select all

<?php
 
class formValidator_Rule_Match extends formValidator
{
    private $_field1;
    private $_field2;
    
    public function __construct($field1, $field2)
    {
        $this->_field1 = $field1;
        $this->_field2 = $field2;
    }
    
    protected function _add()
    {
        $ret = array();
        $ret['match'][] = array($this->_field1, $this->_field2);
        return $ret;
    }
    
    protected function _setError($errorMessage)
    {
        return array('match' => $errorMessage);
    }
}
And here's a full page example of how I'm using it (register.php)

Code: Select all

<?php
 
//page title
$title = 'Sign up for a free account';
 
//page header
require 'header.php';
 
//control variable for whether or not to show the form
$showForm = true;
 
//was form posted?
if (!empty($_POST['action']) && ($_POST['action'] == 'register'))
{
    //form validation object
    require_once 'class/formvalidator/formvalidator.php';
    $fv = new formValidator();
    
    //add rules
    $fv->addRule(
        new formValidator_Rule_Required(array('username', 'password', 'passwordConfirm', 'email', 'emailConfirm')),
        'Please fill in all of the required fields.'
    );
    
    $fv->addRule(
        new formValidator_Rule_Length('username', 3, 25),
        'Your username must be between 3 and 25 characters in length.'
    );
    
    $fv->addRule(
        new formValidator_Rule_Length('password', 6, false),
        'Your password must be at least 6 characters in length.'
    );
    
    $fv->addRule(
        new formValidator_Rule_Regex('username', '/[\w]+/'),
        'Your username may only contain the characters a-z, 0-9, and underscores.'
    );
    
    $fv->addRule(
        new formValidator_Rule_Match('password', 'passwordConfirm'),
        'Your passwords do not match.'
    );
    
    $fv->addRule(
        new formValidator_Rule_Match('email', 'emailConfirm'),
        'Your email addresses do not match.'
    );
    
    if ($fv->validate())
    {
        echo 'validates';
        //do this
        $showForm = false;
    } else
    {
        //the form did not validate
        //assign the errors to the template
        $tpl->assign('errors', $fv->getErrors());
        
        //show the form again with posted values
        $tpl->assign('username', !empty($_POST['username']) ? htmlentities($_POST['username'], ENT_QUOTES) : '');
        $tpl->assign('email', !empty($_POST['email']) ? htmlentities($_POST['email'], ENT_QUOTES) : '');
        $tpl->assign('emailConfirm', !empty($_POST['emailConfirm']) ? htmlentities($_POST['emailConfirm'], ENT_QUOTES) : '');
    }
}
 
//assign the showForm control variable
$tpl->assign('showForm', $showForm);
 
//display the page
$tpl->display('register.tpl');
 
//page footer
require 'footer.php';
If I can ask a favor when you're critiquing, please try not to go off into a hundred different ways of changing things.. that really confuses me since I'm learning right now. And sometimes the technical terms get me too, but I'll ask if I don't know what something means. :)

Thanks a lot!
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
yacahuma
Forum Regular
Posts: 870
Joined: Sun Jul 01, 2007 7:11 am

Does it have a javascript component

Post by yacahuma »

Does it have a javascript component?
User avatar
yacahuma
Forum Regular
Posts: 870
Joined: Sun Jul 01, 2007 7:11 am

validation with client and server side component

Post by yacahuma »

Let me start by saying that I dont like javascript. Well, not really javascript, just the fact that every browser behaves different. In any case, having immediate feedback improves usability. I think it will be nice if your library has a JS component.

The library I created http://www.stccorp.net/stclib.html has only a JS component and not a server side. I have been meaning to implemented but have not have the time yet. Maybe I use your code.

But the idea is as follow.

Code: Select all

//create a field with a validator
$m1 = new STCText  (array('name' => 'money',
                          'label'    => 'Money:',
                          'value'    => $p->money,
                          'validator' = > new STCValidationMoney(array('req'=>'Please enter money amount','max'=>2000, min=>'1000')
);
$b1 = new STCButton(array('name'  => 'submit_bt',
                          'value' => 'Save',
                          'type'  => 'submit'));


/Form
$f = new STCForm (array('name'   => 'form1',
                                         'class'  => 'stcform'));

//You can use f->add($t1) or add many elemets at once
$f->add($m1);

//Set the command buttons
$f->setButtons(array($b1));

$alert ='';
if (isset($_POST['submit_btn'])
{
   //at this point the server side validation occurs. It is just a matter on going through each form element and calling the  
   //validator code
    if (!$f->isValid())
    {
         $alert  = >$f->getErrorMsg();
    }
}

//Create page
$p  = new STCPage  (array('doctype' =>'xhtml',
                          'alert' => $alert,
                          'css'   => array('stcstyle.css')));

$p->add($f);//add form to page

$p->draw();//draw page, all the js code is automatically created here for the client side validation

Then again I could just use my object to validate the data instead of the form

Code: Select all

if (isset($_POST['submit_btn'])
{
    $myObj = new MyObject($_POST);
    if (!$myObj->isValid())
    {
         $alert  = >$myObj->getErrorMsg();
    }
}

I think I am more incline to go with the latest.
Last edited by yacahuma on Wed Dec 12, 2007 2:54 pm, edited 1 time in total.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

I only do simple checks with javascript to aid the end user's experience. My question is not the code at hand, but rather the OO practices within the code. How/if it can be refactored, and if I made the best choices with deciding where things go.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

I separated to an error class for handling errors, but the set error method still came in the rules. So I'm thinking the rules should do their own validation, and leave the errors to the error class.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
yacahuma
Forum Regular
Posts: 870
Joined: Sun Jul 01, 2007 7:11 am

Some comments

Post by yacahuma »

The only weird part that I can see is

Code: Select all

public function validate()
   {
      $this->_validateRequired();
      $this->_validateLength();
      $this->_validateRegex();
      $this->_validateMatch();
      
      return empty($this->_errors);
Why not code the validation in such a way that you can do this

Code: Select all

public function validate()
{
   foreach ($rules as $rule)
     $rule->check();
}

I am just wondering what will happen if I extend your class and create a new routine. If I call the parent to validate my function will not be there so it will not be called. It will be nice if I can create a new validation rule and have it accepted by the parent
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Just a quick observation I want to make since I have been down this road before.

$fv->addRule(new formValidator_Rule_Required(array('username', 'password', 'passwordConfirm', 'email', 'emailConfirm')), 'Please fill in all of the required fields.');
//should probably be
$fv->addRule(array('username', 'password', 'email', 'emailConfirm'), new formValidator_Rule_Required, 'Please fill in all of the fields');

You want your rules as decoupled from it's manager as possible. All the rules should ever care about is does data X validate (yes or no). It should not care about the datasource (some may argue otherwise), nor should it care about the fieldnames. The rules can then be re-used in another application, or even used without it's manager.

Code: Select all

if (formValidator_Rule_Required::isAlpha('fdfsdr324fdsfs')) {
   echo 'Yay!';
}
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Just a quick observation I want to make since I have been down this road before.

Code: Select all

$fv->addRule(new formValidator_Rule_Required(array('username', 'password', 'passwordConfirm', 'email', 'emailConfirm')), 'Please fill in all of the required fields.');
//should probably be
$fv->addRule(array('username', 'password', 'email', 'emailConfirm'), new formValidator_Rule_Required, 'Please fill in all of the fields');
You want your rules as decoupled from it's manager as possible. All the rules should ever care about is does data X validate (yes or no). It should not care about the datasource (some may argue otherwise), nor should it care about the fieldnames. The rules can then be re-used in another application, or even used without it's manager.

Code: Select all

if (formValidator_Rule_Alpha::validate('fdfsdr324fdsfs')) {
   echo 'Yay!';
}
User avatar
mpeacock
Forum Newbie
Posts: 10
Joined: Thu Apr 12, 2007 9:07 am
Location: Mobile AL

Re: form validation - extreme critique needed

Post by mpeacock »

I posted a blog entry about using the Strategy pattern in field and form validation a couple years ago. This extended an example on the phppatterns site. Like yacahuma and Jcart, I completely agree that your solution would be more flexible if you encapsulate your rules from the manager. In my example, I extend the application of the Strategy pattern to the task of form validation in addition to individual field validation - and arrive at a framework kernel that you could apply to just about any form.

The old blog entry is here: http://blog.springboard-software.com/20 ... gy-pattern.

If I get some free time - I may update that site :)
Post Reply