Validate data for insert, server side

Question

I have a function for parsing data, that goes to the database, (it's more validation, the sanitization process is taken care of by the framework). Here's my handmade script, based on a jQuery library, forgot its name. The comments are in Spanish.

/**
* Procesar Información.
*
* Compara la información proporcionada con las reglas establecidas para su validación.
* @param array $fields Arreglo con los campos y sus reglas de validación.
* @param array $data Arreglo con la información a validar.
* @param bool $strict Si es definido como true, marcará como ilegales los campos no definidos en las reglas,
* de lo contrario los ignora y toma como base las reglas.
* @return array Arreglo con los campos separados en legales e ilegales, de acuerdo a las reglas proporcionadas.
* @usage
* $reglas = array(
*   'nombre_campo'=>array(
*    'fieldname'=>'nombre_campo',
*    'label'=> 'Etiqueta descriptiva',
*    'rules'=> array(
*     'required'=>true,
*     'regex'=> 'Expresión regular',
*     'custom01'=>'funcion01', # Valida mediante funciones personalizadas.
*     'custom02'=>'funcion02'  # Deben devolver true o false.
*    ),
*    'errors'=> array(), # Opcional, debe coincidir con las reglas.
*     'required'=>'Mensaje de error para la regla required.',
*     #El resto de las reglas despliega un mensaje predeterminado de error.
*   ), ...
* );
* $datos = array('nombre_campo'=>'Valor a validar',...);
* $procesar = parseData($reglas, $datos);
*/

private function parseData(array $fields, array $data, $strict = false)
{

    // Campos válidos.
    $legal_fields   = array();
    // Campos no válidos.
    $ilegal_fields  = array();

    foreach ( $fields as $field=>$attr)
    { #START FIELDS_LOOP
        $isValid = true;
        $error_msg='';
        if(!empty($attr['rules']))
        {#START RULES_PARSE

            foreach($attr['rules'] as $rule=>$value)
            {#START RULES_LOOP

                // Si el valor no es válido, deja de comprobar el resto de las reglas.
                // y marca el campo como ilegal con su correspondiente mensaje de error.
                if(!$isValid)
                {
                    break;
                }

                switch($rule)
                {#START RULE_TYPE_SWITCH

                    // Para la regla requerido, se comprueba que la regla este definida como true y que el valor
                    // del campo no este vacío.
                    case 'required' :
                        $isValid = (!$value OR ($value=== true AND !empty($data[$attr['fieldname']])));
                        $error_msg = !empty($attr['errors'][$rule]) ? $attr['errors'][$rule]
                            : 'El campo '.$attr['label'].' es requerido.';
                        break;

                    case 'regex':
                    // Se compara el valor contra la expresión regular definida.
                        $isValid = (empty($value) OR 
                            empty($data[$attr['fieldname']]) OR 
                            (!empty($data[$attr['fieldname']]) AND preg_match($value, $data[$attr['fieldname']])));
                        $error_msg = !empty($attr['errors'][$rule]) ? $attr['errors'][$rule]
                            : 'El campo '.$attr['label'].' no tiene un formato válido.';
                        break;
                    case 'list':
                        $isValid = in_array($data[$attr['fieldname']], $value);
                        $error_msg = !empty($attr['errors'][$rule]) ? $attr['errors'][$rule]
                            : 'El campo '.$attr['label'].' no tiene un valor válido.';
                        break;
                    default:
                    // Para cualquier otro tipo de regla busca un método o función con el nombre definido
                    // y le pasa el valor del campo como argumento, la función debe devolver true o false
                    // únicamente despues de realizar la validación del valor.
                        if(!method_exists($this, $value) AND !function_exists($value))
                        {
                            throw new Exception('Method or function '.$value.' not found.');
                        }
                        $isValid = (empty($value) OR 
                            empty($data[$attr['fieldname']]) OR 
                            (
                                !empty($data[$attr['fieldname']]) AND 
                                ( 
                                    (method_exists($this, $value) AND $this->$value($data[$attr['fieldname']])) OR
                                    (function_exists($value) AND $value($data[$attr['fieldname']]))
                                )
                            )
                        );
                        $error_msg = !empty($attr['errors'][$rule]) ? $attr['errors'][$rule]
                            : 'El campo '.$attr['label'].' no contiene un valor válido.';
                        break;
                }#END RULE_TYPE_SWITCH

            }#END RULES_LOOP

        }#END RULES_PARSE

        if($isValid)
        {
            $legal_fields[$attr['fieldname']] = !empty($data[$attr['fieldname']]) ?$data[$attr['fieldname']] : '' ;
        }
        else
        {
            $ilegal_fields[$attr['fieldname']]['value'] = !empty($data[$attr['fieldname']]) ?$data[$attr['fieldname']] : '' ;
            $ilegal_fields[$attr['fieldname']]['error_msg'] = $error_msg;
        }
    }#END FIELDS_LOOP

    if($strict)
    {#START STRICT_MODE
        foreach($data as $fieldname=>$value)
        {
            if(!isset($fields[$fieldname]))
            {
                $ilegal_fields[$fieldname]['value'] = $value;
                $ilegal_fields[$fieldname]['error_msg'] = 'Campo '.$fieldname.' no reconocido.';
            }
        }
    }#END STRICT_MODE

    return array('legal' =>$legal_fields, 'ilegal'=>$ilegal_fields);

}#END parseData()

The usage it's very simple.

$parsedData = parseData($rules, $data);

Where the $data is a simple array with the data in the form $fieldname => $value and the $rules are an array with the fields description to validate. Example

$fields     = array(
        'name'=> array(
            'fieldname'=>'name',
            'label' =>'Nombre',
            'rules' => array(
                'required' => true,
                'regex'=>'/^\p{L}[\p{L} \']+$/u',
            ),
            'errors'=> array(
                'required' => 'Por favor proporcione su nombre.',
                'regex'=>'El nombre debe contener únicamente letras, acentos y espacios.',
            ),
        ),
        'apellidos'=> array(
            'fieldname'=>'apellidos',
            'label' =>'Apellidos',
            'rules' => array(
                'required' => true,
                'regex'=>'/^\p{L}[\p{L} \']+$/u',
            ),
            'errors'=> array(
                'required' => 'Por favor proporcione sus apellidos.',
                'regex'=>'Los apellidos solamente pueden contener letras, acentos y espacios.',
            ),
        ),
        'description'=> array(
            'fieldname'=>'description',
            'label' =>'Descripción',
        ),
        'fechadenacimiento'=> array(
            'fieldname'=>'fechadenacimiento',
            'label' =>'Fecha de Nacimiento',
            'rules' => array(
                'required' => false,
                'regex' =>'/^[0-9]{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[0-1])$/',
                'valid'=>'isValidDate',
            ),
            'errors'=> array(
                'required' => 'Por favor proporcione su fecha de nacimiento.',
                'regex'=>'La fecha debe ser en el formato aaaa-mm-dd.',
                'valid'=>'Por favor proporcione una fecha válida.',
            ),
        ),

The return is an array containing the legal (valid) and illegal (invalid) pair field values, the strict mode also verifies if the user add random unnecessary inputs on the contrary just ignore those. Any suggestions to improve it are welcome.

Answer 1

The first thing that struck me as odd was how you're declaring a private method, but that that method seems to be responsible for validating data from all sorts of places. If this is the case, then I think this method should've been either a simple global function, or a public method of a utility service of sorts.
Because your example of how this code is to be used is clearly calling a function, you're best to drop the private keyword all together, it'll generate an error if you use it outside of a class anyway:

private function parseData(array $fields, array $data, $strict = false)

Should be

function parseData(array $fields, array $data, $strict = false)

Next, you're not exactly sticking to the PSR standards. You might find it a bit silly to point this out, but trust me: it really pays off adopting the most commonly used coding standard out there. You'll find it easier to read other code, and vice-versa.

There's also a lot of arrays going on here. Your entire function is expecting to receive data in a very specific way:

• the keys must equal the field names
• The values must be arrays
• the value array must contain a rules key or the data cannot be validated
• The rules key must be an array. A non-empty string will pass the !empty($attr['rules']) check, but you can't foreach over a string...
• This rules array, again, must adhere to a rather specific format.

Now the only thing you can do in PHP is type-hint for either instances of a class/interface, or for arrays (I know: in PHP7, you can also hint for scalars, but that's not relevant here). It's nice to see that you're using type-hints, but think about it: I can pass a lot of things to this function

function iNeedData(array $data)
{}

Sure, you're documenting what the function is expecting extensively, but code-wise, there's nothing to prevent me from passing something like this:

parseData(range(1, 10), range(1, 2), true);

It would probably make more sense for you to create a ValidationRule class, use class constants for the validation types (the ones in your switch-case), and add a required property:

class ValidationRule
{
    const TYPE_REGEX = 1;
    const TYPE_LIST = 2;
    const TYPE_CALLBACK = 3;

    /**
     * @var int
     */
    protected $ruleType = null;

    /**
     * @var mixed
     */
    protected $ruleValue = null

    /**
     * @var bool
     */
    protected $required = false;

    //add methods to get/set the properties here
}

The way to use this class might look something like this:

$rule = new ValidationRule();
$rule->setType(ValidationRule::TYPE_CALLBACK)
    ->setRuleValue(function($val) {
        //validation callback to ensure a value
        //is a valid month, for example:
        return ($val == (int) $val && $val >= 1 && $val <= 12);
    })->setRequired(true);

So, these are a few recommendations/issues I spotted when I began to look at your code. However, these niggles are really not thát significant considering what follows.

The big problem

There is a problem here, that I feel I have to point out. It's not about inefficiencies or coding style, but more the code itself. Code like this really shouldn't exist in a well-design system.
It relies on the user (caller) to pass both the data and the validation rules in an arbitrary format. However, you're validating data that will be inserted in a database. Because you're using keys like "fieldname" I'm assuming the DB you're using is a traditional SQL style system (MySQL, MariaDB, PostgreSQL, ...).

Systems like that store data in a fixed format, and you're sort of expected to know what the schema looks like in order for you to use it. In that case, I'd really not bother passing arrays of rules to a single function/method to ensure I'm only storing valid data. Instead, I'd create classes that represent records in various tables (data objects, data models, entities... whatever you want to call them).

The advantages of doing this are:

• Changing a table requires you to change a single class, and the rest of your code can remain unchanged. Whereas now, every validation array you're passing might need changing, which is a nightmare
• You can use existing DBAL/ORM's to save time
• By using data objects, you're adding a layer of abstraction without adding too much overhead. This can be helpful if ever you're going to change your storage (Moving to a NoSQL database system, for example).
• Seeing as these classes represent certain data, these classes should be capable of validating the actual data. This means that the validation of a set of values can be centralized. If you use an instance of a given class, you can safely assume that the data you're getting from it has been properly validated.
• You can type-hint for a specific type of data. If you're passing an array you end up with code like this:

An example processing user-related data:

/**
 * Some comment explaining what data you need:
 * This method requires user data (email, id, and name)
 * @param array $user
 * @return mixed (returns something)
 * @throws InvalidArgumentException (in case the data was malformed)
 * @throws Exception (something went wrong processing the actual data)
 */
public function processUserData(array $user)
{
    $requiredKeys = [
        'email',
        'id',
        'name',
    ];
    foreach ($requiredKeys as $key) {
        if (!isset($user[$key])) {
            throw new \InvalidArgumentException(
                sprintf(
                    'Missing %s key from user array',
                    $key
                )
            );
        }
    }
    //finally, we can set about working on the data
}

If you have a User class that represents the user data (id, email, name, ...), you could simply change this method to something like this:

public function processUser(User $user)
{
    $email = $user->getEmail();//We're sure the email returned here has been validated already
}

So what could one of these data classes look like? Well, it's a pretty simple object with a property for each field in the table it represents, a getter to allow access to the values, and a setter to allow updating these values. The setters will be the methods that take care of validation:

class User
{
    /**
     * ID, if null: user is a new record to be inserted
     *
     * @var int|null
     */
    protected $id = null;

    /**
     * @var string
     */
    protected $email = null;

    /**
     * @var string
     */
    protected $name = null;

    /**
     * @return null|int
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getEmail()
    {
        return $this->email;
    }

    /**
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }

    /**
     * @param int $id
     * @return $this
     * @throws InvalidArgumentException
     */
    public function setId($id)
    {
        if (!is_numeric($id) || $id != (int) $id)) {
            throw new \InvalidArgumentException(
                'ID should be an integer'
            );
        }
        $this->id = (int) $id;

        return $this;
    }

    /**
     * @param string $email
     * @return $this
     * @throws InvalidArgumentException
     */
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new \InvalidArgumentException(
                sprintf(
                    '"%s" is not a valid email',
                    $email
                )
            );
        }
        $this->email = $email;

        return $this;
    }

    /**
     * Possible setter if you're storing name as VARCHAR(255)
     *
     * @param string $name
     * @return $this
     * @throws InvalidArgumentException
     */
    public function setName($name)
    {
        if (strlen($name) > 255) {
            throw new \InvalidArgumentException(
                sprintf(
                    '"%s" is too long',
                    $name
                )
            );
        }
        $this->name = $name;

        return $this;
    }
}

Note

I realize that this isn't really a code-review as much as it is a design review. But honestly: I could just restrict myself to reviewing the code you've written and wait for you to find out how hard it'll be to maintain code like that. But that would be silly. We've all written code like this when we started out, and most of us have learnt the hard way that sometimes, you just have to take that step back and reconsider the basis of your approach.