Improvements to the validation

[stojg]

I could not find a reference to the validation improvements for the Framework in issues.

The only information I've been able to find are:

http://open.silverstripe.org/wiki/development/validation http://open.silverstripe.org/wiki/development/validation-new

There were a GSOC 2012 project, but I'm not sure where that ended up. @chillu, @halkyon, @mateusz I think you been involved in this before. If you have any information to share that would be great.

[sminnee]

The GSOC project kind of died on the vine. Here was my view of the necessary work:

• Amend the Form system so that a ValidationException could be thrown in order to trigger validation errors. I had a gist around for this that I've misplaced. It wasn't a complex piece of work; it mainly involved adding the notion of a field-specific error to ValidationResult, and then getting Form.php to make use of ValidationException / ValidationResult.
• Add the notion of a constraint, which would something like "FirstName is required" or "Email must match this regex". They could be applied to either a DataObject, or a Form. They can just throw ValidationExceptions, and upstream services (notably a form) would handle them. A ConstraintSet might be created to validate each constraint and combine all the errors into a single ValidationResult. throw -> catch -> combine -> rethrow may seem a little more complex than just returning ValidationResults, but it means that a constraint can be used on its own more easily.
• Possibly, build a piece of JS that reads all the constraint configuration and replicates that validation in a jQuery plugin.
• Possibly, add the ability to post to <formurl/validiate> and just run the validation. Then you could get front-end validation on blur without needing to recode validation in JS.

[sminnee]

Aha: https://github.com/sminnee/sapphire/commit/8074970928565d2a35ca20b8e328f6fe89d15282

[sminnee]

OK - I spent a couple of hours getting that over the line and I've pushed it here: https://github.com/silverstripe/silverstripe-framework/pull/2119

This PR addresses the first of my bullet points above.

[sminnee]

Following on from #2119 I would see the following changes make sense:

• Introduce an interface, Validatable, that is implemented by Form, FormField, and DataObject. It defines a method, validate($result) that is passed a ValidationResult that it expects to manipulate.
  • This involves an API change in Form::validate() and DataObject::validate(), but it means that everyone has the same call signature for these things.
• Introduce Validator::validateData($data, $result), that behaves very similarly, but the data of the form is passed in the $data argument, in something that implements ArrayAccess. DataObjects, Forms, and Fields validate themselves; Validators need to validate someone else. validateData() would need to be constructed so that DataObjects and Forms can be passed directly; Form may need to implement array-access to support this.
• Create a CompositeValidator (or ValidatorList?), that will validate against a list of Validatable sub-items. If those sub-items are Validators themselves, then the subject is passed on and validateData() is used, otherwise validate() is called. This lets you, for example, bundle all the FormFields up in a CompositeValidator and call validate() on it.
• Allow of a validator to be attached to both a Form and a DataObject
• Allow for a config static to define the default validators for a DataObject (and maybe a form?)

That would basically get you to the "let's do this properly" state that I know has been a desire for a long while.

But what about Constraints? Implicit in this is the idea is that a "constraint" is nothing but a validator that only bothers looking at one field. So, if you did this:

$dataObject->addConstraint(new RequiredField("Title"));

Then RequiredField would be a Validator that would execute like so:

function validateData($data, $result) {
  // $data would be the actual DataObject, but it implements ArrayAccess.
  if(!$data['Title']) $result->addFieldError("Title", "Title is required");
}

Of course, in this case, Title would be a parameter passed to the constructor of RequiredField and then referenced, but you get the idea. You would probably have a base class for field-specific validators, and you might even call that base-class "Constraint". But I would probably call it FieldValidator, as that seems more consistent. Something like this:

abstract class FieldValidator extends Validator {
   protected $field;
   function __construct($field) {
      $this->field = $field;
   }

   function validateData($data, $result) {
       $this->validateField($this->field, $data[$this->field], $data, $result);
   }

   abstract function validateField($fieldName, $fieldValue, $data, $result);
}

class RequiredField extends FieldValidator {
    function validateField($fieldName, $fieldValue, $data, $result) {
      if(!$fieldValue) $result->addFieldError($fieldName, "$fieldName is required");
    }
 }

Given all of this, you could define constraints on a DataObject by:

• Having a $validator parameter
• Setting it to a CompositeValidator containing a number of FieldValidators
• Have DataObject::validate($result) call $this->validator->validateData($this, $result);

One thing I'm less clear on is the format of the config static for defining the constraints.

[sminnee]

This begs the question "should we rely on Symfony constraint classes", as referenced here: http://symfony.com/doc/current/book/validation.html

My view on this is that while the volume of constraints they have is nice, trying to weld their system to be a part of ours is too much work to be feasible, and I don't think it's critical.

If someone has the desire, they could plumb between Symfony validators or constraints, but I would see that as an done with an optional adapter class rather than being a part of the core validator scheme in SilverStripe.

They have a lot of constraints objects; although for simple things like required fields pre-existing objects are helpful, at a pretty simple level of complexity it becomes easier just to do this:

function validate($result) {
   parent::validate($result);
   if($this->Type == "Child" && $this->Age > 15) $result->addFieldError("Children shouldn't be older than 14");
 }

I would focus the core validators/constraints on

• Required fields
• Min/max length
• Simple inequalities (perhaps even with a single class that take a string '>','<','>=',etc as its first argument).

[chillu]

Sounds great :) I agree that Symfony constraints as such won't offer us much benefits given how simple most of them are. It would make more sense if used together with the Symfony Form and Validator components, but that's probably a too disruptive change for SilverStripe.

I like that we keep validators stateless through passing in ValidationResult, and the composition powers this allows.

Presumably you want to keep the static DataObject::$validations approach outlined in the wiki, which generates a model-specific validator on request? We'd need to cache or register these validators somewhere, so we don't create a new one for each model instance.

Do you see model binding in forms as a next step, and have FieldValidator specified manually for now? The wiki already outlines an approach for this. I want to make sure the validator holds up to more complex scenarios: How would you validate a form submission with a list of new users for example? (in the format User[0]['FirstName], User[1]['FirstName], ...). The wiki suggests using CompositeField with a bindToModel() method. Looking at your approach, I guess you'd do this in a custom Form->validate() method, where you call Validator->validateData() on the specific parts of the submitted data?

[sminnee]

We'd need to cache or register these validators somewhere, so we don't create a new one for each model instance.

We could potentially use the Injector system for this, and treat the DataObject::$validations as a special kind of DI configuration. To be honest, this is the part of the system that I am least clear on, and I would probably start by writing some test apps just by instantiating validators inside DataObject::validate() or something.

Do you see model binding in forms as a next step, and have FieldValidator specified manually for now?

The current system doesn't so much bind models to forms, as it does assume that the fieldnames on field-specific errors are the same between forms and dataobjects.

So, on a DataObject's validator you could do:

 $result->addFieldError("FirstName", "I don't like your name");

And it would show up as an error on the FirstName field of the relevant form.

Certainly for this default case this works well; it's not clear what we'd need to do for non-default cases. I guess it basically comes down to re-mapping -- "an error on field X of the model should be shown on field Y of the form". Something like:

 try {
    $model->write();
 } catch(ValidationException $e) {
    $e->getResult()->remapFieldErrors(array("Title" => "MyTitleFormField"));
    throw $e;
 }

A little bit verbose, but it's a pretty rare use-case. The only extra piece of code you would need is to implement remapFieldErrors(), which is trivial.

[tractorcow]

Form validation has been rewritten in 4.x, and includes the above discussed enhancement around using exception handling to trigger and safely display errors. I'm closing this, as any future enhancements to validation will likely need to be implemented from scratch and in a react-form supporting way.